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

poc: Light idx factory for SIW gen3 #1460

Closed
wants to merge 8 commits into from

Conversation

denysoblohin-okta
Copy link
Contributor

@denysoblohin-okta denysoblohin-okta commented Oct 11, 2023

OKTA-656620

Changes:

  • Created new mixinBaseOAuth to replace mixinOAuth - removed TokenManager and unnecessary API methods like storeTokensFromRedirect, signInWithRedirect, isAuthenticated, revokeAccessToken, revokeRefreshToken, signOut
  • Created createBaseTokenAPI to replace createTokenAPI - removed all methods except 3 needed ones. This function is called from mixinBaseOAuth
  • Created new createBaseIdxAPI to replace createIdxAPI - removed methods that are not needed in SIW like interact, introspect, poll etc.
  • Created mixinBaseIdx to replace mixinIdx - calls createBaseIdxAPI to construct authClient.idx
  • To not import all idx remediators and idx authenticators (we need only GenericRemediator), refactoring was made: Added allRemediators and getFlowSpecification into authClient.idx . In createIdxAPI() it's initialized by importing from 'lib/idx/remediators' and lib/idx/flow , in createBaseIdxAPI() it's initialized with empty objects.
  • Exporting new factory createOktaAuthBaseIdx that applies mixinBaseOAuth and mixinBaseIdx to core, resulted light-weight class can be used by SIW gen3

Size comparison:

okta-auth-js.idx.min.js 342 KiB
okta-auth-js.idx-lite.min.js 181 KiB

Webpack bundle analyzer:
Before:

Screenshot 2023-10-11 at 19 12 36

After:

Screenshot 2023-10-11 at 19 12 42

siw-next-bundle-analyze.zip


classDiagram

class BaseIdxAPI {
    -start()
    -proceed()
    -getSavedTransactionMeta()
    -clearTransactionMeta()

    -makeIdxResponse()
    -startTransaction()
}
class IdxAPI {
    -interact()
    -introspect()
    -poll()
    -authenticate()
    -register()
    -recoverPassword()
    -unlockAccount()
    -getFlow()
    -setFlow()
    -handleInteractionCodeRedirect()
}
BaseIdxAPI <|-- IdxAPI


class mixinBaseIdx["mixinBaseIdx()"] {
    +webauthn
    -idx: BaseIdxAPI
}
class mixinIdx["mixinIdx()"] {
    +webauthn
    -idx: IdxAPI
}
mixinBaseIdx <-- mixinIdx


class mixinCore["mixinCore()"] {
    -authStateManager
    -serviceManager
    -start()
    -stop()
    -handleLoginRedirect()
}

class mixinBaseOAuth["mixinBaseOAuth()"] {
    -token: BaseTokenAPI
    -transactionManager
    -isPKCE()
    -isAuthorizationCodeFlow()
}
class mixinOAuth["mixinOAuth()"] {
    -transactionManager
    +crypto // token verification
    -tokenManager
    -pkce // generateVerifier, computeChallenge
    -token: TokenAPI
    -isAuthenticated()
    -signInWithRedirect()
    -getUser()
    -getAccessToken()
    -storeTokensFromRedirect()
    -revokeAccessToken()
    -signOut()
    -clearStorage()
}
mixinBaseOAuth <-- mixinOAuth

class BaseTokenAPI:::base {
    -exchangeCodeForTokens()
}
class TokenAPI {
    -getWithoutPrompt()
    -getWithPopup()
    -getWithRedirect()
    -revoke()
    -renew()
    -getUserInfo()
    -verify()
}
BaseTokenAPI <|-- TokenAPI
Loading

Downstream to SIW:
https://github.com/okta/okta-signin-widget/compare/master...od-test-bundle-idx-lite
https://bacon-go.aue1e.saasure.net/commits?artifact=okta-signin-widget&branch=od-test-bundle-idx-lite&page=1&pageSize=6&tab=main


@denysoblohin-okta denysoblohin-okta force-pushed the od-bundle-OKTA-462738 branch 3 times, most recently from a418ee9 to 0479e1e Compare October 12, 2023 14:19
@denysoblohin-okta denysoblohin-okta changed the title poc: Light idx build for SIW gen3 poc: Light idx factory for SIW gen3 Oct 16, 2023
@@ -1,6 +1,5 @@
import { warn, split2 } from '../util';
import * as remediators from './remediators';
import { RemediationValues, Remediator, RemediatorConstructor } from './remediators';
import { RemediationValues, Remediator, RemediatorConstructor } from './remediators/Base/Remediator';
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, what kind of size savings does this pattern of changing the path to a specific file rather than importing the barrel file yield? Does ESM tree shaking not take care of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. Changes like this don't make size savings for ESM bundle.
When I added idx-lite CDN bundle in first commits, these changes reduced CDN bundle size.


// Factory
export function createIdxAPI(sdk: OktaAuthIdxInterface): IdxAPI {
const boundStartTransaction = startTransaction.bind(null, sdk);
const baseApi = createBaseIdxAPI(sdk);
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand the pattern being established here, however it adds complexity to an already complex pattern. I'd prefer this file to not change at all and

import { createBaseIdxAPI } from './baseApi';

become something like minimalApi, where api and minimalApi would have redundant export statements and these mixins/factories would be completely isolated from each other. (The goal being to simply the factory/mixin pattern)

isTransactionMetaValid,
saveTransactionMeta
} from '../transactionMeta';
import { FlowIdentifier, BaseIdxAPI, OktaAuthIdxInterface, FlowSpecification } from '../types';
Copy link
Contributor

Choose a reason for hiding this comment

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

See comment on /factory/api, but I am imaging this file as

Suggested change
import { FlowIdentifier, BaseIdxAPI, OktaAuthIdxInterface, FlowSpecification } from '../types';
import { FlowIdentifier, MinimalIDXAPI, MinimalOAuthInterfaceForIDX, FlowSpecification } from '../types';
export function createMinimalIdxAPI(sdk: MinimalOAuthInterfaceForIDX): MinimalIDXAPI {

@@ -115,7 +114,7 @@ export function getAvailableSteps(
): NextStep[] {
const res: NextStep[] = [];

const remediatorMap: Record<string, RemediatorConstructor> = Object.values(remediators)
const remediatorMap: Record<string, RemediatorConstructor> = Object.values(authClient.idx.allRemediators)
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand why this is being done, but I am very wary of adding this map to the idx client itself. This then become part of the public contract and customers will start interfacing with it, which we really don't want them to

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.

2 participants