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

Added ServiceManager to elect leader for token auto renew service #1117

Closed
wants to merge 7 commits into from

Conversation

denysoblohin-okta
Copy link
Contributor

@denysoblohin-okta denysoblohin-okta commented Feb 17, 2022

Added broadcast-channel package to solve cross-tab token renewal issues with tab leader election method.
Added ServiceManager that is responsible for:

Notes:

  • syncStorage should be set to true to start auto-renew only on 1 leader tab. Otherwise auto-renew service will run on every tab.
  • tokenManager.start() does not start token service anymore. serviceManager.start() is responsible for starting services and is called in oktaAuth.start()

Internal ref: OKTA-456234 (and OKTA-456251)

Resolves #1001

Cross-tab simulation
before:

cross1.mov

after:

cross2.mov

@codecov-commenter
Copy link

codecov-commenter commented Feb 18, 2022

Codecov Report

Merging #1117 (54f5bcf) into master (afe7e17) will decrease coverage by 0.07%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1117      +/-   ##
==========================================
- Coverage   93.51%   93.43%   -0.08%     
==========================================
  Files         154      158       +4     
  Lines        4147     4251     +104     
  Branches      906      922      +16     
==========================================
+ Hits         3878     3972      +94     
- Misses        253      263      +10     
  Partials       16       16              
Impacted Files Coverage Δ
lib/services/TokenService.ts 80.00% <75.00%> (-17.78%) ⬇️
lib/ServiceManager.ts 91.17% <91.17%> (ø)
lib/services/SyncStorageService.ts 95.45% <95.45%> (ø)
lib/OktaAuth.ts 89.80% <100.00%> (+0.13%) ⬆️
lib/TokenManager.ts 94.84% <100.00%> (-0.11%) ⬇️
lib/services/AutoRenewService.ts 100.00% <100.00%> (ø)
lib/services/index.ts 100.00% <100.00%> (ø)
lib/types/index.ts 100.00% <100.00%> (ø)

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 afe7e17...54f5bcf. Read the comment docs.

@denysoblohin-okta denysoblohin-okta marked this pull request as ready for review February 18, 2022 02:01
@denysoblohin-okta denysoblohin-okta force-pushed the od-broadcast-channel-OKTA-456234 branch from 0cb18c7 to 4dc6e37 Compare February 18, 2022 18:33
denys.oblohin added 2 commits February 18, 2022 20:33

private static knownServices = ['autoRenew', 'syncStorage'];

constructor(tokenManager: TokenManager, options: TokenManagerOptions = {}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ServiceManager could accept an instance of OktaAuth (like the AuthStateManager does https://github.com/okta/okta-auth-js/blob/master/lib/AuthStateManager.ts#L53). This would still give it access to the tokenManager and it's config options, but also future-proofs the ServiceManager, in case we need to add a Service in the future that needs different configs

constructor(sdk: OktaAuth) {
  this.sdk = sdk;
  this.tokenManager = sdk.tokenManager;
  ...
}

this.services = new Map();
}

private startElector() {
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if the elected leader dies? For example the tab is closed. Is a new leader elected?

@@ -89,6 +87,9 @@ export class TokenManager implements TokenManagerInterface {
if (!isLocalhost()) {
options.expireEarlySeconds = DEFAULT_OPTIONS.expireEarlySeconds;
}
if (!options.broadcastChannelName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this config should moved outside of the TokenManager

export class AutoRenewService extends TokenService {
private renewTimeQueue: Array<number>;

constructor(tokenManager: TokenManager, options: TokenManagerOptions = {}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This service should have it's own Options type

private storageListener?: (event: StorageEvent) => void;
private onTokenExpiredHandler?: (key: string) => void;
private syncTimeout: unknown;
export abstract class TokenService implements TokenServiceInterface {
Copy link
Contributor

Choose a reason for hiding this comment

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

TokenService abstract class probably isn't necessary for this effort. A simple Interface with start/stop is probably sufficient

import { TokenManagerOptions } from '../types';
import { isBrowser } from '../features';

export class AutoRenewService extends TokenService {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should implement ServiceInterface

@@ -0,0 +1,7 @@
export interface TokenServiceInterface {
Copy link
Contributor

Choose a reason for hiding this comment

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

rename this to ServiceInterface. Future services may not be Token/TokenManager related

@denysoblohin-okta
Copy link
Contributor Author

Included in #1124

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.

429 Too many requests: How autoRenew and syncStorage works together?
3 participants