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

feat: CXSPA-6689 one time password for login #18752

Merged
merged 106 commits into from
May 16, 2024
Merged

feat: CXSPA-6689 one time password for login #18752

merged 106 commits into from
May 16, 2024

Conversation

i333055
Copy link
Contributor

@i333055 i333055 commented Apr 22, 2024

Call OTP Creation API on login form: https://jira.tools.sap/browse/CXSPA-6672
Login with OTP: https://jira.tools.sap/browse/CXSPA-6689

Hi, @SAP/spartacus-codeowners could you help review and approve for change of projects/core module, thanks!

Hi @morganm58 , @developpeurweb could you help review the UI styles, thanks!

@i333055 i333055 requested review from a team as code owners April 22, 2024 09:05
@github-actions github-actions bot marked this pull request as draft April 22, 2024 09:05
@github-actions github-actions bot marked this pull request as draft May 15, 2024 01:33
@i333055 i333055 marked this pull request as ready for review May 15, 2024 06:44
@github-actions github-actions bot marked this pull request as draft May 15, 2024 07:39
@i333055 i333055 marked this pull request as ready for review May 15, 2024 08:31
@github-actions github-actions bot marked this pull request as draft May 15, 2024 08:44
@i333055 i333055 marked this pull request as ready for review May 15, 2024 09:00
Copy link
Contributor

@i53577 i53577 left a comment

Choose a reason for hiding this comment

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

LGTM

@developpeurweb
Copy link
Contributor

I'll approve but you still have very basic Accessibility issues that will come up in audits for sure, please create a ticket for each issue.

  1. You have to complement all role="button" elements with full keyboard functionality as if they were real buttons. Both Enter and Spacebar should work for buttons. Right now only one of them works.
  2. To comply with requirement ACC-278 you still need to provide context for the verification field using aria-describedby as shown previously in the password registration example. It could be something very simple, such as: "You can request a new code by activating the button below. A timer will start indicating the seconds left until you can request a new one".
  3. When using keyboard-only navigation and focusing on the "Resend" button, the focus gets trapped and I can keep pressing Spacebar and reset the timer every time. Same thing when using a Screen Reader on read mode, I can still access the button while it's supposed to be disabled. This is an Accessibility issue, but might as well be a Security issue too. My suggestion is to disable the button on activation and remove focus from element so it's not reachable with keyboard and add aria-disabled so screen readers will ignore it until it becomes focusable again. Tagging @giancorderoortiz to follow up on Security concerns.

@github-actions github-actions bot marked this pull request as draft May 16, 2024 02:04
@i333055 i333055 marked this pull request as ready for review May 16, 2024 03:19
@i333055 i333055 merged commit 01269ce into develop May 16, 2024
26 checks passed
@i333055 i333055 deleted the feat/CXSPA-6689 branch May 16, 2024 04:18
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.

6 participants