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: storageManager and transactionManager #604

Closed
wants to merge 3 commits into from

Conversation

aarongranick-okta
Copy link
Contributor

@aarongranick-okta aarongranick-okta commented Jan 23, 2021

adds 2 new objects on the sdk instance:

  • storageManager provides access to storage providers using sdk config.
  • transactionManager provides access to a saved transaction

some things have been moved around internally, but compatibility should be preserved.

httpCache and other high level providers (using "storageBuilder") have been moved out of "storageUtil" to StorageManager. storageBuilder has been renamed to SavedObject and is now a class.

shims are provided on "options.storageUtil" to preserve compatibility if anyone was using that interface (was never documented).

"legacy" PKCE meta and OAuth params compatibility logic has been added. Going forward, PKCE meta and OAuth params are stored together along with the other transaction data.

@codecov-io
Copy link

codecov-io commented Jan 23, 2021

Codecov Report

Merging #604 (75ef64c) into master (e50d96b) will decrease coverage by 1.16%.
The diff coverage is 87.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #604      +/-   ##
==========================================
- Coverage   93.07%   91.91%   -1.17%     
==========================================
  Files          38       41       +3     
  Lines        2166     2349     +183     
  Branches      455      513      +58     
==========================================
+ Hits         2016     2159     +143     
- Misses        150      190      +40     
Impacted Files Coverage Δ
lib/server/serverStorage.ts 65.00% <43.47%> (-35.00%) ⬇️
lib/types/Transaction.ts 76.00% <76.00%> (ø)
lib/OktaAuthBase.ts 90.00% <83.33%> (+2.00%) ⬆️
lib/SavedObject.ts 84.61% <84.61%> (ø)
lib/browser/browserStorage.ts 93.98% <90.47%> (-6.02%) ⬇️
lib/TransactionManager.ts 91.17% <91.17%> (ø)
lib/StorageManager.ts 100.00% <100.00%> (ø)
lib/TokenManager.ts 96.96% <100.00%> (+0.28%) ⬆️
lib/browser/browser.ts 90.66% <100.00%> (+0.08%) ⬆️
lib/constants.ts 100.00% <100.00%> (ø)
... and 11 more

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 e50d96b...75ef64c. Read the comment docs.

}
```

##### `storageType`
Copy link
Contributor

Choose a reason for hiding this comment

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

From the code samples above, looks like the storageType accepts an array of types, it would be great to also explain the meaning of multiple versus single storageType input.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a 2nd option called storageTypes, I will add some docs for it

@@ -321,19 +416,31 @@ var config = {
var authClient = new OktaAuth(config);
```

Even if you have specified `localStorage` or `sessionStorage` in your config, the `TokenManager` may fall back to using `cookie` storage on some clients. If your site will always be served over a HTTPS connection, you may want to enable "secure" cookies. This option will prevent cookies from being stored on an HTTP connection.
A custom [storage provider](#storageprovider) instance can also be passed here. (This will override any `storageProvider` value set under the `token` section of the [storageManager](#storagemanager) configuration)
Copy link
Contributor

Choose a reason for hiding this comment

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

old approach override the new one?

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 currently operating in compatibility mode. A default set storageTypes is set for each well known section. If the config specifies a storageType it will be used as the first choice. Then, it will go to the NEXT entry in storageTypes. This matches current behavior. Fallback logic can be disabled (see README additions).

* `localStorage`: available to all browser tabs
* `cookie`: available to all browser tabs, and server-side code

##### `storageProvider`
Copy link
Contributor

Choose a reason for hiding this comment

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

Per #606, looks like users are confused about how the custom storage (storageProvider) is used internally by auth-js. Probably, we can add some explanation for that.

var config = {
storageManager: {
token: {
storageTypes: [
Copy link
Contributor

Choose a reason for hiding this comment

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

so here means the supported storage types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, these are the preferred storage types in fallback order

the "storageUtil" (browser or server) decides what types are supported

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps a name to indicate that? storageOrder?

@shuowu
Copy link
Contributor

shuowu commented Jan 27, 2021

Generally looks good to me! One concern is that looks like some options provide the same functionality, but can override some existing options. We probably can provide warning messages if option conflict is detected.

@aarongranick-okta
Copy link
Contributor Author

Screen Shot 2021-01-29 at 9 49 05 AM

**Important:** A storage provider will receive sensitive data, such as the user's raw tokens, as a string. Any custom storage provider should take care to save this string in a secure location which is not accessible by other users.
A `storageProvider` provides low-level access to storage. An example of a `storageProvider` is [localStorage][]. It has a method called `getItem` that returns a string value for a key and a method called `setItem` which accepts a string value and key.

**Important:** A storage provider will receive sensitive data, such as the user's raw tokens, as a readable string. Any custom storage provider should take care to save this string in a secure location which is not accessible to unauthorized users.
Copy link
Contributor

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.

None yet

5 participants