search

manfredsteyer / angular-oauth2-oidc

Support for OAuth 2 and OpenId Connect (OIDC) in Angular.
MIT License
1.88k stars 686 forks source link

Code flow and automatic refresh token #722

Closed aznarepse closed 4 years ago

aznarepse commented 4 years ago

I have been banging my head against this for many hours now and would appreciate some direction.

The setup:

Config excerpt:

responseType: 'code',
scope: 'openid profile email roles offline_access [...some more private]',

configure method:

private configure() {
    this.oauthService.configure(authConfig);
    this.oauthService.tokenValidationHandler = new JwksValidationHandler();
    this.oauthService.loadDiscoveryDocumentAndTryLogin();
    this.oauthService.setupAutomaticSilentRefresh();
  }

Problem

Using code flow and not a problem to login with a user and to actually enforce the authorization both in client and back end by roles.

However, refreshing the token fails always and the behavior is 'strange' in my opinion. The client tries refreshing with two different refresh tokens consecutively and I receive two responses (see debugging log): 

refresh tokenResponse
angular-oauth2-oidc.js:761
Object {id_token: "eyJhbGciOiJSUzI1NiIsImtpZCI6IlV4NE5tOXhBQ0JyNVE0N2…", access_token: "eyJhbGciOiJSUzI1NiIsImtpZCI6IlV4NE5tOXhBQ0JyNVE0N2…", expires_in: 3600, token_type: "Bearer", refresh_token: "SOcPcu1GPcuM08Y1B1udzo3NJKYV4KLBhoqdUmfDWvo", …}
angular-oauth2-oidc.js:761
refresh tokenResponse
angular-oauth2-oidc.js:761
Object {id_token: "eyJhbGciOiJSUzI1NiIsImtpZCI6IlV4NE5tOXhBQ0JyNVE0N2…", access_token: "eyJhbGciOiJSUzI1NiIsImtpZCI6IlV4NE5tOXhBQ0JyNVE0N2…", expires_in: 3600, token_type: "Bearer", refresh_token: "iucZ4iJPKKXMx23zNUqbxEhontiPIXVTJHDf7aOW-5g", …}

The idtoken and the accesstoken are similar but the refresh is not. These request are sent one after the other (straight).

Then, I get two failures in a row:

Failed to load resource: the server responded with a status of 400 (Bad Request) [....omitted...]
Error performing password flow
angular-oauth2-oidc.js:1358
HttpErrorResponse {headers: HttpHeaders, status: 400, statusText: "Bad Request", url: "...omitted...", ok: false, …}
angular-oauth2-oidc.js:1358
Automatic silent refresh did not work
angular-oauth2-oidc.js:761
Failed to load resource: the server responded with a status of 400 (Bad Request) [...omitted...]
Error performing password flow
angular-oauth2-oidc.js:1358
HttpErrorResponse {headers: HttpHeaders, status: 400, statusText: "Bad Request", url: "....omitted...", ok: false, …}
angular-oauth2-oidc.js:1358
Automatic silent refresh did not work

Note the error 'Error performing password flow', which I am not sure is just a funny in the loging or it is actually trying a password flow... because IdentityServer4 is not complaining about the password flow but about the token not existing...

[2020-02-15T13:28:43.2741552+00:00][DBUG][31][IdentityServer4.EntityFramework.Stores.PersistedGrantStore] "zeIPcnCv0o9OAQ8bBK8J0YHKnX5I9JG8lDpRAPwSPyw=" found in database: False
[2020-02-15T13:28:43.2742509+00:00][DBUG][31][IdentityServer4.Stores.DefaultRefreshTokenStore] "refresh_token" grant with value: "iucZ4iJPKKXMx23zNUqbxEhontiPIXVTJHDf7aOW-5g" not found in store.
[2020-02-15T13:28:43.2743632+00:00][WARN][31][IdentityServer4.Validation.TokenValidator] Invalid refresh token
[2020-02-15T13:28:43.2747049+00:00][WARN][31][IdentityServer4.Validation.TokenRequestValidator] Refresh token validation failed.

Which I believe it is in the db.

Finally, at some point, the client will not be able to access the back end with the error ' Token expired'

However, the client is still working fine with the user logged in. If I logout and login again, or simply send a login again (without actually logout), I will have access to the back end again since my new token is fresh.

Please, any advice or lead for me to be able to progress?

jeroenheijmans commented 4 years ago

As a thought, maybe #682 is related?

thebaron24 commented 4 years ago

@aznarepse

try a different order for your setup sequence. I found a suggestion in another issue that worked for me. I use code flow and identityserver also for a demo and I can see the refresh happening in the console. However this version is still and Angular8 version though.

this.oauthService.configure(authConfig);
this.oauthService.setupAutomaticSilentRefresh();
this.oauthService.tokenValidationHandler = new JwksValidationHandler();
this.oauthService.loadDiscoveryDocumentAndTryLogin()

you can see the refresh token happening in the console for my demo once you login after a moment

aznarepse commented 4 years ago

@jeroenheijmans Possibly. However, see below as thebaron24 suggestion seems to work around it. I have it now running in a testing environment (compiles as production) and the messages are more explicit:

Odly, thebaron24's suggestion seems to work... spooky! @thebaron24 Indeed! Thank you sir! Good for now.

jeroenheijmans commented 4 years ago

Nice, good to hear you solved things, thanks for sharing!

aznarepse commented 4 years ago

@jeroenheijmans Well, it is working. However, I still think it is a bit odd behavior and the work around a bit voodoo. I am rather new to Angular but I'll be happy to do some debugging if you lead me in the right direction around the library code to focus on this subject.

jeroenheijmans commented 4 years ago

Maybe @thebaron24 has some suggestions on that? We could re-open or open a fresh issue if the docs need brushing up, or if we know we should explicitly throw an error under some configuration circumstances....

thebaron24 commented 4 years ago

@aznarepse

I think conceptually it makes sense to set up the refresh flow before you get the discovery document and init the login flow. It looks like the majority of people were using this package in the implicit flow and as things are moving towards the code from with PKCE we are seeing some gaps in the documentation. I had a difficult time setting up a nodejs based server side rendering with universal also because the documentation pointed to an angular 5 version using .net as the server side tech.

However, the source code viewer they have set up was what saved me:

for example the auth service source and people who hover int he issues section are pretty helpful :)

I think the only suggestion I have would be what you already said - update the documentation to reflect this order because anyone who used this would probably want to setup short term token refresh like this.

jeroenheijmans commented 4 years ago

Ah, that comments helps me see the actual issue 😅. In that case, in my flow I also set up the listener way before I run the initial login sequence with the disco document.

thebaron24 commented 4 years ago

your example was actually a huge help while setting up my project.

aznarepse commented 4 years ago

With all due respect (to all the amazing hard work put in this project and the people who did it), I think a public API should only expose function members that are commutative. If a public method needs some previous state set by other method, then in my opinion the latter should call the previous. Again, in my humble opinion, if the previous methods are just setting a state defined by properties, then this 'properties' should be properties and not a function method. Let's assume a chain of methods to obtain my taxes return. In such case, the chaining of several functions is not commutative since they would affect the result. Assume: F(gross)-F(expense) = net then F(net*0.2) = tax. Clearly, the chain is not commutative since the order matters (F(gross)-F(expense) !=F(expense)-F(gross)). Hence, in my ideal API, I would have setters for the properties gross and expense and then a single call to F(gross,expense,0.2) which would become atomic.

Nonetheless, I am still having some odd failures when renewing the token and the logging is definitely not correct "Error performing password flow" or in any case misleading. I am working on this implementation heavily and I will very gladly have a look at the code indicated by thebaron24 and will come back with some 'visibility'. (end of week or weekend).

thank you all.

jeroenheijmans commented 4 years ago

@aznarepse Is it correct to assume that you suggest that calling setupAutomaticSilentRefresh() after the other methods (e.g. loadDisco....AndTryLogin(...)) should

does that paraphrasing sound correct?

If so, I think I'd agree, and I'd be tempted to open up a fresh issue for that (linked to this one) so we can focus on those improvements?

aznarepse commented 4 years ago

@jeroenheijmans Sorry, have been out of the computer... Yes, that's what I meant. Definitely written in much clearer way. I will be very happy to dig in the code and put the time myself and pass it back. The least I can do after using this fantastic lib.