search

Franpastoragusti / oidc-react-app

Oidc Client implementation with React Context and react-router-dom, to manage the auth process and manage private and public routes
43 stars 23 forks source link

Logout user instead of calling signinSilent when token expires #12

Closed sorenhoyer closed 4 years ago

sorenhoyer commented 4 years ago

If automaticSilentRenew is set to true, oidc-client will automatically try to renew the access token prior to its expiration:

"Flag to indicate if there should be an automatic attempt to renew the access token prior to its expiration. The attempt is made as a result of the accessTokenExpiring event being raised"

https://github.com/IdentityModel/oidc-client-js/wiki#other-optional-settings

Also fixes the bug from before where you could not disable silent renew, by setting automaticSilentRenew to false.

What we might want to do instead, is to call the logout function in the accessTokenExpired event in case the token is not renewed in the accessTokenExpiring event.

Franpastoragusti commented 4 years ago

Hi @sorenhoyer you are fully right, I don't know why I have not see it. I want to merge your contribution, but as you said if we change the automaticSilentRenew to true it works. I have test this approach with the addAccessTokenExpiring and addAccessTokenExpired and works too but it should be done by automaticSilentRenew. I want to have your contribution in the repository if you are able please remove the code of addAccessTokenExpired and addAccessTokenExpiring and set the automaticSilentRenew to tue and I will merge it. Thanks

sorenhoyer commented 4 years ago

Hi @Franpastoragusti I did remove the addAccessTokenExpired event handler in the last commit, so this PR just replaces this.,signinSilent() with this.logout() in addAccessTokenExpired. Without that change nothing happens when the token expires. The expected behavior when a token has expired (if silent renew is turned off or fails), would be to log a user out, right?

Franpastoragusti commented 4 years ago

@sorenhoyer, it seams to have conflicts, can you solve them first?

sorenhoyer commented 4 years ago

Hmm weird, I guess I had an older fork of your repo, without knowing. Well up to you if you want to merge this PR or just make this tiny change yourself. If you decide to merge, remember to squash the commits heh ;-)