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]>
supabase · Sep 17, 2024 · 25d9874 · 25d9874
1 parent 77d5897
commit 25d9874
Show file tree

Hide file tree

Showing 3 changed files with 78 additions and 0 deletions.
diff --git a/internal/api/mfa_test.go b/internal/api/mfa_test.go
@@ -295,6 +295,68 @@ func (ts *MFATestSuite) TestDuplicateTOTPEnrollsReturnExpectedMessage() {
 	require.Contains(ts.T(), errorResponse.Message, expectedErrorMessage)
 }
 
+func (ts *MFATestSuite) AAL2RequiredToUpdatePasswordAfterEnrollment() {
+	resp := performTestSignupAndVerify(ts, ts.TestEmail, ts.TestPassword, true /* <- requireStatusOK */)
+	accessTokenResp := &AccessTokenResponse{}
+	require.NoError(ts.T(), json.NewDecoder(resp.Body).Decode(&accessTokenResp))
+
+	var w *httptest.ResponseRecorder
+	var buffer bytes.Buffer
+	token := accessTokenResp.Token
+	// Update Password to new password
+	newPassword := "newpass"
+	require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{
+		"password": newPassword,
+	}))
+
+	req := httptest.NewRequest(http.MethodPut, "http://localhost/user", &buffer)
+	req.Header.Set("Content-Type", "application/json")
+
+	req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token))
+
+	w = httptest.NewRecorder()
+	ts.API.handler.ServeHTTP(w, req)
+	require.Equal(ts.T(), http.StatusOK, w.Code)
+
+	// Logout
+	reqURL := "http://localhost/logout"
+	req = httptest.NewRequest(http.MethodPost, reqURL, nil)
+	req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token))
+	w = httptest.NewRecorder()
+
+	ts.API.handler.ServeHTTP(w, req)
+	require.Equal(ts.T(), http.StatusNoContent, w.Code)
+
+	// Get AAL1 token
+	require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{
+		"email":    ts.TestEmail,
+		"password": newPassword,
+	}))
+
+	req = httptest.NewRequest(http.MethodPost, "http://localhost/token?grant_type=password", &buffer)
+	req.Header.Set("Content-Type", "application/json")
+	w = httptest.NewRecorder()
+	ts.API.handler.ServeHTTP(w, req)
+	require.Equal(ts.T(), http.StatusOK, w.Code)
+	session1 := AccessTokenResponse{}
+	require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&session1))
+
+	// Update Password again, this should fail
+	require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{
+		"password": ts.TestPassword,
+	}))
+
+	req = httptest.NewRequest(http.MethodPut, "http://localhost/user", &buffer)
+	req.Header.Set("Content-Type", "application/json")
+
+	req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", session1.Token))
+
+	w = httptest.NewRecorder()
+	ts.API.handler.ServeHTTP(w, req)
+	require.Equal(ts.T(), http.StatusUnauthorized, w.Code)
+
+}
+
 func (ts *MFATestSuite) TestMultipleEnrollsCleanupExpiredFactors() {
 	// All factors are deleted when a subsequent enroll is made
 	ts.API.config.MFA.FactorExpiryDuration = 0 * time.Second

diff --git a/internal/api/user.go b/internal/api/user.go
@@ -100,6 +100,12 @@ func (a *API) UserUpdate(w http.ResponseWriter, r *http.Request) error {
 		}
 	}
 
+	if user.HasMFAEnabled() && !session.IsAAL2() {
+		if (params.Password != nil && *params.Password != "") || (params.Email != "" && user.GetEmail() != params.Email) || (params.Phone != "" && user.GetPhone() != params.Phone) {
+			return httpError(http.StatusUnauthorized, ErrorCodeInsufficientAAL, "AAL2 session is required to update email or password when MFA is enabled.")
+		}
+	}
+
 	if user.IsAnonymous {
 		if params.Password != nil && *params.Password != "" {
 			if params.Email == "" && params.Phone == "" {

diff --git a/internal/models/user.go b/internal/models/user.go
@@ -772,6 +772,16 @@ func (u *User) IsBanned() bool {
 	return time.Now().Before(*u.BannedUntil)
 }
 
+func (u *User) HasMFAEnabled() bool {
+	for _, factor := range u.Factors {
+		if factor.IsVerified() {
+			return true
+		}
+	}
+
+	return false
+}
+
 func (u *User) UpdateBannedUntil(tx *storage.Connection) error {
 	return tx.UpdateOnly(u, "banned_until")
 }