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.

Sign up for GitHub

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

TokenSet draft #1317

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

TokenSet draft #1317

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions lib/core/TokenSet.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import { OktaAuthCoreInterface } from './types';
import { TokenManager } from '../oidc';
import { TOKEN_STORAGE_NAME } from '../constants';
import { ServiceManager } from './ServiceManager';

export class TokenSet {
private sdk: OktaAuthCoreInterface;
private map: Map<string, any>; // TODO: add type

constructor(sdk: OktaAuthCoreInterface) {
this.sdk = sdk;

this.map = new Map();
Copy link
Contributor

Choose a reason for hiding this comment

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

I would move the map to a higher level (see below)

const options = sdk.options;
const tokenManager = new TokenManager(this.sdk, options.tokenManager);
Copy link
Contributor

Choose a reason for hiding this comment

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

As I look through the code some more, maybe we want to have the TokenSet contain an instance of the oidc interface rather than purely a TokenManager instance

https://github.com/okta/okta-auth-js/blob/master/lib/oidc/mixin/index.ts

this contains the methods like isAuthenticated and getIdToken etc we will want to expose on the TokenSet class

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 think it comes back to the question, should we manage one or multiple authState, ideally isAuthenticated and getIdToken should return the same result as the in memory authState, and those wrapper method does not add too much value.
For this change, I think we can focus on the following goals, and add extra public methods at later stage if needed:

  1. add public interface/s to support multiple token sets
  2. keep existing methods functional by default

const serviceManager = new ServiceManager(this.sdk, options.services);
this.map.set(TOKEN_STORAGE_NAME, {
tokenManager,
serviceManager,
});
}

start(key?: string) {
// start services for one or all token sets
}

stop(key?: string) {
// stop services for one or all token sets
}

// call this method when need to add a new token set, like after receiving new tokens from authorize/idx
addTokenSet(tokens, options) {
const storageKey = options.storageKey;
const tokenManager = new TokenManager(this.sdk, options.tokenManager); // with specific storageKey
const serviceManager = new ServiceManager(this.sdk, options.services);
this.map.set(storageKey, {
tokenManager,
serviceManager,
});

tokenManager.setTokens(tokens);

if (options.default) {
this.useTokenSet(options.tokenManager.storageKey);
}
}

useTokenSet(key) {
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 this method should be moved up a level and allow for "querying" of multiple criteria, like scopes, acr_values

authClient.with({name: 'token_set_key'})
// or
authClient.with({scopes: ['offline_access']})
// or
authClient.with({acr: 'acr_value'})

.with will return a TokenSet that matches the criteria that was "queried" for

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 will need to make sure each query end up with only one token set, so maybe just use one type of identifier. For internal api (myaccount), we can find the best practice to pick the key, otherwise it leaves to devs to figure out the key.

const tokenSet = this.map.get(key);
this.sdk.tokenManager = tokenSet.tokenManager;
this.sdk.serviceManager = tokenSet.serviceManager;
// sync authState with new token set
this.sdk.authStateManager.updateAuthState();
}
}
6 changes: 4 additions & 2 deletions lib/core/mixin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
import { AuthStateManager } from './AuthStateManager';
import { ServiceManager } from './ServiceManager';
import { OktaAuthCoreInterface, OktaAuthCoreOptions } from './types';
import { TokenSet } from './TokenSet';

export function mixinCore
<
Expand All @@ -27,15 +28,16 @@ export function mixinCore
{
authStateManager: AuthStateManager<M, S, O>;
serviceManager: ServiceManager<M, S, O>;
tokenSet: TokenSet;

constructor(...args: any[]) {
super(...args);

// AuthStateManager
this.authStateManager = new AuthStateManager<M, S, O>(this);

// ServiceManager
this.serviceManager = new ServiceManager<M, S, O>(this, this.options.services);
this.tokenSet = new TokenSet(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

Something like:

this.map = new Map();
const defaultTokenSet = new TokenSet(this);
this.map.set(DEFAULT_TOKEN_SET_KEY, defaultTokenSet);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so authjs itself becomes the manager of different token sets, and TokenSet becomes a logic group for TokenManager and ServiceManager?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

personally, I think we should avoid put too much management logic in the top level instance, maybe rename TokenSet as TokenSetManager?

this.tokenSet.useTokenSet(this.options.tokenManager.storageKey);
}

async start() {
Expand Down
2 changes: 1 addition & 1 deletion lib/oidc/TokenManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ export class TokenManager implements TokenManagerInterface {
// eslint-disable-next-line complexity
constructor(sdk: OktaAuthOAuthInterface, options: TokenManagerOptions = {}) {
this.sdk = sdk;
this.emitter = (sdk as any).emitter;
this.emitter = new EventEmitter();
if (!this.emitter) {
throw new AuthSdkError('Emitter should be initialized before TokenManager');
}
Expand Down