Skip to content

Adjust 2FA step 2 behaviour to move QR code into a modal #22775

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 21 commits into from
Nov 25, 2024
Merged

Adjust 2FA step 2 behaviour to move QR code into a modal #22775

merged 21 commits into from
Nov 25, 2024

Conversation

michalkleiner
Copy link
Contributor

@michalkleiner michalkleiner commented Nov 19, 2024 •
edited
Loading

Description:

Adjust step 2 of the 2FA setup to only show the QR code after clicking a button to prevent it from being seen unexpectedly.

Based on a desk-check review with product, I've also slightly moved the icon in the copy code button and did a small enhancement to the modal mechanism to allow to select which element should be focused when the modal is opened.

Fixes #18667.
Ref DEV-14129.

Review

@michalkleiner michalkleiner force-pushed the dev-14129-2fa-qr-code branch 3 times, most recently from dc46797 to e045c25 Compare November 21, 2024 21:55
@michalkleiner michalkleiner marked this pull request as ready for review November 22, 2024 04:32
@michalkleiner michalkleiner added Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. c: Security For issues that make Matomo more secure. Please report issues through HackerOne and not in Github. labels Nov 22, 2024
@michalkleiner michalkleiner added this to the 5.2.0 milestone Nov 22, 2024
sgiehl
sgiehl requested changes Nov 22, 2024
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Left some minor comments. Besides that some UI tests are failing and need to be updated. Otherwise this looks good to me and everything worked as expected during some local testing.

@mneudert mneudert mentioned this pull request Nov 22, 2024
Closed
11 tasks
@michalkleiner michalkleiner added the Needs Review PRs that need a code review label Nov 25, 2024
@michalkleiner michalkleiner requested review from sgiehl and a team November 25, 2024 11:18
sgiehl
sgiehl approved these changes Nov 25, 2024
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Failing tests are unrelated. Everything looks fine to me now.

@michalkleiner michalkleiner merged commit a612aaa into 5.x-dev Nov 25, 2024
23 of 26 checks passed
@michalkleiner michalkleiner deleted the dev-14129-2fa-qr-code branch November 25, 2024 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgiehl sgiehl sgiehl approved these changes

Assignees
No one assigned
Labels
c: Security For issues that make Matomo more secure. Please report issues through HackerOne and not in Github. Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. Needs Review PRs that need a code review
Milestone
5.2.0
Development

Successfully merging this pull request may close these issues.

When setting up two factor authentication (2FA), don't show the QR code right away
3 participants
@michalkleiner @sgiehl @innocraft-automation