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

Issue #391 - Don't allow refresh_token for auth; Add refreshExpiration #402

Merged
merged 5 commits into from
Aug 30, 2019

Conversation

longwa
Copy link
Contributor

@longwa longwa commented Aug 29, 2019

Prevent refresh_token from being used for authentication as well as adding the option to specify an expiration for the refresh_token.

Documentation has been updated and test cases added for the new functions.

@longwa
Copy link
Contributor Author

longwa commented Aug 29, 2019

Looks like the Travis build is an issue with Oracle JDK 8, not specific to my change

@alvarosanchez
Copy link
Member

I appreciate you updating the Travis build.

Still, this test fails:

rest.RestTokenValidationFilterSpec > a valid user can access the secured controller

@alvarosanchez
Copy link
Member

I've pushed this commit to try to fix the test: 0d6304f

Please merge it in your branch and push to the PR branch again

@longwa
Copy link
Contributor Author

longwa commented Aug 30, 2019

Thanks. I think I was only running the tests in the base subproject so I might’ve missed this one.

@longwa
Copy link
Contributor Author

longwa commented Aug 30, 2019

Merged and ran test-app.sh and was successful locally. We will see if travis works. Nice test build BTW, I love the docker memcache and creating all the test applications on the fly. I had just been running the tests locally via gradle, I didn't even realize it had all that.

@longwa
Copy link
Contributor Author

longwa commented Aug 30, 2019

I just thought of another change that might be needed for people that are upgrading to this version. Their existing refresh token won't have the new claim, so when they go to refresh it, it won't go through the loadFromRefreshToken path.

I think making the check still include the null expiration would be ok:

boolen isRefreshToken = jwt.JWTClaimsSet.getBooleanClaim(AbstractJwtTokenGenerator.REFRESH_ONLY_CLAIM) ||
jwt.JWTClaimsSet.expirationTime == null

Once a new token is issued, it would have the claim and be prevented from being used as an access token.

I guess the other option to this would be to include an optional parameter to loadUserByToken interface indicating whether it was being called via the refresh path or the normal authentication path, instead of looking at the token to decide.

@alvarosanchez
Copy link
Member

Good idea!

PS: I've restarted the build just in case.

@alvarosanchez alvarosanchez merged commit c6aa5b1 into grails:2.x Aug 30, 2019
@alvarosanchez
Copy link
Member

I'll do a 2.x release next Monday.

Thank you very much for your contributions!

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

Successfully merging this pull request may close these issues.

None yet

2 participants