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

Add information to JWT about enabled MFA's, allowing to enforce aal2 only if 2FA is enabled on user's account #1823

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

Conversation

tomekit
Copy link

@tomekit tomekit commented Oct 30, 2024

Hi Supabase team,

This is to satisfy setup in which supabase/auth genereated JWT can be passed to external API which validates JWT validity on its own. With this change the external API can determine whether to enforce aal2 level or not, based on has_factor boolean which determines if user has at least one verified factor.

Commit description:

Add field which can be used to determine if 2FA is enabled.
That way the JWT consumer (e.g. if JWT is passed to some other API which verifies its authenticity) can only enforce 2FA AAL levels once 2FA is enabled. This allows to enforce 2FA only when at least one factor is used making 2FA roll-out optional.

Please let me know if you would be happy to merge that into your codebase.

…the JWT consumer (e.g. if JWT is passed to some other API which verifies its authenticity) can only enforce 2FA AAL levels once 2FA is enabled. This allows to enforce 2FA only when at least one factor is used making 2FA roll-out optional.
@tomekit tomekit requested a review from a team as a code owner October 30, 2024 08:53
@tomekit tomekit changed the title Add field which can be used to determine if 2FA is enabled. Add information to JWT about enabled MFA's, allowing to enforce aal2 only if 2FA is enabled on user's account Oct 30, 2024
@coveralls
Copy link

Pull Request Test Coverage Report for Build 11589790641

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 5 of 9 (55.56%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 57.178%

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/api/token.go 5 9 55.56%
Totals Coverage Status
Change from base Build 11555996021: 0.02%
Covered Lines: 9571
Relevant Lines: 16739

💛 - Coveralls

@@ -309,6 +309,15 @@ func (a *API) generateAccessToken(r *http.Request, tx *storage.Connection, user
issuedAt := time.Now().UTC()
expiresAt := issuedAt.Add(time.Second * time.Duration(config.JWT.Exp)).Unix()

atLeastOneVerifiedFactor := false
for i := 0; i < len(user.Factors); i++ {
Copy link
Contributor

@J0 J0 Nov 2, 2024

Choose a reason for hiding this comment

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

Thanks for the contribution! There's a user.HasMFAEnabled() function I believe. Can consider using that . Otherwise no objections to this change - I think there might be some internal configuration we use to track claim additions, we will need to update that before merging this.

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.

3 participants