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

added /api/auth/tokens endpoint to return oauth tokens #425

Closed
wants to merge 51 commits into from

Conversation

tomvoss
Copy link

@tomvoss tomvoss commented Jul 12, 2020

A method to retrieve OAuth tokens from the database was needed in order to use them with third-party APIs as Bearer tokens.

Fumler and others added 30 commits July 10, 2020 12:33
When using a provider that uses Token ID option (like Apple) a user hitting cancel with no longer cause the app to crash.

Users who do this will now be taken back to the sign in page.

This was already working for other providers that didn't use this option but wasn't supported for providers that did use it.
* add: marquee provider section
* fix: lint
* update: adjust node sizes
* fix: window undefined SSR
* fix: path to imgs

Co-authored-by: Iain Collins <me@iaincollins.com>
Includes breaking changes for v3 and updates to documentation.

If using the client, the only required change should be setting the NEXTAUTH_URL environment variable.
Passing a redirect function like this is a bit horrible, but is less horrible than before.
* clientMaxAge now passive
* clientPollInterval added (works like old clientMaxAge)
* poll intervals uses timer (more efficent)
* updates state on window focus/blur
Improves how well syncing client state is handled and how well caching is leveraged.

Reduces network load, cpu load and memory footprint.
This should never be called server side, but just in case someone calls setOptions server side this prevents it from being invoked at all.
* This is a breaking change in v3
* Includes updated documentation
Unproven, but should fix nextauthjs#395 and improve middleware compatibility.
Accidentally squashed a couple of lines in OAuth callback.
A method to retrieve OAuth tokens from the database was needed in order to use them with third-party APIs as Bearer tokens.
@vercel
Copy link

vercel bot commented Jul 12, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/iaincollins/next-auth-docs/7e9dtj6xc
✅ Preview: https://next-auth-docs-git-fork-tomvoss-feature-api-auth-accounts.iaincollins.vercel.app

@vercel vercel bot temporarily deployed to Preview July 12, 2020 21:55 Inactive
@tomvoss
Copy link
Author

tomvoss commented Jul 13, 2020

I just noticed that token rotation is not handled so having access to the OAuth refresh and access tokens isn't terribly helpful yet.

@iaincollins
Copy link
Member

@tomvoss Thanks for starting work on this!

This is great and we should leave it open for a bit, until we have time to come back to it, if you don't mind?

The plan regarding token rotation is has been to handle it like this but call the API for the providers a user has to get a new access token if they existing access token has expired (and if it has, save the new token to the database, with the new expiry time).

What isn't clear yet is how many providers return the token in a similar way - basically how many providers will require special handling (probably not many I think) and/or don't actually return tokens.

Because going out to a third party service is a blocking call (even if most of the time we can re-use the result from the database) is probably sensible to expose per-provider endpoints (e.g. /api/auth/tokens/twitter or some thing like that) though technically we could also provide one endpoint that returns all provided tokens if that is easier to consume.

I don't think we should merge this it in yet, given the above, but maybe in a branch where we can work on that?

I think we could build on this to do that!

A method to retrieve OAuth tokens from the database was needed in order to use them with third-party APIs as Bearer tokens.
@vercel vercel bot temporarily deployed to Preview July 14, 2020 16:27 Inactive
@tomvoss
Copy link
Author

tomvoss commented Jul 14, 2020

I went ahead and added the /api/auth/token endpoint and added logic to return a specific token type. It defaults to returning the access token, but could be easily modified to return all tokens or no tokens by default. It's currently limited to database sessions only.

My priority is to work towards automatically renewing expired tokens. My database's accounts.access_token_expires value is null for my Cognito account, but I was expecting it to contain the value returned from the provider with the OAuth tokens. Without integrating other providers into my project I can't know if it's null for other providers. It would be helpful if someone else would check their provider's value. I think this value will be important to know when to proactively renew an access token by exchanging the refresh token. The same is likely true for the refresh token itself.

@tomvoss
Copy link
Author

tomvoss commented Jul 14, 2020

I see now I probably should have used the plural form and made the endpoint /api/auth/tokens instead. I will fix that. While I'm at it I'll remove the /api/auth/accounts endpoint as I'm not sure that's necessary at this point.

