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

Is this login / refresh token flow correctly implemented? #142

Closed
rfgamaral opened this issue Jun 7, 2018 · 11 comments
Closed

Is this login / refresh token flow correctly implemented? #142

rfgamaral opened this issue Jun 7, 2018 · 11 comments

Comments

@rfgamaral
Copy link

rfgamaral commented Jun 7, 2018

onWindowLoadEvent(function () {
    const currentUser = netlifyIdentity.currentUser();

    if (!currentUser) {
        setTimeout(function() {
            netlifyIdentity.on('login', function(user) {
                netlifyIdentity.close();

                loadSiteContent(user.token.access_token);
            });

            netlifyIdentity.open('login');
        }, 0);
    } else {
        currentUser.jwt().then(function(accessToken) {
            loadSiteContent(accessToken);
        });
    }
});

On the window load event, I read the current user. If one is available, invoke the jwt() to refresh the token if necessary, either way, the current or new token will be returned and I use that to invoke loadSiteContent(). If the user is not available (it never logged into the site before), open the login dialog and if the user performs a successful login, invoke loadSiteContent() with the user token from the successful login.

The loadSiteContent() function will make an HTTP request to https://example.com/.netlify/git/github/contents?ref=secret and retrieve ALL site content from the secret branch (on a private repository) to display on the site. This HTTP request needs a valid access token, otherwise it won't work.

Given my code above and the way I implemented the login / refresh token flow, will I allow any user to successfully login and keep it logged in for all time (due to token refresh) until the user explicitly logs out?

For instance, say a user logins today (by default the token expires after 1 hour) and keeps visiting the site for the next few hours... After 1 hour, the token will be expired, but with my flow above, a new one will be generated and the user will remain logged in. But say the user didn't visit the site for 24 hours, or a week, or a month/year... Will the token refresh still work after this time? Or is there a true expiration date that requires me to invoke the login dialog again? Which my current flow does not support. What is that true expiration date? Also, is it possible to change the default token expiration time for 24 hours instead of just 1?

@rfgamaral
Copy link
Author

@bcomnes I apologize for tagging you like this but could you please give me some input on my concerns aobve?

@bcomnes
Copy link
Contributor

bcomnes commented Jun 8, 2018

@rfgamaral I'll try to get to this today this evening. Working on finishing up a release today.

@fool
Copy link

fool commented Jun 8, 2018

We have already decided not to extend the token expiration time after a lot of discussion, so that is a question that is already decided. I don't know the answers to whether your flow is correct or not, but I think @bcomnes or @futuregerald may be able to advise, though they're pretty busy so not making any promises of their time.

@futuregerald
Copy link
Contributor

@rfgamaral The refresh_token doesn't expire unless the user logs out, you force them to logout, or the user logs in again (which will generate a new refresh_token). There are also some rare cases, for instance if a user logs in and in another browser window they attempt to use an old refresh_token, then it will invalidate the refresh_token and you'll need another login. That's not something you should see happen often, if at all, just sharing that there are some cases where they can't use the refresh token. Beyond that, the refresh_token will remain indefinitely. This means that in your

.then(function(accessToken) { loadSiteContent(accessToken); });

You should account for not getting back a valid JWT, or you'll have a possible unhandled error state.

@rfgamaral
Copy link
Author

rfgamaral commented Jun 9, 2018

@futuregerald I'm a bit confused because you keep mention refresh_token, am I right to assume that by refresh_token you are referring to do this one:

image

Because I'm only using the access_token above that one. Does the expires_in refers to access_token or refresh_token?

You should account for not getting back a valid JWT, or you'll have a possible unhandled error state.

I'm not exactly sure how to do, can you provide some pointers?

@rfgamaral
Copy link
Author

Ok, after a while I noticed that my flow is not actually working as I expected it... Let me start from scratch so I can try to understand how to best implement this.

$window.on('load', function() {
    const currentUser = netlifyIdentity.currentUser();

    if (!currentUser) {
        setTimeout(function() {
            netlifyIdentity.on('login', function(user) {
                netlifyIdentity.close();

                getPrivateContent(user.token.access_token);
            });

            netlifyIdentity.open('login');
        }, 0);
    } else {
        getPrivateContent(currentUser.token.access_token);
    }
});

The code above will run on the page load event and will work like this:

  • Get the current user data from localStorage (if there's any).
  • Check if the current user data is defined or not.
  • If the current user data is NOT defined, open the login dialog and wait for the user to login successfully.
  • When the user logs in successfully, close the login dialog and invoke getPrivateContent() with the just created access_token from the user object.
  • If the current user data is defined, invoke getPrivateContent() with the access_token from that user data.

The getPrivateContent() will make an HTTP request to https://example.com/.netlify/git/github/contents?ref=secret and retrieve site content from the secret branch (on a private repository) to display on the site. That HTTP request needs the access_token to make a valid request, otherwise it won't work, that's why we always pass the access_token to the getPrivateContent() function.

The flow above will work just fine when the user logins for the first time and will continue to work until the token used for the /.netlify/git/github/contents?ref=secret request is no longer valid. When that happens, the request will fail.

Now, say that the user closed the site and came back a few hours later and the token has expired. The HTTP request to get the private content will fail and this is where I need to implement a mechanism to refresh that token and allow the user to keep browsing the website private content without asking the user to login again. The user should only be asked to login again if the session data stored on the browser was cleaned (either by logging out explicitly or something else, doesn't matter).

What's the best/recommended way for me to implement this?

@imorente
Copy link
Contributor

imorente commented Jun 9, 2018

@rfgamaral, instead of getting the access_token property directly from the current user object, have you tried getting it through the jwt function instead?

That should renew the access token for you if it has expired (see GoTrueJS)

const currentUser = netlifyIdentity.currentUser();
if (currentUser) {
   currentUser.jwt().then(accessToken => {
      // fetch the content that requires the access token
   });
} else {
    // show login
}

derp – I missed that you actually did use the jwt function in your opening example. Did that not work for you? That should indeed be the recommended method

@rfgamaral
Copy link
Author

@imorente

There was at least one edge case were my code was not working properly but I couldn't figure out the conditions for it to happen and it didn't happen very often. I ended up implementing something a bit different, similar to my initial solution but with 2 changes:

  • Added a setTimeout inside the currentUser.jwt().then() handler before fetching content.
  • Added a handler for when the private content fetch request fails to call jwt(true) (to force a new token) and then reloaded the page instead of requesting the content again.

I probably don't need the setTimeout on the jwt().then() handler. And I'm not sure if I need to force a token refresh on the failed handler, but it's hard to debug this and I just forced these things to be sure content is always loaded no matter what.

@rfgamaral
Copy link
Author

Just simplified my code a little bit to make more sense and added a bunch of logging to better understand what's happening in case of an error. Let's see how this goes and I'll report back when possible.

@rfgamaral
Copy link
Author

I think I have a bigger issue not quite related to login, that is actually working and the token refresh properly when needed. That's what looks like at least. I'm closing this and opening a new issue to better explain the issue I'm facing. Thank you all for trying to help, see you on the other issue 😆

@rfgamaral
Copy link
Author

@bcomnes @futuregerald @imorente If any of you could take a look at #144, that would be great. Thanks.

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

No branches or pull requests

5 participants