-
Notifications
You must be signed in to change notification settings - Fork 321
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
OKTA-722502 : Same device enrollment flow updates #3632
OKTA-722502 : Same device enrollment flow updates #3632
Conversation
728a936
to
8534a67
Compare
ec767ff
to
4e057c2
Compare
4e057c2
to
1404568
Compare
@@ -79,7 +99,7 @@ export default View.extend({ | |||
{{#if sameDeviceOVEnrollmentEnabled}} | |||
<li> | |||
<span>{{i18n code="customUri.required.content.download.title" bundle="login"}}</span> | |||
<a href="{{deviceMap.downloadHref}}" target="_blank" id="download-ov" class="orOnMobileLink"> |
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.
this was causing the download link to navigate to the enroll on another device flow
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.
Can you point out where this unwanted behavior was coming from?
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.
yeah here is where the click event is attached to the Set up on another device link and looks for this classname. Since this download link was using the same classname, that event was being triggered when this link was pressed.
function createInvisibleIFrame(iFrameId, iFrameSrc) { | ||
const iFrameView = View.extend({ | ||
tagName: 'iframe', | ||
id: iFrameId, | ||
attributes: { | ||
src: iFrameSrc, | ||
}, | ||
initialize() { | ||
this.el.style.display = 'none'; | ||
} | ||
}); | ||
return iFrameView; | ||
} |
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 to ChallengeViewUtil.js
src/v3/src/components/OpenOktaVerifyFPButton/OpenOktaVerifyFPButton.tsx
Outdated
Show resolved
Hide resolved
src/v3/src/transformer/oktaVerify/transformOktaVerifyChannelSelection.ts
Outdated
Show resolved
Hide resolved
channelSelectionElement.options.format = 'radio'; | ||
const { options: { inputMeta: { options = [] } } } = channelSelectionElement; | ||
channelSelectionElement.options.customOptions = options | ||
.filter((opt) => opt.value !== lastSelectedChannel) | ||
.filter((opt) => { |
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.
Is it possible for samedevice
to be the only enrollment option? If so, channelSelectionElement.options.customOptions[0].value
below will fail
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.
it is possible for samedevice
to be the only channel, but that is only when OV security setting is set to HIGH. In that case, the user would not have to option to navigate to this channel selection screen anyway, so It probably wouldnt be a problem. But I have added a safeguard if condition just to be safe.
sameDeviceChannelField.options = sameDeviceChannelField.options?.filter((option) => | ||
option.value === sameDeviceChannel); |
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.
Same concern here about the filtered result in the event that samedevice
is the only channel. Will that cause issues?
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.
I dont think it should be an issue because the user should not have access to the channel selection screen; when samedevice
is the only channel, that means OV security is set to HIGH. The Or set up Okta Verify on a mobile device
link to enroll in other channels will not display when security is set to HIGH
if (!this.settings.get('features.sameDeviceOVEnrollmentEnabled')) { | ||
this.add(SwitchEnrollChannelLinkView, 'form'); | ||
} |
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.
Since the conditional is gone, what is the impact of this change?
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.
now with this change the switch channel link will display on the enrollment channel page (sms, email etc. ) when the
sameDeviceOVEnrollmentEnabled feature is enabled.
which matches the figma here: https://www.figma.com/file/ZP8O9yyMXIsnOhyt8yOKcw/OV-local-enrollment-enhancements?type=design&node-id=2728-15464&mode=design&t=mZ4a71AGYREvZH2Q-0
src/v2/view-builder/views/ov/EnrollChannelPollDescriptionView.js
Outdated
Show resolved
Hide resolved
src/v2/view-builder/views/ov/EnrollChannelPollDescriptionView.js
Outdated
Show resolved
Hide resolved
// do not show the 'samedevice' radio option, that option is displayed as a link instead | ||
channelField.options = _.filter(channelField?.options, (option) => ( | ||
option.value !== this.options.appState.get('currentAuthenticator')?.contextualData?.selectedChannel | ||
&& option.value !== 'samedevice' | ||
)); |
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.
I think my comment in SameDeviceEnrollLink
should actually have been 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.
I dont think it should be an issue because the user should not have access to the channel selection screen; when samedevice is the only channel, that means OV security is set to HIGH. The Or set up Okta Verify on a mobile device link to enroll in other channels will not display when security is set to HIGH
text: string; | ||
}; | ||
|
||
const ListItemText: FunctionComponent<ListItemTextProps> = ({ text }) => { |
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.
Not a blocker at all, but is there much value in this component compared to inserting the useHtmlContentParser
hook in List
? This is essentially a thin wrapper around that
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.
was not able to use the useHtmlContentParser hook in List without getting the
React Hook "useHtmlContentParser" cannot be called inside a callback. React Hooks must be called in a React function component or a custom React Hook function react-hooks/rules-of-hooks
error
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.
In List.tsx
there already is usage of a hook useOdysseyDesignTokens()
- why is this hook not allowed?
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.
looks like because we use useHtmlContentParser
within a callback for the array map on line 100. Which in that case the hooks call order cannot be guaranteed: https://www.dhiwise.com/post/fixing-the-react-hook-cannot-be-called-inside-a-callback#:~:text=This%20is%20because%20React%20relies%20on%20the%20order%20of%20hook%20calls%20to%20track%20the%20state%20and%20effects%20associated%20with%20each%20hook%20properly.%20When%20you%20place%20a%20hook%20inside%20a%20callback%20function%2C%20the%20hook%27s%20call%20order%20can%20no%20longer%20be%20guaranteed%2C%20as%20the%20callback%20function%20may%20not%20be%20called%20on%20every%20render.
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.
im questioning whether useHtmlContentParser
even needs to be a hook, seems to just act like a normal function that parses some text on the spot. Maybe we could just treat it as a regular function?
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.
@leemchale-okta you're right - let's move this to another directory (maybe there's something for utils) and rename it so it doesn't get picked up by the linter for hooks rules.
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.
yeah agreed sounds good will get that refactored.
slack discussion ref: https://okta.slack.com/archives/C04NYNLLRDL/p1721768477843139
@@ -199,7 +199,7 @@ exports[`okta-verify-enrollment-version-upgrade should render form 1`] = ` | |||
<li | |||
class="MuiListItem-root emotion-33" | |||
> | |||
On your mobile device, download the Okta Verify app from the App Store (iPhone and iPad) or Google Play (Android devices). | |||
On your other device, download the Okta Verify app from the App Store® (iPhone® and iPad®) or on Google Play (Android? devices). |
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.
Might want to check the translation string here - "Android" has a ?
showing 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.
looks like this key was originally added here: df224e3#:~:text=oie.enroll.okta_verify.qrcode.step1.updated%20%3D%20On%20your%20other%20device%2C%20download%20the%20Okta%20Verify%20app%20from%20the%20App%20Store%C2%AE%20(iPhone%C2%AE%20and%20iPad%C2%AE)%20or%20on%20Google%20Play%20(Android%3F%20devices).
with a "?" by mistake. Asking globalization if this can be fixed manually
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.
Make sure the downstream monolith is passing before merging since there's some classname changes to the Gen2 widget in this PR
Description:
This PR adds the following updates to the Okta Verify enrollment flow in SIW Gen2 & Gen 3:
-Updates to use new translation keys
-Adds the
Or set up Okta Verify on this device
link on the select enroll channel view when Same device enrollment is available. (this feature relies on the backend change below)Backend PR to fix
samedevice
channel option not being included in response:https://github.com/atko-eng/okta-core/pull/95705
Figma reference: https://www.figma.com/file/ZP8O9yyMXIsnOhyt8yOKcw/OV-local-enrollment-enhancements?type=design&node-id=2728-15464&mode=design&t=mZ4a71AGYREvZH2Q-0
Design approval: https://okta.slack.com/archives/C06NW6QAHLZ/p1721323879609869?thread_ts=1721087617.224389&cid=C06NW6QAHLZ
Downstream test on monolith: https://bacon-go.aue1e.saasure.net/commits?artifact=monolith&author=lee.mchale%40okta.com&branch=LM-OKTA-722502-downstream-test&page=1&pageSize=6&tab=main
Gen 2
Or set up Okta Verify on this device
link flow:Screen.Recording.2024-07-17.at.10.10.07.PM.mov
Gen 3
Or set up Okta Verify on this device
link flow:Screen.Recording.2024-07-17.at.10.36.42.PM.mov
PR Checklist
Issue:
Reviewers:
Screenshot/Video:
Downstream Monolith Build: