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

refactor(mfa): Add totp_url to headless mfa activate response data #3884

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mecampbellsoup
Copy link
Contributor

@mecampbellsoup mecampbellsoup commented Jun 10, 2024

Description

Adds totp_url to allauth.headless.mfa.response.TOTPNotFoundResponse. The rationale is so that we get a ready-to-render TOTP URI for our frontend and I don't have to re-implement any of the allauth.mfa.views.ActivateTOTPView.get_context_data logic in my application or elsewhere/some other way.

Submitting Pull Requests

General

  • Make sure you use semantic commit messages.
    Examples: "fix(google): Fixed foobar bug", "feat(accounts): Added foobar feature".
  • All Python code must formatted using Black, and clean from pep8 and isort issues.
  • JavaScript code should adhere to StandardJS.
  • If your changes are significant, please update ChangeLog.rst.
  • If your change is substantial, feel free to add yourself to AUTHORS.

@coveralls
Copy link

Coverage Status

coverage: 95.708%. remained the same
when pulling 4ee8a11 on mecampbellsoup:mc/mfa/add-totp-url-to-headless-resp-data
into f6577fa on pennersr:main.

@mecampbellsoup mecampbellsoup force-pushed the mc/mfa/add-totp-url-to-headless-resp-data branch 2 times, most recently from f590d46 to 8919dd7 Compare June 10, 2024 23:39
@@ -44,14 +45,21 @@ def get(self, request, *args, **kwargs):
return response.AuthenticatorsResponse(request, authenticators)


# TODO: Enforce reauthentication
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pennersr BTW, I noticed that in headless mode, enabling MFA does not explicitly check the user has authenticated recently. Is that done intentionally? Is the idea that headless allauth applications should enforce that via a separate API call, i.e. to headless:browser:mfa:reauthenticate?

Copy link
Owner

Choose a reason for hiding this comment

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

When activating TOTP, headless invokes flows.totp.activate_totp(), which has raise_if_reauthentication_required(request). Deleting TOTP, viewing and regenerating recovery codes is handled similarly. Doesn't that cover this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm I will play around and confirm!

@mecampbellsoup mecampbellsoup force-pushed the mc/mfa/add-totp-url-to-headless-resp-data branch from 8919dd7 to 46f80de Compare June 10, 2024 23:43
@coveralls
Copy link

Coverage Status

coverage: 95.709% (+0.001%) from 95.708%
when pulling 46f80de on mecampbellsoup:mc/mfa/add-totp-url-to-headless-resp-data
into f6577fa on pennersr:main.

allauth/headless/mfa/response.py Show resolved Hide resolved
allauth/headless/mfa/views.py Outdated Show resolved Hide resolved
@mecampbellsoup
Copy link
Contributor Author

@pennersr closed via 18c7ca4 right?

@mecampbellsoup mecampbellsoup force-pushed the mc/mfa/add-totp-url-to-headless-resp-data branch from 46f80de to b55497d Compare June 27, 2024 16:33
@coveralls
Copy link

Coverage Status

coverage: 95.746% (+0.001%) from 95.745%
when pulling b55497d on mecampbellsoup:mc/mfa/add-totp-url-to-headless-resp-data
into d17010b on pennersr:main.

@mecampbellsoup mecampbellsoup force-pushed the mc/mfa/add-totp-url-to-headless-resp-data branch 2 times, most recently from c603c19 to c005a69 Compare June 27, 2024 17:23
@coveralls
Copy link

Coverage Status

coverage: 95.746% (+0.001%) from 95.745%
when pulling c603c19 on mecampbellsoup:mc/mfa/add-totp-url-to-headless-resp-data
into d17010b on pennersr:main.

@mecampbellsoup
Copy link
Contributor Author

@pennersr this one ready to go now I think! 🙏

@coveralls
Copy link

Coverage Status

coverage: 95.746% (+0.001%) from 95.745%
when pulling c005a69 on mecampbellsoup:mc/mfa/add-totp-url-to-headless-resp-data
into d17010b on pennersr:main.

totp_url:
type: string
description: |
URL-encoded QR code containing the TOTP secret and other metadata to be scanned by OTP clients.
Copy link
Owner

Choose a reason for hiding this comment

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

This description seems off -- it is not a QR code right, nor is it URL encoded. It's just a URL/URI , from which the QR code could be generated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mecampbellsoup mecampbellsoup force-pushed the mc/mfa/add-totp-url-to-headless-resp-data branch from c005a69 to f7d091a Compare June 28, 2024 13:56
@mecampbellsoup mecampbellsoup force-pushed the mc/mfa/add-totp-url-to-headless-resp-data branch from f7d091a to d83533d Compare June 28, 2024 13:59
@coveralls
Copy link

Coverage Status

coverage: 95.746% (+0.001%) from 95.745%
when pulling d83533d on mecampbellsoup:mc/mfa/add-totp-url-to-headless-resp-data
into 2a98828 on pennersr:main.

@coveralls
Copy link

Coverage Status

coverage: 95.746% (+0.001%) from 95.745%
when pulling d83533d on mecampbellsoup:mc/mfa/add-totp-url-to-headless-resp-data
into 2a98828 on pennersr:main.

@mecampbellsoup
Copy link
Contributor Author

@pennersr this one ready to go now I think! 🙏

NOW it should be ready 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants