Skip to content

Commit 8676548

Browse files
J0hf
authored andcommitted
fix: update aal requirements to update user (#1766)
## What kind of change does this PR introduce? If a user has verified factors (mfa enabled) we should require an AAL2 session in order to proceed with any operation We restrict phone, email, and password from updates as we consider those as sensitive fields Context: https://supabase.slack.com/archives/C02AK9166FR/p1725466764804889 --------- Co-authored-by: Stojan Dimitrovski <[email protected]>
1 parent c0f167c commit 8676548

File tree

3 files changed

+78
-0
lines changed

3 files changed

+78
-0
lines changed

internal/api/mfa_test.go

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -295,6 +295,68 @@ func (ts *MFATestSuite) TestDuplicateTOTPEnrollsReturnExpectedMessage() {
295295
require.Contains(ts.T(), errorResponse.Message, expectedErrorMessage)
296296
}
297297

298+
func (ts *MFATestSuite) AAL2RequiredToUpdatePasswordAfterEnrollment() {
299+
resp := performTestSignupAndVerify(ts, ts.TestEmail, ts.TestPassword, true /* <- requireStatusOK */)
300+
accessTokenResp := &AccessTokenResponse{}
301+
require.NoError(ts.T(), json.NewDecoder(resp.Body).Decode(&accessTokenResp))
302+
303+
var w *httptest.ResponseRecorder
304+
var buffer bytes.Buffer
305+
token := accessTokenResp.Token
306+
// Update Password to new password
307+
newPassword := "newpass"
308+
require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{
309+
"password": newPassword,
310+
}))
311+
312+
req := httptest.NewRequest(http.MethodPut, "http://localhost/user", &buffer)
313+
req.Header.Set("Content-Type", "application/json")
314+
315+
req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token))
316+
317+
w = httptest.NewRecorder()
318+
ts.API.handler.ServeHTTP(w, req)
319+
require.Equal(ts.T(), http.StatusOK, w.Code)
320+
321+
// Logout
322+
reqURL := "http://localhost/logout"
323+
req = httptest.NewRequest(http.MethodPost, reqURL, nil)
324+
req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token))
325+
w = httptest.NewRecorder()
326+
327+
ts.API.handler.ServeHTTP(w, req)
328+
require.Equal(ts.T(), http.StatusNoContent, w.Code)
329+
330+
// Get AAL1 token
331+
require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{
332+
"email": ts.TestEmail,
333+
"password": newPassword,
334+
}))
335+
336+
req = httptest.NewRequest(http.MethodPost, "http://localhost/token?grant_type=password", &buffer)
337+
req.Header.Set("Content-Type", "application/json")
338+
w = httptest.NewRecorder()
339+
ts.API.handler.ServeHTTP(w, req)
340+
require.Equal(ts.T(), http.StatusOK, w.Code)
341+
session1 := AccessTokenResponse{}
342+
require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&session1))
343+
344+
// Update Password again, this should fail
345+
require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{
346+
"password": ts.TestPassword,
347+
}))
348+
349+
req = httptest.NewRequest(http.MethodPut, "http://localhost/user", &buffer)
350+
req.Header.Set("Content-Type", "application/json")
351+
352+
req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", session1.Token))
353+
354+
w = httptest.NewRecorder()
355+
ts.API.handler.ServeHTTP(w, req)
356+
require.Equal(ts.T(), http.StatusUnauthorized, w.Code)
357+
358+
}
359+
298360
func (ts *MFATestSuite) TestMultipleEnrollsCleanupExpiredFactors() {
299361
// All factors are deleted when a subsequent enroll is made
300362
ts.API.config.MFA.FactorExpiryDuration = 0 * time.Second

internal/api/user.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,12 @@ func (a *API) UserUpdate(w http.ResponseWriter, r *http.Request) error {
100100
}
101101
}
102102

103+
if user.HasMFAEnabled() && !session.IsAAL2() {
104+
if (params.Password != nil && *params.Password != "") || (params.Email != "" && user.GetEmail() != params.Email) || (params.Phone != "" && user.GetPhone() != params.Phone) {
105+
return httpError(http.StatusUnauthorized, ErrorCodeInsufficientAAL, "AAL2 session is required to update email or password when MFA is enabled.")
106+
}
107+
}
108+
103109
if user.IsAnonymous {
104110
if params.Password != nil && *params.Password != "" {
105111
if params.Email == "" && params.Phone == "" {

internal/models/user.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -772,6 +772,16 @@ func (u *User) IsBanned() bool {
772772
return time.Now().Before(*u.BannedUntil)
773773
}
774774

775+
func (u *User) HasMFAEnabled() bool {
776+
for _, factor := range u.Factors {
777+
if factor.IsVerified() {
778+
return true
779+
}
780+
}
781+
782+
return false
783+
}
784+
775785
func (u *User) UpdateBannedUntil(tx *storage.Connection) error {
776786
return tx.UpdateOnly(u, "banned_until")
777787
}

0 commit comments

Comments
 (0)