Skip to content
This repository has been archived by the owner on Oct 24, 2024. It is now read-only.

Handle errors during token renewal. Expose TokenManager [react, angular, vue] #551

Closed
wants to merge 2 commits into from

Conversation

aarongranick-okta
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Adding Tests
  • Build related changes
  • CI related changes
  • Documentation changes
  • Other... Please describe:

What is the current behavior?

Issue Number: 245536

What is the new behavior?

Exposes the internal TokenManager instance via a convenience method for users of the Vue, Angular or React libraries. This allows (among other possibilities), adding event listeners for token expired or login_required events.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

No new behavior is being added to TokenManager. Additional tests and minor documentation additions related to this task are in PR: okta/okta-auth-js#247

Reviewers

@aarongranick-okta aarongranick-okta changed the title Expose TokenManager [react, angular, vue] Handle errors during token renewal. Expose TokenManager [react, angular, vue] Sep 11, 2019
Copy link
Contributor

@robertjd robertjd left a comment

Choose a reason for hiding this comment

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

See inline comments

packages/okta-react/src/Auth.js Outdated Show resolved Hide resolved
packages/okta-react/test/e2e/harness/src/App.js Outdated Show resolved Hide resolved
packages/okta-react/src/Auth.js Show resolved Hide resolved
packages/okta-react/README.md Outdated Show resolved Hide resolved
@aarongranick-okta aarongranick-okta force-pushed the ag-auth-instance-OKTA-245536 branch 2 times, most recently from a4b3a51 to 8db37dd Compare October 21, 2019 17:30
vijetmahabaleshwar-okta pushed a commit that referenced this pull request Oct 22, 2019
* Adding new `podname` attribute to package.json. Current package.json name is not a valid name for a pod.
* Adding missing homepage attribute to package.json
@aarongranick-okta aarongranick-okta force-pushed the ag-auth-instance-OKTA-245536 branch from f5dabb9 to 43517ea Compare October 23, 2019 18:27
@aarongranick-okta
Copy link
Contributor Author

@robertjd @swiftone Updated with tests/docs for React, Angular and Vue.

@@ -94,6 +94,10 @@ For PKCE flow, this should be left undefined or set to `['code']`.
- `autoRenew` *(optional)*:
By default, the library will attempt to renew expired tokens. When an expired token is requested by the library, a renewal request is executed to update the token. If you wish to to disable auto renewal of tokens, set autoRenew to false.

- `onTokenError` *(optional)* - callback function. If there is an error while renewing a token, the error will be passed to a handler function. The default handler calls `loginRedirect()` to initiate a login flow. Passing a function here will override the default handler.
Copy link
Contributor

@swiftone swiftone Oct 23, 2019

Choose a reason for hiding this comment

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

Is this an error in renewing (onRenewError()?) or an error with the token (and/or getting it) in general? If the latter, the documentation does not match that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is an error emitted from the TokenManager. Currently, Renew is the only case where this error will be emitted. Would onTokenManagerError be better?

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's for all errors and the renew is all that we have now, that's fine as is, I just couldn't tell if we expected future errors from the token manager to come through this callback from the text, so I wanted to be sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could instead have a callback ONLY for the "login_required" error code. (One could still getTokenManager().on('error', ... to listen for other errors should they ever exist) This would be almost identical in purpose to the existing callback onAuthRequired. In fact we could use that same callback.

@@ -194,6 +194,10 @@ For PKCE flow, this should be left undefined or set to `['code']`.
1. `auth.login` is called
2. SecureRoute is accessed without authentication

- **onTokenError** *(optional)* - callback function. If there is an error while renewing a token, the error will be passed to a handler function. The default handler calls `login()` to initiate a login flow. Passing a function here will override the default handler.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same message re: name vs docs

@aarongranick-okta
Copy link
Contributor Author

@bretterer updated based on feedback

@aarongranick-okta aarongranick-okta force-pushed the ag-auth-instance-OKTA-245536 branch from e6943c1 to 01dac25 Compare November 11, 2019 23:28
@aarongranick-okta
Copy link
Contributor Author

Closed in favor of #648

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants