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

Allow any user to manage two-factor #16130

Merged
merged 33 commits into from
May 28, 2024
Merged

Conversation

MikeAlhayek
Copy link
Member

@MikeAlhayek MikeAlhayek commented May 21, 2024

Fix #15335
Fix #16129

Summary of changes

  • The feature OrchardCore.Users.EmailConfirmation was removed. This feature was recently added and was enabled by dependency only. It's services have been moved up to the OrchardCore.Users feature since it is a core functional and needed by OrchardCore.Users.
  • Using 2FA email method now requires email confirmation using the existing email confirmation process.
  • The method IsRequiredAsync() in both the ITwoFactorAuthenticationHandlerCoordinator and ITwoFactorAuthenticationHandler was changed to IsRequiredAsync(IUser user).

@hishamco
Copy link
Member

Can we have some unit tests?

@MikeAlhayek
Copy link
Member Author

Sorry I don't have much time. But these bugs need to be addressed. If you want to submit test, you're more than welcome to add tests.

@hishamco
Copy link
Member

I might suggest adding tests for upcoming PRs to ensure that we don't break anything and improve the quality

@MikeAlhayek MikeAlhayek requested a review from Piedone May 22, 2024 16:09
@MikeAlhayek
Copy link
Member Author

@hishamco meanwhile, do you like to review this PR?

@Piedone do you also like to review this PR?

@hishamco
Copy link
Member

hishamco commented May 23, 2024

@MikeAlhayek I have a workshop on Saturday, and I need to prepare something, I can review and probably add unit tests after

@Piedone
Copy link
Member

Piedone commented May 23, 2024

I'm reviewing.

@MikeAlhayek MikeAlhayek requested a review from Piedone May 24, 2024 02:35
@MikeAlhayek
Copy link
Member Author

@Piedone this one is ready for another review

Copy link
Contributor

This pull request has merge conflicts. Please resolve those before requesting a review.

@MikeAlhayek
Copy link
Member Author

@Piedone I added 2 new layouts to the Blog theme so we have a proper theme for the login and 2FA.

Still now sure why does the footer icons in the blog theme look huge when using 2FA. which appears on the main 2FA index only and not the other pages.

@MikeAlhayek
Copy link
Member Author

@Piedone Last commit. I undid the changes that I made to the blog theme. That should be done in a separate PR to fix existing issues in the blog theme. Are you good with this now?

@Piedone
Copy link
Member

Piedone commented May 28, 2024

I don't think the Blog theme should contain anything like that, so with that part, yes. However, the net of the last two commits are still changes that I'm confused why are necessary: https://github.com/OrchardCMS/OrchardCore/pull/16130/files/3f9c1d25cf36b69a60cd7e71a0b4caffcf7e23ac..0be22c7c1469bd248f36604e9a14b0ab626c13b2

@MikeAlhayek
Copy link
Member Author

look at the last commit using script instead of style only in the blog theme and the other layouts seems to have fixed the issues.

Using blog theme with the 2FA does not work because the menu seems to cover the disable button. I think that is something we can fix in the theme in a separate PR.

@Piedone
Copy link
Member

Piedone commented May 28, 2024

Why are any of the changes in the last three commits necessary? We don't need to care about 2FA not working with the Blog theme, as mentioned; if one wants to have it on the frontend, one will have a custom theme anyway.

@MikeAlhayek
Copy link
Member Author

Well, it all started when you mentioned that the icons are not working with the Blog Theme. Then you had another comment about the icons in the footer showing up very large. And then you had another comment about unable to enable 2FA when you were using the blog theme.

In my last commit, at least the icons are fixed. In a follow up PR, we should update the blog theme and the agency theme by providing a layout for the login and the TwoFactor layouts so that both look good by default.

@Piedone
Copy link
Member

Piedone commented May 28, 2024

Yes, but as I followed up under those comments, nothing needs to be done. I was incorrect to bring them up.

@MikeAlhayek
Copy link
Member Author

Okay. So do you need any other changes to this PR or should we merge it?

@Piedone
Copy link
Member

Piedone commented May 28, 2024

Yes, please revert what's after my approve.

@MikeAlhayek MikeAlhayek enabled auto-merge (squash) May 28, 2024 16:28
@MikeAlhayek
Copy link
Member Author

done. merging this

@MikeAlhayek MikeAlhayek merged commit a95a876 into main May 28, 2024
7 checks passed
@MikeAlhayek MikeAlhayek deleted the ma/fix-2fa-admin-attribute branch May 28, 2024 16:36
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.

Unable to enable 2FA using email when the email is not verified Allow 2FA for non-admin users too
3 participants