@iaincollins
Copy link
Member

@tomvoss Thanks Tom! Yes this looks great!

Yeah the expires value is currently blank for all providers as it's not saved. I'm very happy to add that to support this!

It shouldn't be hard to add I just didn't want to add it without verifying it was going to be stored correctly (i.e. without the date/time getting mangled) and I don't think I had time to test that so left it as is with the expectation of coming back to it.

@iaincollins
Copy link
Member

I didn't expect to get this feature in v3 but given all this work it seems like worth a shot! :-)

I'll take a look at fixing what is missing (and doing it for both adapters) over the weekend.

Thanks for looking at this! I know there are a lot of moving parts and it's not an easy project to get started contributing to!

@vercel vercel bot temporarily deployed to Preview July 14, 2020 16:58 Inactive
@tomvoss
Copy link
Author

tomvoss commented Jul 14, 2020

The endpoint is now /api/auth/tokens. If a provider is not specified then the entire accounts object is returned. We may want to return something slightly different there, but I pressed the easy button for now.

@iaincollins I'm not sure what your thoughts are for renewing tokens, but my first thought was to do something like /api/auth/tokens/:provider/:type/:action where action would be something like refresh or renew. I believe a manual renew method will be necessary even if we proactively renew tokens because a provider can invalidate refresh tokens or a client may attempt to use a token after it has expired and request that we renew it before sending it to them again.

@tomvoss tomvoss changed the title added /api/auth/accounts endpoint to return oauth tokens added /api/auth/tokens endpoint to return oauth tokens Jul 14, 2020
@iaincollins
Copy link
Member

@iaincollins I'm not sure what your thoughts are for renewing tokens, but my first thought was to do something like /api/auth/tokens/:provider/:type/:action where action would be something like refresh or renew. I believe a manual renew method will be necessary even if we proactively renew tokens because a provider can invalidate refresh tokens or a client may attempt to use a token after it has expired and request that we renew it before sending it to them again.

Oooh, that is a great point.

Thoughts:

I think it's good point to provide a way to clients to forcibly renew but maybe some sort of configurable frequency (and/or be disabled by default with an option to enable) so that people can't spam it and create a Denial of Service vector by triggering too many API invocations - as services like Google and Twitter limit how many requests per hour and per day from an application and the defaults are quite low.

As far as default behaviour for the end points, auto-rotating tokens on an HTTP GET if stale (which is technically breaking the RFC expectations of not making changes on a GET request) seems fair use IMO, as returning an Access Token only needs a valid session token and doesn't need CSRF token protection. As with JWT rotation or CSRF tokens I feel like it's an edge case where sticking strictly to the RFC for HTTP isn't in the spirit of user expectation.

@iaincollins
Copy link
Member

@tomvoss Hey, just as aheads up, I've been thinking with all the other stuff going on for v3 I'm thinking we make this the main new feature of 3.1 (not to delay it, just so we can test it in isolation and get it right). I'm hoping we can wrap up v3 and get it out the door this weekend, as it's mostly dependant on change log / release notes to explain breaking changes at this point.

I don't think adding this involves any breaking changes, so should be easy to drop it in after, and actually I think in practice we can get it out sooner (as a beta, then release) if we take that approach. Does that sound okay?

Thanks again for working on this!

@tomvoss
Copy link
Author

tomvoss commented Jul 15, 2020

That sounds good to me.

@iaincollins
Copy link
Member

@tomvoss Um sorry I didn't intentionally close this issue!

@iaincollins
Copy link
Member

iaincollins commented Jul 29, 2020

@tomvoss Ah sorry, it looks like like this happened because the branch was merge to main and also the repo moved form my personal account to an org account for v3 release. For some reason GitHub won't me edit the target branch now.

Would you mind re-opening this PR against main on http://github.com/nextauthjs/next-auth? Thank you! :-)

@tomvoss
Copy link
Author

tomvoss commented Jul 30, 2020

Would you mind re-opening this PR against main on http://github.com/nextauthjs/next-auth? Thank you! :-)

Done. See PR 513.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants