-
Notifications
You must be signed in to change notification settings - Fork 265
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
Minimal idx factory for SIW gen3 (v2) #1476
Minimal idx factory for SIW gen3 (v2) #1476
Conversation
import { generateRemediationFunctions } from './remediationParser'; | ||
import generateIdxAction from './generateIdxAction'; | ||
import { jsonpath } from '../../../util/jsonpath'; | ||
import { AuthSdkError } from '../../../errors'; | ||
|
||
const SKIP_FIELDS = Object.fromEntries([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using Object.fromEntries
causes TS error:
Property 'fromEntries' does not exist on type 'ObjectConstructor'. Do you need to change your target library? Try changing the 'lib' compiler option to 'es2019' or later.
Changing lib
in tsconfig
doesn't help, so refactored code to not use fromEntries
@@ -11,22 +11,22 @@ | |||
*/ | |||
|
|||
/* eslint-disable max-len */ | |||
// eslint-disable-next-line @typescript-eslint/ban-ts-comment | |||
// @ts-nocheck |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed @ts-nocheck
in lib/idx/idxState/v1/*
hasResponseType(responseType: OAuthResponseType): boolean { | ||
let hasResponseType = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved code to lib/oidc/util/loginRedirect.ts
to prevent code duplication
@@ -86,9 +91,9 @@ function initializeData(authClient, data: RunData): RunData { | |||
const status = IdxStatus.PENDING; | |||
|
|||
// certain options can be set by the flow specification | |||
flow = flow || authClient.idx.getFlow() || 'default'; | |||
flow = flow || authClient.idx.getFlow?.() || 'default'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using ?.
cause idx
can be of type MinimalIdxAPI
which does not have get/set flow methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be written as authClient.idx?.getFlow()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this will throw authClient.idx?.getFlow is not a function
:
let authClient = { idx: {} }
authClient.idx?.getFlow()
lib/oidc/factory/baseApi.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using names baseApi.ts
and createBaseTokenAPI
(instead of minimal..
) cause type BaseTokenAPI
has been already created before my PR
|
||
// Factory | ||
export function createIdxAPI(sdk: OktaAuthIdxInterface): IdxAPI { | ||
setRemediatorsCtx({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See setRemediatorsCtx
in utils - https://github.com/okta/okta-auth-js/pull/1476/files#diff-71b56cc120e09a7c5f1d47c7637461aece91f39807b35359697038d48b38ddd3
This is needed for minimal IDX API to not import all remediators and flow specifications (except GenericRemediator).
By default remediatorsCtx
has empty list of remediators
.forEach( ([subField, value]) => { | ||
if (value.rel) { // is [field].value[subField] an action? | ||
// add any "action" value subfields to actions | ||
// eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
// @ts-ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the TS error being ignored here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error in subField.name
: Property 'name' does not exist on type 'string'
Looks like a valid remark from TS linter, as Object.entries
enumerates string-keyed properties.
subField.name
looks wrong
@@ -86,9 +91,9 @@ function initializeData(authClient, data: RunData): RunData { | |||
const status = IdxStatus.PENDING; | |||
|
|||
// certain options can be set by the flow specification | |||
flow = flow || authClient.idx.getFlow() || 'default'; | |||
flow = flow || authClient.idx.getFlow?.() || 'default'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be written as authClient.idx?.getFlow()
?
PR #1460 without mixin inheritance and with addressed comments
POC PR for SIW gen3: okta/okta-signin-widget#3419