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.

Sign up for GitHub

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

Jump to bottom

Fix error on tokens renew with refresh token #632

Closed
wants to merge 14 commits into from
Closed

Fix error on tokens renew with refresh token #632

wants to merge 14 commits into from

Conversation

oleksandrpravosudko-okta
Copy link
Contributor

/token response validation fails as code mistakenly expects it to contain state property .

Resolves OKTA-367047

@aarongranick-okta
Copy link
Contributor

The state should on the response should match what was sent on the request. Is it possible the state is not being sent?

@oleksandrpravosudko-okta
Copy link
Contributor Author

It looks like state is not present in /token response (it is also true for /token response as a step in authorization code flow).

swiftone
swiftone reviewed Feb 22, 2021
View reviewed changes
test/spec/oidc/renewTokens.ts Outdated

return Promise.resolve({
'id_token': tokens.standardIdToken,
'refresh_token': 'fakeRerfeshTalken'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice :)

swiftone
swiftone reviewed Feb 22, 2021
View reviewed changes
test/support/tokens.js Outdated
'offline_access'
],
'tokenUrl': 'https://auth-js-test.okta.com//oauth2/v1/token',
'authorizeUrl': 'https://auth-js-test.okta.com//oauth2/v1/authorize',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we testing the double // above deliberately?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks - fixed

@codecov-io
Copy link

codecov-io commented Feb 23, 2021 •
edited
Loading

Codecov Report

Merging #632 (ba439a2) into master (80a3cfe) will increase coverage by 0.45%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #632      +/-   ##
==========================================
+ Coverage   92.29%   92.74%   +0.45%     
==========================================
  Files          67       67              
  Lines        2439     2440       +1     
  Branches      515      515              
==========================================
+ Hits         2251     2263      +12     
+ Misses        188      177      -11
Impacted Files Coverage Δ
lib/oidc/renewTokensWithRefresh.ts 87.50% <ø> (+54.16%) ⬆️
lib/TokenManager.ts 97.72% <0.00%> (+0.75%) ⬆️
lib/oidc/renewTokens.ts 93.75% <0.00%> (+6.25%) ⬆️

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 80a3cfe...ba439a2. Read the comment docs.

@oleksandrpravosudko-okta oleksandrpravosudko-okta force-pushed the op-okta-367047-refresh-token-renew branch from 9a074c4 to 96be132 Compare February 23, 2021 16:07
@oleksandrpravosudko-okta oleksandrpravosudko-okta marked this pull request as ready for review February 23, 2021 16:33
oleksandrpravosudko-okta
oleksandrpravosudko-okta commented Feb 23, 2021
View reviewed changes
lib/oidc/renewTokensWithRefresh.ts Outdated
const { scopes } = getDefaultTokenParams(sdk);
const handleResponseParams = {
scopes
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SDK's scopes configuration is used for building parsed token objects, however scopes is also available in /token response - would it make sense to use it instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does probably make sense to use the value from the response rather than config. scopes can be overridden on the call used to obtain the tokens, so it could differ from the configured value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@aarongranick-okta
Copy link
Contributor

According to the API docs, state is neither a param or a response on the /token call. (It is passed to and received from /authorize)

@oleksandrpravosudko-okta oleksandrpravosudko-okta force-pushed the op-okta-367047-refresh-token-renew branch from 263d525 to 8475a3e Compare February 24, 2021 12:29
swiftone
swiftone reviewed Feb 26, 2021
View reviewed changes
lib/oidc/handleOAuthResponse.ts Outdated
if (res.scope) {
scopes = res.scope.split(' ');
} else {
var scopes = clone(tokenParams.scopes);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no var here

swiftone
swiftone approved these changes Feb 26, 2021
View reviewed changes
Copy link
Contributor

@swiftone swiftone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix the extra var, then LGTM

@dependabot dependabot bot mentioned this pull request Mar 8, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aarongranick-okta aarongranick-okta aarongranick-okta approved these changes

@swiftone swiftone swiftone approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@oleksandrpravosudko-okta @aarongranick-okta @codecov-io @swiftone @eng-prod-CI-bot-okta