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

validateUser semantics #43

Closed
CarmineM74 opened this issue Sep 29, 2014 · 17 comments
Closed

validateUser semantics #43

CarmineM74 opened this issue Sep 29, 2014 · 17 comments

Comments

@CarmineM74
Copy link

Hi,

First off thank you for the great work!

I'm having issues understaning how validateUser() is supposed to work. I mean the documentation states that "This method returns a promise that will resolve if a user's auth token exists and is valid".

So I expected that, after a successful login, when the user's session is expired a successive call to validateUser() should find an expired token and emit events accordingly.

This doesn't seem to be the case.

Indeed, when the current session is expired the call to validateUser() will find both existing auth_headers and a user with the flag "signedIn" set to true! There's no roundtrip to the backend to check wether a token is expired or not.

Is that the expected behaviour of validateUser()?

Thanks in advance

@lynndylanhurley
Copy link
Owner

How are you expiring the session?

@CarmineM74
Copy link
Author

The session expires by inactivity. I've set the timeout on the backend to 1 minute. After the time out, I click on the logout button on my UI, which at some point calls validateUser() and the promise gets resolved.

@lynndylanhurley
Copy link
Owner

I'm sorry if the documentation isn't clear. The validateToken method will only make a request if there is no authentication token present in the client's session. The idea is that access permissions should be established by the API anyway, so a preceding call to validateToken would be redundant. The method exists to prevent requests that the client knows will fail without authentication.

I'll look into the expired session issue. The client should be able to reject the validateUser promise due to expired tokens without the need for an API request. If this isn't happening, I'll push a fix ASAP.

@CarmineM74
Copy link
Author

Thanks for the clarification, and the kind replies!

@lynndylanhurley
Copy link
Owner

@CarmineM74 - so I actually do have a test case for this, and it is passing. The test is here.

I'm assuming that you're using the token_lifespan setting in devise_token_auth to change the token expiry duration. Was this setting changed on the server before or after the token headers were sent to the client?

@CarmineM74
Copy link
Author

The token_lifespan is set on the backend before starting everything. The sequence is as follows:

  1. Start rails backend
  2. Start frontend (via grunt serve)
    2.1 I log into the system and by inspecting cookies I can see that a valid auth_headers is stored via $cookieStore.
    2.2 Then I sit idle until the session expires (for simplicity I've set that limit to 60 seconds).
    2.3 After the 60 seconds have passed, I click on a button on the page which does:
    2.3.1 AuthorizationService.currentUser().then((user) => ...)
    What AuthorizationService.currentUser() does is:
    $auth.validateUser().then((validatedUser) => ... console.log('OK')).catch((e) => ... console.log('NOT OK'))

Now, after the session has expired I expect the call to $auth.validateUser() to trigger the catch instead of the then branch. Unfortunately this doesn't happen, and the then branch gets triggered.

For the sake of completeness, these are the versions I'm using right now:

  • nt-token-auth v0.0.22
  • angularjs v1.2.16
  • devise_token_auth gem v0.1.28

Should you need to peek at the actual code, please feel free to ask.

@lynndylanhurley
Copy link
Owner

Thanks for the step-by-step! I'll try to reproduce.

@lynndylanhurley
Copy link
Owner

This was indeed a bug. I'll push the fix tonight.

@lynndylanhurley
Copy link
Owner

@CarmineM74 - I think this has been resolved. Please update to ng-token-auth version 0.0.23.beta1 and devise_token_auth version 0.1.29.beta6 to verify.

lynndylanhurley added a commit that referenced this issue Oct 2, 2014
@lynndylanhurley
Copy link
Owner

Ugh that's a typo - should be ng-token-auth version 0.0.23-beta1

@CarmineM74
Copy link
Author

I believe I will be able to test everything today in the afternoon. Will report as soon! Thanks again for your promptness and great work!

@lynndylanhurley
Copy link
Owner

No problem! Thanks for your excellent error reporting 👍

@CarmineM74
Copy link
Author

I have done the checks and it appears the issue is still present.
That's an excerpt from your validateUser():

         `... 
          # save trip to API if possible. assume that user is still signed
          # in if auth headers are present and token has not expired.
          if @userIsAuthenticated()
              # user is still presumably logged in
              @resolveDfd()
            ...`

Judging from the comments I'd expect that the call to @userIsAuthenticated() is in charge
of verifying the freshness of the token.

Assuming so, I went to take a look at the code for userIsAuthenticated() and from what I see
there's no check in place for token's freshness. Is that correct?

I believe that to fix the issue, the code should be changed to:

if @userIsAuthenticated() and !@tokenHasExpired() ...

What do you think?

Edited:

Of course then it should be rethought where to broadcast the auth:ession-expired event too!

@lynndylanhurley
Copy link
Owner

I think you might be right. I'll check as soon as I can. Thanks!

lynndylanhurley added a commit that referenced this issue Oct 6, 2014
@lynndylanhurley
Copy link
Owner

@CarmineM74 - I just added the tokenHasExpired method as one of the conditions of the userIsAuthenticated method. Please check out version 0.0.23-beta2 and let me know if it solves the issue.

Also, I'm not sure why the tests for this weren't failing like they should have. Can you find anything obviously wrong with these tests?

@lynndylanhurley
Copy link
Owner

Ok, the test should be better now.

The validateUser method is really messy. Once we get everything working, and we're sure that the tests are good, I'll try to clean up the code a bit.

@CarmineM74
Copy link
Author

Hi,
Sorry for the delay. With 0.0.23-beta2 the things are working as expected. After timeout the validateUser() correctly invokes the promise's catch callback. Thanks!

lynndylanhurley added a commit that referenced this issue Oct 9, 2014
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

2 participants