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

Minor access token jwt fixes #4333

Merged
merged 19 commits into from Oct 14, 2019

Conversation

@charlibot
Copy link
Contributor

commented Oct 9, 2019

Found a couple more places where should return the encodedAccessToken instead of the ID. I'm not 100% sure about the UmaAuthorizationRequestEndpointController though so let me know if I should remove this part.

Also, the encoded access token is different for each encode since OAuth20JwtAccessTokenEncoder uses ZonedDateTime.now(). This means the client checking the hash may see different results. I opted to pass the encoded access token to generateAccessTokenHash to avoid this. This is now replaced with authentication.getAuthenticationDate().

charlibot added 4 commits Sep 23, 2019
….getID()

- remove unused bean oauthAccessTokenResponseGenerator
- pass encodedAccessToken to the id token generator service since running encoder twice will result in different strings if jwt (ZonedDateTime.now())
charlibot added 6 commits Oct 10, 2019
… easily get it out in OidcAccessTokenResponseGenerator. Instead make encoder use a fixed date
apereocas-bot and others added 4 commits Oct 10, 2019
@codecov

This comment has been minimized.

Copy link

commented Oct 13, 2019

Codecov Report

Merging #4333 into master will decrease coverage by 26.21%.
The diff coverage is 0%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master    #4333       +/-   ##
=============================================
- Coverage     37.79%   11.58%   -26.22%     
+ Complexity     4560     1642     -2918     
=============================================
  Files          1666     1667        +1     
  Lines         37008    37012        +4     
  Branches       3417     3416        -1     
=============================================
- Hits          13987     4287     -9700     
- Misses        21601    32280    +10679     
+ Partials       1420      445      -975
Impacted Files Coverage Δ Complexity Δ
...cas/uma/ticket/rpt/UmaIdTokenGeneratorService.java 0% <ø> (-100%) 0 <0> (-5)
...sstoken/response/OAuth20JwtAccessTokenEncoder.java 0% <0%> (-93.34%) 0 <0> (-2)
...back/OAuth20TokenAuthorizationResponseBuilder.java 0% <0%> (-77.15%) 0 <0> (-5)
...eo/cas/oidc/token/OidcIdTokenGeneratorService.java 0% <0%> (-84.27%) 0 <0> (-13)
...okenResponseTypeAuthorizationRequestValidator.java 0% <0%> (-100%) 0% <0%> (-2%)
.../discovery/OidcServerDiscoverySettingsFactory.java 0% <0%> (-100%) 0% <0%> (-3%)
...eb/support/InMemoryThrottledSubmissionCleaner.java 0% <0%> (-100%) 0% <0%> (-3%)
...g/apereo/cas/services/DynamoDbServiceRegistry.java 0% <0%> (-100%) 0% <0%> (-7%)
...rg/apereo/cas/adaptors/yubikey/YubiKeyAccount.java 0% <0%> (-100%) 0% <0%> (-1%)
...org/apereo/cas/audit/CouchDbAuditTrailManager.java 0% <0%> (-100%) 0% <0%> (-4%)
... and 634 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2328fb...2b5451d. Read the comment docs.

apereocas-bot and others added 4 commits Oct 13, 2019
@mmoayyed

This comment has been minimized.

Copy link
Member

commented Oct 14, 2019

LGTM. Tests however seem to be failing:

org.apereo.cas.oidc.token.OidcIdTokenGeneratorServiceTests > verifyAccessTokenAsJwt() FAILED
    java.lang.NullPointerException
        at org.apereo.cas.support.oauth.web.response.accesstoken.OAuth20AccessTokenAtHashGenerator.generate(OAuth20AccessTokenAtHashGenerator.java:38)
        at org.apereo.cas.oidc.token.OidcIdTokenGeneratorServiceTests.verifyAccessTokenAsJwt(OidcIdTokenGeneratorServiceTests.java:173)
@charlibot

This comment has been minimized.

Copy link
Contributor Author

commented Oct 14, 2019

@mmoayyed thanks for the review.

The test fails but now with the hashes being different. I think it's because the access token is encoded with encryption so the hash will be different every time we encode because of the nonce. Not sure how to get around this apart from passing the encodedAccessToken directly to generateAccessTokenHash which I started with but opted out of since it would involve quite a bit of refactoring.

@mmoayyed

This comment has been minimized.

Copy link
Member

commented Oct 14, 2019

Thank you. I understand. As you point out, there is quite a bit involved and we are pretty close to the final release date. I wonder if this is low-impact enough that we might be able to punt to the next release?

@mmoayyed

This comment has been minimized.

Copy link
Member

commented Oct 14, 2019

I take that back :) The date is set to be next Friday, and there should be plenty of time to review and make fixes to small measures. Thank you for taking care of this as much as you have. I'll set aside some to review more thoroughly this week.

@mmoayyed mmoayyed merged commit 24de5ed into apereo:master Oct 14, 2019
4 of 5 checks passed
4 of 5 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Summary 2 potential rules
Details
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details
@hdeadman

This comment has been minimized.

Copy link
Member

commented Oct 14, 2019

The verifyAccessTokenAsJwt() test is still failing for me (on my PR branch but also on master). Was it fixed?

@mmoayyed

This comment has been minimized.

Copy link
Member

commented Oct 15, 2019

Not yet. I am working on it and should have it done by end of day. Changes are a bit more extensive than what I had hoped.

@charlibot, other than the user info endpoints for oauth and oidc, are there any other areas you can think of where the encoded access token is passed that would need to be decoded and extracted?

@charlibot

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2019

@mmoayyed I just did a quick search for AccessToken.class with getTicket in and looks like the following would need the extractor:

  • OAuth20AccessTokenAuthenticator
  • BaseUmaTokenAuthenticator
  • OAuth20UserProfileEndpointController
  • OAuth20IntrospectionEndpointController
  • OAuth20TokenManagementEndpoint
@mmoayyed

This comment has been minimized.

Copy link
Member

commented Oct 15, 2019

Thanks. Will probably have something semi-functional towards the weekend.

@mmoayyed

This comment has been minimized.

Copy link
Member

commented Oct 18, 2019

Work is almost finalized with a whole batch of tests covering this bit. SNAPSHOTs published next week should start to support this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.