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: add method handleLoginRedirect #528

Merged
merged 2 commits into from
Oct 29, 2020

Conversation

shuowu
Copy link
Contributor

@shuowu shuowu commented Oct 28, 2020

No description provided.

CHANGELOG.md Outdated
@@ -21,13 +21,15 @@
- `sdk.setFromUri`
- `sdk.getFromUri`
- `sdk.removeFromUri`
- `sdk.handlePostLoginRedirect`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can name this handleLoginRedirect which matches better with isLoginRedirect. Also is isLoginRedirect being hoisted out of the token namespace? Either isLoginRedirect should be hoisted up, or handleLoginRedirect should be put inside the token namespace, because these 2 methods belong together.

README.md Outdated
@@ -491,6 +491,25 @@ oktaAuth.authStateManager.subscribe(authState => {
oktaAuth.authStateManager.updateAuthState();
```

##### `onPostLoginRedirect`
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is related to the fromUri concept. It would be nice if the naming matched for these methods. Currently we have:

  • setFromUri
  • getFromUri
  • removeFromUri

Rather than being an event hook (onX), this method would make more sense as something that DOES something, like "restoreFromUri", but I think I don't really like the "from". From WHAT? Maybe we can find a better name for the core concept then have get/set/remove/restore around that concept.

The URI is where you were navigating to before you were interrupted for an auth flow. When the auth flow completes you would like to continue on to that original/intended URI. Maybe originalUri or intendedUri ? or pendingUri ?

restorePendingUri or redirectToPendingUri sounds OK. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel originalUri makes sense in the context. Changed related methods/docs accordingly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, "restore original" makes sense to me

@shuowu-okta shuowu-okta changed the base branch from sw-add-methods-for-4.1-OKTA-340176 to master October 29, 2020 14:49
@shuowu-okta shuowu-okta changed the base branch from master to sw-add-methods-for-4.1-OKTA-340176 October 29, 2020 14:50
@shuowu-okta shuowu-okta changed the base branch from sw-add-methods-for-4.1-OKTA-340176 to master October 29, 2020 14:51
@shuowu-okta shuowu-okta force-pushed the sw-add-handlePostLoginRedirect branch from 08e0390 to 59c7958 Compare October 29, 2020 14:54
@shuowu-okta shuowu-okta changed the title feat: add method handlePostLoginRedirect feat: add method handleLoginRedirect Oct 29, 2020
@codecov-io
Copy link

Codecov Report

Merging #528 into master will increase coverage by 0.17%.
The diff coverage is 94.91%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #528      +/-   ##
==========================================
+ Coverage   91.28%   91.45%   +0.17%     
==========================================
  Files          32       32              
  Lines        1847     1885      +38     
  Branches      412      422      +10     
==========================================
+ Hits         1686     1724      +38     
  Misses        161      161              
Impacted Files Coverage Δ
lib/browser/browser.ts 94.53% <94.54%> (-0.77%) ⬇️
lib/util.ts 74.23% <100.00%> (+0.64%) ⬆️
lib/token.ts 96.08% <0.00%> (+0.27%) ⬆️
lib/AuthStateManager.ts 91.39% <0.00%> (+2.15%) ⬆️

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 0d625b1...59c7958. Read the comment docs.

@shuowu-okta shuowu-okta merged commit b441ee0 into master Oct 29, 2020
@aarongranick-okta aarongranick-okta deleted the sw-add-handlePostLoginRedirect branch October 30, 2020 16:26
eng-prod-CI-bot-okta pushed a commit that referenced this pull request Nov 18, 2020
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
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