accept responseType as a constructor arg #525
Conversation
lib/browser/browser.ts
Outdated
| } else { | ||
| isCodeResponse = responseType === 'code'; | ||
| } | ||
| return (!this.options.pkce && isCodeResponse); |
There was a problem hiding this comment.
I am probably wrong, my understanding is authorization code flow is a super set of pkce flow.
There was a problem hiding this comment.
you are probably right. we need a way of knowing whether we expect to redirect or not. For us this is web/native apps with "authorization_code" flow who are not using PKCE. Open to suggestions for better naming here.
There was a problem hiding this comment.
A proper name can be complex, how about
isAuthorizationCodeFlow(pkce?: boolean): boolean
There was a problem hiding this comment.
I don't follow the impact of the passed boolean.
If this is checking for an Authorization code flow without PKCE, I'd say
isAuthorizationCodeFlowWithoutPKCE() - a little Java-esque, but not confusing
If want to cover both, I'd be more explicit in the param:
isAuthorizationCodeFlow({ withPKCE?: boolean })
- Ideally
withPKCEwould be required - using the object so that it's clear, as (
isAuthorizationCodeFlow(true)is not clear) - This still makes it less than perfectly clear what happens if you pass
false, which is why I prefer the explicit name above.
There was a problem hiding this comment.
@shuowu I don't want to accept arguments, because the point of this method is for AuthJS to tell someone else whether it is operating in (non-PKCE) authorization code flow. Maybe if I had another function isPKCE then the caller can say !isPkce() and isAuthorizationCodeFlow() to get the same meaning.
There was a problem hiding this comment.
that would make sense! our pkce flow actually is pkce + ahthorization code flow,that's where the confusion is introduced, imo
There was a problem hiding this comment.
!isPkce() and isAuthorizationCodeFlow() - this looks ideal to me.
lib/browser/browser.ts
Outdated
| @@ -430,6 +431,17 @@ class OktaAuthBrowser extends OktaAuthBase implements OktaAuth, SignoutAPI { | |||
| const storage = browserStorage.getSessionStorage(); | |||
| storage.removeItem(REFERRER_PATH_STORAGE_KEY); | |||
| } | |||
|
|
|||
| isAuthorizationCodeFlow(): boolean { | |||
There was a problem hiding this comment.
update changelog and readme if it's a public method
add methods: isPKCE, hasResponseType
aarongranick-okta
force-pushed
the
ag-OKTA-340725-response-type
branch
from
0192db7 to
a649975
Compare
Codecov Report
@@ Coverage Diff @@
## master #525 +/- ##
==========================================
+ Coverage 91.28% 91.50% +0.22%
==========================================
Files 32 32
Lines 1847 1896 +49
Branches 412 424 +12
==========================================
+ Hits 1686 1735 +49
Misses 161 161
Continue to review full report at Codecov.
|
add methods: isPKCE, hasResponseType
add methods: isPKCE, hasResponseType
add methods: isPKCE, hasResponseType
base pattern for renewTokensWithRefresh renew successfully (once) Fixed renewal with refresh token Adds base revoke for refresh tokens Updates existing tests to pass updates docs a bit feat: add method handleLoginRedirect (#528) accept responseType as a constructor arg (#525) add methods: isPKCE, hasResponseType update samples to use authStateManager (#506) revised to try for non-breaking Fix for lack of AuthState Fix return val for renewTokens Fix responseType on renew error Fix urls passed for renew Run e2e tests w/o refresh token for now prep for 4.2.0 release OKTA-330344 <<<Jenkins Check-In of Tested SHA: 8f7dfb6 for eng_productivity_ci_bot_okta@okta.com>>> Artifact: okta-auth-js
https://oktainc.atlassian.net/browse/OKTA-340725