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

[WIP] Add refresh token support to Auth flow #2704

Closed
wants to merge 9 commits into from
Closed

Conversation

nyzm
Copy link

@nyzm nyzm commented Jan 28, 2019

My PR is a:

  • 💥 Breaking change
  • 🐛 Bug fix #issueNumber
  • 💅 Enhancement
  • 🚀 New feature

Main update on the:

  • Admin
  • Documentation
  • Framework
  • Plugin

Manual testing done on the following databases:

  • Not applicable
  • MongoDB
  • MySQL
  • Postgres

Description:

  • A refresh token will be included in the response of below api's
  • /auth/local
  • /auth/:provider/callback
  • /auth/reset-password
    image
  • Using a refresh token to generate a new jwt access token. Mobile clients required to save both identifier and refresh token to be able to get a new access token.
    image

  • user/me api will return tokens of the user (in case multiple mobile and desktop apps logged in)
    image

  • Revoke api for deleting the specified token from database.
    image

I think we will need a way to revoke the jwt tokens as well. which I havent think about yet. Its like log out from specified device.

Concerns:

  • I haven't tested it on other db's but just postgres. It should work but need a complete test on it.
  • Documentation and probably strapi-sdk-javascript need a little update for this

Pitch in any concerns or ideas
Regards

@derrickmehaffy
Copy link
Member

Awesome job! There will need to be a lot of testing to make sure everything works as expected but if it all goes smoothly this will be a great step forward for mobile developers.

Have you put any thought into also supporting refresh tokens for any of the OAuth providers? (Many of them already have refresh tokens, I think just the logic is required to use them), a good example being discord.

@nyzm
Copy link
Author

nyzm commented Jan 29, 2019

As I see in code that all the providers are ending with jwt token at the end.
I am not sure if we should generate a refresh token for 3rd party providers or not(Code in the commit does generate a refresh token for 3rd party providers as well). Cause user can just disconnect the app from 3rd party provider' website/app. But our mobile app will continue to get new jwt tokens with the refresh token that we generated. might need more checks on that part.

@derrickmehaffy
Copy link
Member

@nyzm it depends on the provider. For the most part once the plugin framework rewrite is done and we can break each provider into it's own plugin (and convert over to passport.js) we can likely deal with refresh tokens then. I'm not terribly familiar with oauth itself but I figure it was least worth looking into.

@derrickmehaffy derrickmehaffy mentioned this pull request Feb 2, 2019
@lauriejim lauriejim self-requested a review February 28, 2019 17:09
@lauriejim lauriejim self-assigned this Feb 28, 2019
@lauriejim lauriejim added pr: 🚀 New feature source: core:content-manager Source is core/content-manager package labels Feb 28, 2019
@lauriejim lauriejim added this to the 3.0.0-alpha.25 milestone Feb 28, 2019
@lauriejim
Copy link
Contributor

Thank you for this PR!

I think we also have to write some lines in the documentation about this new feature https://github.com/strapi/strapi/blob/master/docs/3.x.x/guides/authentication.md

@lauriejim
Copy link
Contributor

lauriejim commented Mar 2, 2019

If I login for the second time from the same agent, previous refreshToken still active.
I think it have to be deleted nop ?

@lauriejim
Copy link
Contributor

And when I register from with auth/local/register I think if the settings.email_confirmation is disable we can also add a refreshToken.

@nyzm
Copy link
Author

nyzm commented Mar 3, 2019

If I login for the second time from the same agent, previous refreshToken still active.
I think it have to be deleted nop ?

Hi Jim, There is a revoke api for refresh tokens. Clients should manage the refresh tokens. Since same OS same device will return the same agent we cannot really know which device the second login came from. Also refresh token should be revoked at logout. Which also requires client to send a revoke request.

@nyzm
Copy link
Author

nyzm commented Mar 4, 2019

Travis shows build fail for node 10.15 , it can be built successfully on my local with the same version of node and npm.

