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

MFA Enrollment #979

Closed
patrykczubek opened this issue Feb 19, 2023 · 12 comments
Closed

MFA Enrollment #979

patrykczubek opened this issue Feb 19, 2023 · 12 comments
Assignees
Labels
bug Something isn't working documentation Improvements or additions to documentation

Comments

@patrykczubek
Copy link

Bug report

Describe the bug

Using the MFA enroll function, the table gets spammed with new unverified factors on website refresh.

Expected behavior

If the user doesn't have the MFA enrollment started, it should work like how it does as of now. However, if there's already a enrollment process started before by the user, it should reuse the existing QR code.

Screenshots

image
image

@patrykczubek patrykczubek added the bug Something isn't working label Feb 19, 2023
@patrykczubek patrykczubek changed the title MFA Enrollemtn MFA Enrollment Feb 19, 2023
@J0
Copy link
Contributor

J0 commented Feb 20, 2023

Hey @patrykczubek,

Thanks for the feedback! This is a good suggestion - as an immediate fix it should be possible to add a state variable in React to ensure it's only called once on the enrollment page. We'll look into the docs/lib to see if there's a better way to handle this

@J0 J0 added the documentation Improvements or additions to documentation label Feb 28, 2023
@tomekit
Copy link

tomekit commented Mar 14, 2023

On our side every time MFA enrolment starts, we delete all unverified factors to avoid the mess in the table and avoid exceeding maximum allowed MFAs.

Perhaps Supabase could cleanup unverified factors after some inactivity time. I don't see what's the point of having unverified factor sitting for a longer than couple minutes (unless there is some MFA workflow which is longer than that).

There is also a config param, specifying how many MFA factors user can have (default 10). I believe this setting shouldn't apply for unverified factors, because currently user can easily "lock themself" out without an option to create a new MFA.

I think reusing QR codes poses some security risks. Once QR code disappear from the screen it should never be displayed again, but that's only my opinion.

@J0
Copy link
Contributor

J0 commented Mar 14, 2023

Hey @tomekit,

Thanks for the feedback! It's helpful in deciding what to prioritize. We're planning on adding a background task of sorts to clean up unverified factors after an interval.

There is also a config param, specifying how many MFA factors user can have (default 10). I believe this setting shouldn't apply for unverified factors, because currently user can easily "lock themself" out without an option to create a new MFA.

This is also a great suggestion, the fix will probably go out with the background task mentioned above.

I think reusing QR codes poses some security risks.

Yes, this is a good point, I'd forgot to consider this and perhaps the proper fix here should be to amend the DX around registering a factors

Please do keep the feedback coming!

@bearbricknik
Copy link

Hi, have you found a solution to the problem yet? I have the same problem and cant figure out a solution that is working reliably without increasing the mac mfa limit to an absurd number but then still the user is able to log himself out

The only idea I had was setting triggers that might delete unverified mfa factors from the table after a period of time.

@J0 J0 self-assigned this Jan 1, 2024
@J0
Copy link
Contributor

J0 commented Jan 19, 2024

Hey @bearbricknik ,

Thanks for reporting the issue you're facing! Quick question about:

cant figure out a solution that is working reliably without increasing the mac mfa limit to an absurd number but then still the user is able to log himself out

Are you primarily facing the issue of hitting the max mfa limit in development or in production? We have an existing cleanup after a factor is verified but am floating the idea of adding a cleanup at two other areas:

  1. Every 24h - all unverified factors will be cleaned up
  2. During enroll - all stale factors (older than five minutes, no challenges made)

This won't solve the issue of hitting max enrolled factors during development though since I guess dev might make quite a few invocations during 24h. Trying to understand the issue better before attempting to resolve that.

Lmk!

@bearbricknik
Copy link

bearbricknik commented Jan 19, 2024 via email

@Wamy-Dev
Copy link

I have encountered this issue during development due to refreshes. A fix for this would be awesome.

@laurencebedford
Copy link

Same here I am now getting from the server:
{"code":403,"msg":"Enrolled factors exceed allowed limit, unenroll to continue"}

how do I fix this? - if I get this error delete the factors? im confused since the docs doesn't cover this case:
https://supabase.com/docs/guides/auth/auth-mfa#apis

@J0
Copy link
Contributor

J0 commented Mar 6, 2024

Hi you can probably call unenroll on the factors. If needed you can do select id from auth.mfa_factors where status='unverified'

I'll also bump the linked PR for review today - lmk if there are other issues

@bearbricknik
Copy link

To give you guys an idea how I did it: Everytime a user enrolls a new factor, I am calling the DB and check for any unverified ones. If there are any, I call the unenroll on every single unverified one before enrolling a new one.

To be clear, its probably not an ideal solution but a little loop over the unverified ones does the job before enrolling a new one.

Hope that helps you.

@laurencebedford
Copy link

To give you guys an idea how I did it: Everytime a user enrolls a new factor, I am calling the DB and check for any unverified ones. If there are any, I call the unenroll on every single unverified one before enrolling a new one.

To be clear, its probably not an ideal solution but a little loop over the unverified ones does the job before enrolling a new one.

Hope that helps you.

thats what I was planning on doing. However, it just seems rather botched and what if a user uses this maliciously to just repeatedly refresh the page - resulting on hundreds of unenrolls / enrolls?

J0 added a commit that referenced this issue Mar 13, 2024
## What kind of change does this PR introduce?

Currently, unverified MFA factors can build up in the database quickly.
Supabase developers can toggle Maximum unverified factors ( `Maximum
number of per-user MFA factors`) via the dashboard but developers will
have to look for the toggle. Developers can also call `unenroll` but it
requires an additional step.


This PR proposes periodic cleanup of stale factors on each request. A
stale factor is:

- Unverified
- Has no associated Challenges 
- Older than five minutes

Why five minutes? 
- Most enrolment or verification flow should be completed within the
five minute window

Factors which are unverified but have associated challenges will be
cleaned up [after the developer makes successful
verification](https://github.com/supabase/gotrue/blob/master/internal/api/mfa.go#L314)

Alternatives considered:
- Return the same factor and QR code if the same user calls `/enroll`
twice. We unfortunately can't reuse the QR code as it poses a security
risk.
- Increase the initial number of default unverified factors (currently
10)
- Drop the unverified factor check. I think this was initially
introduced to prevent a malicious user from creating excessive entries
in the database


Address #979.

---------

Co-authored-by: joel@joellee.org <joel@joellee.org>
Co-authored-by: joel <joel@joels-MacBook-Pro.local>
@kangmingtay
Copy link
Member

i'm closing this issue since it seems to be resolved by #1371 - please feel free to reopen it if there are still any issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

7 participants