Fix for Invalid Service Token error - Refresh Token Duration same as Access Token #152
Conversation
|
This should probably target #111 rather than dev, which already aims to update several authentication systems and already has reworks for the token generation |
|
I feel 30 days is a bit too much? The console will ask for a new token on a fresh boot anyway. I think I would put it between 1 and 7 days |
|
The official servers likely only do 24 hours in NNAS. I just replayed an oauth request I had in Charles from 4 days ago but swapped from password auth to refresh token auth, and it said the token was invalid already. It's definitely not 30 days, but doesn't seem to be 7 either For our own tokens for the website and stuff we can raise that, but for NNAS we should keep it at 24 like the official servers |
From what I can see, the PR doesn't change any token generation code? |
Happy to update to use 24 hours for NNAS 👍 Happy for 30 days for website tokens? |
You're right, generation was the wrong word to use there. Utilization is what I was looking for. The PR does not change the structure of tokens, but it changes how routes utilize tokens since now there's explicit handling of each token type in The main point being, #111 is already designed for updating auth/security and these are also auth changes, so these changes should probably target the existing PR to keep everything in sync and in one place. There's already a PR for these kinds of changes, so it makes more sense imo to keep working off that one The existing PR is nearly ready to be merged, the last thing I wanted to do was rework the middleware some to add more authentication checks. I'm still bouncing the ideas in my head right now for how to go about it (since the Wii U only sends certain data like the device certificate in some routes, but not others whereas the 3DS sends it every time, so I have to see what can be checked where and when to bypass certain checks, etc. etc.), but I should be able to finish that out by friday
I don’t have a strong opinion either way for this. Dani mentioned thinking this sounds too long, which I do agree 30 days IS a bit long, but again no strong opinion. What is the standard for this? |
|
Sure, I'll rebase off your branch then if it's going to be sorted this week 👍 I just wanted to avoiding PRing into a heavily in-progress PR, which I thought it was due to the TODOs. Since that's not the case, I'll get it rebased and re-targetted
In my experience 30 days is the usual for refresh token duration There is an argument that we could set it to be something like 14 days, since the refresh token gets reset on every auth Since you feel 30 days is a bit long, I say we reduce it to 14 days which I'd say is plenty |
|
My concern of 30 days was only on NNAS, I'm fine with doing 30 days everywhere else |
jonbarrow
force-pushed
the
auth-updates
branch
from
d803094 to
d28ccbd
Compare
…se oauth generation
binaryoverload
force-pushed
the
token-fix
branch
from
52d4700 to
577b502
Compare
binaryoverload
requested review from
jonbarrow
and removed request for
jonbarrow
| let pnid: HydratedPNIDDocument | null; | ||
|
|
||
| if (grantType === 'password') { | ||
| pnid = await getPNIDByUsername(username!); // * We know username will never be null here | ||
| if (!username) throw new ServerError(Status.INVALID_ARGUMENT, 'Invalid or missing username'); |
There was a problem hiding this comment.
Style nit for the common config, we always use full blocks
There was a problem hiding this comment.
Will be linted later
There was a problem hiding this comment.
I'm not sure what this response means. I made the comment because I was under the impression that this was not part of the common config (hence why I said "for the common config"). It's not clear to me what the resolution here is based on this response. Is it missing and will be added later? Does it exist and the linter will just be ran later? Something else?
There was a problem hiding this comment.
We agreed previously that if it's not formatted by the linter, review comments won't be made on it.
The eslint PR #151 hasn't been merged yet, so this will be formatted in accordance with the linter when both are merged
I'll update common-config
There was a problem hiding this comment.
This has been resolved in PretendoNetwork/common-configs#2
There was a problem hiding this comment.
More like PretendoNetwork/common-configs@3399c72
There was a problem hiding this comment.
We agreed previously that if it's not formatted by the linter, review comments won't be made on it
I know you said you misunderstood on Discord, but just to clarify (and to make the clarification public): comments like this are mean't only to bring up styling examples to be added to the common config, not necessarily to fix them in the current PR (though that is also welcomed). I figured I would save making an issue and just bring it up here with a real code example. Would you prefer issues on the common config repo directly instead?
There was a problem hiding this comment.
Yeah in future I think making an issue on the common config repo would be better 👍
Resolves 115-5016 error occurring in Juxt
Changes:
This issue was raised by imora7024 in the Discord. The problem was getting the error 115-5016 (Invalid Service Token) which, through a Charles dump, was correlated to an OAuth request failing with token auth, and then trying again with password auth.