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

feat: Enroll Authenticator via /authorize #1324

Merged

Conversation

denysoblohin-okta
Copy link
Contributor

@denysoblohin-okta denysoblohin-okta commented Oct 18, 2022

  • Added new method endpoints.authorize.enrollAuthenticator(options: EnrollAuthenticatorOptions).
    Added enrollAmrValues to TokenParams.
    EnrollAuthenticatorOptions extends TokenParams and requires enrollAmrValues .
  • User will be redirected to redirectUri after authenticator enrolment, but without tokens since responseType is none.
    To allow user to still use handleRedirect in this new flow (to show errors and redirect to originalUri on success, but without storing tokens), added responseType to TokenResponse.
    In case of none value, don't clear tokens in storeTokensFromRedirect .
  • Added handleRedirect(options), deprecated handleLoginRedirect(tokens, options) in readme
  • Updated test app: added option Enroll AMR values to config and button Enroll authenticator (for both auth and non-auth states)
  • Updated readme.
  • Added unit tests, type tests. Added enroll-authenticator.feature, needs FF OAUTH2_ENROLL_AUTHENTICATOR

Internal ref: https://oktainc.atlassian.net/browse/OKTA-539548

Tech design: https://oktawiki.atlassian.net/wiki/spaces/eng/pages/2630550024/Technical+Design+for+Enroll+Authenticator+via+authorize
List of AMR values: https://oktawiki.atlassian.net/wiki/spaces/eng/pages/2230422496/Authenticator+Platform+Taxonomy#AuthenticatorPlatformTaxonomy-Table

@codecov-commenter
Copy link

codecov-commenter commented Oct 19, 2022

Codecov Report

Base: 92.11% // Head: 92.25% // Increases project coverage by +0.13% 🎉

Coverage data is based on head (5d5cc12) compared to base (fa37178).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1324      +/-   ##
==========================================
+ Coverage   92.11%   92.25%   +0.13%     
==========================================
  Files         214      218       +4     
  Lines        5152     5204      +52     
  Branches     1094     1103       +9     
==========================================
+ Hits         4746     4801      +55     
+ Misses        383      380       -3     
  Partials       23       23              
Impacted Files Coverage Δ
lib/core/mixin.ts 100.00% <100.00%> (ø)
lib/oidc/endpoints/authorize.ts 100.00% <100.00%> (ø)
lib/oidc/enrollAuthenticator.ts 100.00% <100.00%> (ø)
lib/oidc/factory/api.ts 83.33% <100.00%> (+1.28%) ⬆️
lib/oidc/getWithRedirect.ts 100.00% <100.00%> (ø)
lib/oidc/handleOAuthResponse.ts 100.00% <100.00%> (ø)
lib/oidc/mixin/index.ts 92.40% <100.00%> (+1.37%) ⬆️
lib/oidc/types/index.ts 100.00% <100.00%> (ø)
lib/oidc/util/defaultEnrollAuthenticatorParams.ts 100.00% <100.00%> (ø)
lib/oidc/util/enrollAuthenticatorMeta.ts 100.00% <100.00%> (ø)
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@aarongranick-okta aarongranick-okta left a comment

Choose a reason for hiding this comment

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

Looking pretty good overall. Just a couple things:

  • See comments on _setLocation. Let's not continue this pattern. If possible let's refactor existing function so that this pattern doesn't exist anymore

  • Is cucumber feature spec part of this task? If E2E testing is not possible at this time because code is not in production, would make sense to at least write out the scenario definition in a .feature file (and then mark it with a @Skip annotation)

@denysoblohin-okta denysoblohin-okta force-pushed the od-mfa_enroll-OKTA-539548 branch 2 times, most recently from 80fcad6 to b0f4ec0 Compare October 25, 2022 10:36
@denysoblohin-okta denysoblohin-okta changed the base branch from step-up-mfa to master November 18, 2022 14:37
@denysoblohin-okta denysoblohin-okta force-pushed the od-mfa_enroll-OKTA-539548 branch 4 times, most recently from 9b4cafe to 5af4857 Compare December 8, 2022 11:39
@@ -56,7 +57,7 @@ export function convertTokenParamsToOAuthParams(tokenParams: TokenParams) {
if (tokenParams.responseType!.indexOf('id_token') !== -1 &&
tokenParams.scopes!.indexOf('openid') === -1) {
throw new AuthSdkError('openid scope must be specified in the scopes argument when requesting an id_token');
} else {
} else if (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.

Why this change? Does empty scope query parameter have a special meaning to the authorize endpoint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

4 participants