@derrickmehaffy
Copy link
Member

@nyzm I've restarted the node 10 build, it might have just been a random error but we will see. Sometimes the linting messes up for no reason. 🤷‍♂️

@derrickmehaffy
Copy link
Member

Side note @nyzm if I might make a suggestion there should be an option to revoke all tokens for a user. It might be good to add a button in the adminUI. Just a thought 😄

@nyzm
Copy link
Author

nyzm commented Mar 8, 2019

Side note @nyzm if I might make a suggestion there should be an option to revoke all tokens for a user. It might be good to add a button in the adminUI. Just a thought 😄

might be useful maybe. but sounds like nice to have :)

@derrickmehaffy
Copy link
Member

@nyzm not sure if this is just user error, I tried manually applying these changes to my current project (commit is here: canonn-science/CAPIv2-Strapi@2606219 )

Attempting to start strapi after modifications I get the following error in the console:

[2019-03-13T06:03:29.728Z] error Cannot find the controller's action refreshtoken.findOne

@derrickmehaffy
Copy link
Member

@lauriejim worked with @nyzm on slack, figured out the issue was it fails to start if graphQL is installed, removing graphQL and it worked fine. I'm guessing this has something to do with graphQL automatically generating schema stuff. Is there a way to exclude stuff from that? As this PR won't have a "findOne" option for a token

@alexandrebodin
Copy link
Member

Hi @nyzm

Thank you for your work, right now I have a few security conserns about the auth flow you are proposing.

We cannot verify who (mobile app / web app etc..) is requesting the refreshtoken. And we don't have the possibility to request an access token without a refresh token.

To add refresh tokens in a secure way we need to work on building a more robust auth server.

I recommend this read to get more info https://auth0.com/docs/tokens/refresh-token/current.
And this one for mobile auth flow https://auth0.com/docs/flows/concepts/mobile-login-flow

@lauriejim lauriejim removed this from the 3.0.0-alpha.25 milestone Mar 14, 2019
@Aurelsicoko
Copy link
Member

@nyzm Do you plan to make any updates on this PR? We cannot merge it until the authentication flow isn't following the guidelines quoted by @alexandrebodin.

@lauriejim lauriejim changed the title Add refresh token support to Auth flow [WIP] Add refresh token support to Auth flow Apr 8, 2019
@snaerth
Copy link

snaerth commented May 18, 2019

Any updates on this PR? This will be an awesome feature

@Aurelsicoko
Copy link
Member

@snaerth Not at all, we aren't focused on this feature. I cannot give you any due date for now. What's your use case?

@andr-ec
Copy link

andr-ec commented May 26, 2019

@Aurelsicoko I can’t speak for anyone else but for us the biggest use case is for a mobile client. Other options are not very practical or secure, like setting the expiration in a long time or storing the username and password

@lauriejim
Copy link
Contributor

Hello!
Thank you for your PR!
We will not merge your PR because we plan to do a complete review of the auth in Strapi.
So this will be reviewed at this time.

@lauriejim lauriejim closed this May 28, 2019
@MarcWadai
Copy link

Hi,
As for today, is there any news on this side ? Is it possible to generate a refresh token on strapi ?
Or are there any recommendations on what to do for mobile application auth in Strapi ?

In my case, the auth will be using firebase and apple signin, which will then request my backend. I was thinking on generating a refresh token and control the auth from the backend side once the first authentification is done from the mobile.

@danielehrhardt
Copy link

Hello!
Thank you for your PR!
We will not merge your PR because we plan to do a complete review of the auth in Strapi.
So this will be reviewed at this time.

5 Months then we have two Years since your comment here. What is your Roadmap about this Topic?

@derrickmehaffy
Copy link
Member

@danielehrhardt sorry plans have changed. It's most likely going to be sometime between Q2-Q4 of this year.

@devpurohit
Copy link

Will be eagerly waiting for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
source: core:content-manager Source is core/content-manager package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants