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.

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

[17.0][ADD] new module auth_admin_passkey_totp_mail_enforce #625

Merged
merged 3 commits into from
Aug 29, 2024
Merged

[17.0][ADD] new module auth_admin_passkey_totp_mail_enforce #625

merged 3 commits into from
Aug 29, 2024

Conversation

astirpe
Copy link
Member

@astirpe astirpe commented Mar 13, 2024

Depends on #624

Context
In #550 (V16) and #624 (V17) there's a proposal for module auth_admin_passkey to bypass TOTP/2FA for admin passkey.
In case module auth_totp_mail_enforce is also installed, the above solution may not work, depending on which module was installed in which order (first or second).

Proposal
We hereby propose this glue module, auto-installable in case both auth_admin_passkey auth_totp_mail_enforce are installed.
This module ensures that method _mfa_url() is executed properly, regardless of which of the modules (auth_admin_passkey or auth_totp_mail_enforce) was installed first.

codeagencybe and others added 2 commits March 13, 2024 08:34
@codeagencybe @astirpe
[IMP] option for TOTP/2FA bypass for admin passkey
8bd6f78 
In the res_users.py file, the `ignore_totp` session variable is now set based on the value of the `auth_admin_passkey_ignore_totp` configuration option. If the option is enabled, the `ignore_totp` session variable is set to True. This ensures that the `_mfa_url` method returns None when `ignore_totp` is True, effectively bypassing the 2FA check.
@astirpe
Check that request.session exists before accessing it
f138da2
@astirpe astirpe marked this pull request as ready for review March 13, 2024 13:48
CasVissers-360ERP
CasVissers-360ERP approved these changes Mar 13, 2024
View reviewed changes
Copy link

@CasVissers-360ERP CasVissers-360ERP left a comment

Choose a reason for hiding this comment

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

Functional review

legalsylvain
legalsylvain reviewed Mar 13, 2024
View reviewed changes
Copy link
Contributor

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

hi. thanks for your contribution !
could you add a test for this new module ?

thanks !

@astirpe astirpe mentioned this pull request Mar 13, 2024
Closed
@astirpe
Copy link
Member Author

astirpe commented Mar 14, 2024

@legalsylvain done, could you check?

@github-actions GitHub Actions
Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Jul 14, 2024
@github-actions github-actions bot closed this Aug 18, 2024
@CasVissers-360ERP
Copy link

@legalsylvain Can we maybe merge this? Tests were added.

@StefanRijnhart StefanRijnhart removed the stale PR/Issue without recent activity, it'll be soon closed automatically. label Aug 20, 2024
StefanRijnhart
StefanRijnhart approved these changes Aug 20, 2024
View reviewed changes
Copy link
Member

@StefanRijnhart StefanRijnhart left a comment

Choose a reason for hiding this comment

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

Thanks for adding the tests!

@StefanRijnhart
Copy link
Member

Oh @astirpe please check precommit:

auth_admin_passkey_totp_mail_enforce/tests/test_login.py:48:89: E501 Line too long (114 > 88)
auth_admin_passkey_totp_mail_enforce/tests/test_login.py:69:89: E501 Line too long (116 > 88)

@astirpe astirpe force-pushed the 17_add_auth_admin_passkey_totp_mail_enforce branch from 148f9dc to 07f503c Compare August 20, 2024 08:42
@astirpe
Copy link
Member Author

astirpe commented Aug 20, 2024

@StefanRijnhart done, thanks!

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@dreispt
Copy link
Member

dreispt commented Aug 29, 2024

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 17.0-ocabot-merge-pr-625-by-dreispt-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit acdacb2 into OCA:17.0 Aug 29, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at b01d341. Thanks a lot for contributing to OCA. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@legalsylvain legalsylvain legalsylvain left review comments

@CasVissers-360ERP CasVissers-360ERP CasVissers-360ERP approved these changes

@StefanRijnhart StefanRijnhart StefanRijnhart approved these changes

Assignees
No one assigned
Labels
approved merged 🎉 ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@astirpe @CasVissers-360ERP @StefanRijnhart @OCA-git-bot @dreispt @legalsylvain @codeagencybe