Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Enhancement]: Centralize response creation #160

Open
1 task done
jonbarrow opened this issue Feb 3, 2025 · 2 comments
Open
1 task done

[Enhancement]: Centralize response creation #160

jonbarrow opened this issue Feb 3, 2025 · 2 comments
Labels
awaiting-approval Topic has not been approved or denied enhancement An update to an existing part of the codebase

Comments

@jonbarrow
Copy link
Member

Checked Existing

  • I have checked the repository for duplicate issues.

What enhancement would you like to see?

Create a helper function to centralize the creation of responses in some services. This is mainly aimed at NNAS but might be useful in other services as well.

The motivation behind this is the fact that our response creation is very clunky in a lot of places. Lots of repeated code, some areas missing things, lots of duplicate calls to set headers, etc.

I noticed this when looking at how #111 adds in a warning comment for the OAuth endpoint. This comment is supposed to warn users that sharing details could compromise their data like their device certificate (we know of at least 1 case of a user being tricked into giving up their device certificate this way, which was the motivation for the comment). I wanted to add this warning to other endpoints, since basically every NNAS endpoint needs it, and realized how much duplicate code that would be.

Any other details to share? (OPTIONAL)

I'm thinking maybe something like this?

import express from 'express';
import xmlbuilder from 'xmlbuilder';

type CreateNNASErrorResponseOptions = {
	status?: number;
	error: {
		cause?: string;
		code?: string;
		message?: string;
	}
}

type CreateNNASResponseOptions = {
	status?: number;
	body: Record<string, any>;
}

const router = express.Router();

router.post('/access_token/generate', async (request: express.Request, response: express.Response): Promise<void> => {
	// Call createNNASErrorResponse if error, otherwise createNNASResponse
});

function createNNASErrorResponse(response: express.Response, options: CreateNNASErrorResponseOptions): void {
	createNNASResponse(response, {
		status: options.status || 400,
		body: {
			error: options.error
		}
	});
}

function createNNASResponse(response: express.Response, options: CreateNNASResponseOptions): void {
	response.set('Content-Type', 'text/xml');
	response.set('Server', 'Nintendo 3DS (http)');
	response.set('X-Nintendo-Date', new Date().getTime().toString());

	const body = xmlbuilder
		.create(options.body)
		.commentBefore('WARNING! DO NOT SHARE ANYTHING IN THIS REQUEST OR RESPONSE WITH UNTRUSTED USERS! IT CAN BE USED TO IMPERSONATE YOU AND YOUR CONSOLE, POTENTIALLY GETTING YOU BANNED!').end();

	response.status(options.status || 200).send(body);
}

This way we can easily just call the helper functions and have them set the headers and warning and all that

It should be noted that I'm unsure how exactly to handle situations where a NNAS error expects a format like:

<error>
	<cause>grant_type</cause>
	<code>0004</code>
	<message>Invalid Grant Type</message>
</error>

Rather than:

<errors>
	<error>
		<code>0108</code>
		<message>Account has been banned</message>
	</error>
</errors>

It seems like sometimes the outer <errors> element is expected, and other times it is not. I could be mistaken about this though? @DaniElectra @shutterbug2000 @InternalLoss can we confirm if this is the case?

@jonbarrow jonbarrow added awaiting-approval Topic has not been approved or denied enhancement An update to an existing part of the codebase labels Feb 3, 2025
@AToska21
Copy link

AToska21 commented Feb 4, 2025

If NNAS sometimes expects an outer <errors> element, maybe there could be an option in createNNASErrorResponse which would toggle this element on?

@jonbarrow
Copy link
Member Author

The thing is, I'm not actually sure if it's optional or not. The longer I think about it, the more a part of me is convinced that it always expects the outer element and any cases in our implementation that are missing it are just wrong

That's why I tagged some people to help verify this. I forgot to tag @ashquarky though so I'll do that now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-approval Topic has not been approved or denied enhancement An update to an existing part of the codebase
Projects
None yet
Development

No branches or pull requests

2 participants