Skip to content

Commit dfe62d0

Browse files
authored
fix(mfa): use AMR method (not factor type) when downgrading sessions to AAL1 (#2615)
`DowngradeSessionsToAAL1` deleted AMR claims where `authentication_method = factor_type`, but the column stores the AMR method (`mfa/phone`, `mfa/webauthn`) — only "totp" matched, so unenrolling a phone/webauthn factor left a stale AAL2 claim. This PR: - Maps factor type → AMR method before the delete - Unknown types now error instead of silently mismatching - Drops the dead "mfa/sms" alias
1 parent c4f6964 commit dfe62d0

2 files changed

Lines changed: 113 additions & 2 deletions

File tree

internal/models/factor.go

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,7 @@ func ParseAuthenticationMethod(authMethod string) (AuthenticationMethod, error)
135135
return EmailChange, nil
136136
case "token_refresh":
137137
return TokenRefresh, nil
138-
case "mfa/sms":
138+
case "mfa/phone":
139139
return MFAPhone, nil
140140
case "mfa/webauthn":
141141
return MFAWebAuthn, nil
@@ -397,13 +397,32 @@ func (f *Factor) UpdateStatus(tx *storage.Connection, state FactorState) error {
397397
return tx.UpdateOnly(f, "status", "updated_at")
398398
}
399399

400+
// amrMethodForFactorType returns the AMR authentication_method string stored in
401+
// mfa_amr_claims for a given factor type.
402+
func amrMethodForFactorType(factorType string) (string, error) {
403+
switch factorType {
404+
case TOTP:
405+
return TOTPSignIn.String(), nil
406+
case Phone:
407+
return MFAPhone.String(), nil
408+
case WebAuthn:
409+
return MFAWebAuthn.String(), nil
410+
default:
411+
return "", fmt.Errorf("no AMR authentication method mapped for factor type %q", factorType)
412+
}
413+
}
414+
400415
func (f *Factor) DowngradeSessionsToAAL1(tx *storage.Connection) error {
401416
sessions, err := FindSessionsByFactorID(tx, f.ID)
402417
if err != nil {
403418
return err
404419
}
420+
amrMethod, err := amrMethodForFactorType(f.FactorType)
421+
if err != nil {
422+
return err
423+
}
405424
for _, session := range sessions {
406-
if err := tx.RawQuery("DELETE FROM "+(&pop.Model{Value: AMRClaim{}}).TableName()+" WHERE session_id = ? AND authentication_method = ?", session.ID, f.FactorType).Exec(); err != nil {
425+
if err := tx.RawQuery("DELETE FROM "+(&pop.Model{Value: AMRClaim{}}).TableName()+" WHERE session_id = ? AND authentication_method = ?", session.ID, amrMethod).Exec(); err != nil {
407426
return err
408427
}
409428
}

internal/models/factor_test.go

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package models
22

33
import (
44
"encoding/json"
5+
"fmt"
56
"testing"
67

78
"github.com/gofrs/uuid"
@@ -31,6 +32,31 @@ func TestFactor(t *testing.T) {
3132
suite.Run(t, ts)
3233
}
3334

35+
// TestAMRMethodForFactorType pins the factor-type -> AMR method mapping.
36+
func TestAMRMethodForFactorType(t *testing.T) {
37+
for factorType, want := range map[string]AuthenticationMethod{
38+
TOTP: TOTPSignIn,
39+
Phone: MFAPhone,
40+
WebAuthn: MFAWebAuthn,
41+
} {
42+
got, err := amrMethodForFactorType(factorType)
43+
require.NoError(t, err, "factor type %q must map", factorType)
44+
require.Equal(t, want.String(), got)
45+
}
46+
47+
_, err := amrMethodForFactorType("not-a-real-factor-type")
48+
require.Error(t, err)
49+
}
50+
51+
// TestAuthenticationMethodRoundTrip guards the String() <-> ParseAuthenticationMethod symmetry.
52+
func TestAuthenticationMethodRoundTrip(t *testing.T) {
53+
for _, m := range []AuthenticationMethod{TOTPSignIn, MFAPhone, MFAWebAuthn} {
54+
parsed, err := ParseAuthenticationMethod(m.String())
55+
require.NoError(t, err, "method %q must round-trip", m.String())
56+
require.Equal(t, m, parsed)
57+
}
58+
}
59+
3460
func (ts *FactorTestSuite) SetupTest() {
3561
TruncateAll(ts.db)
3662
user, err := NewUser("", "agenericemail@gmail.com", "secret", "test", nil)
@@ -72,3 +98,69 @@ func (ts *FactorTestSuite) TestEncodedFactorDoesNotLeakSecret() {
7298
json.Unmarshal(encodedFactor, &decodedFactor)
7399
require.Equal(ts.T(), decodedFactor.Secret, "")
74100
}
101+
102+
// TestDowngradeSessionsToAAL1RemovesAMRClaim asserts that unenrolling a verified
103+
// factor strips the AMR claim it granted, dropping the session back to AAL1.
104+
func (ts *FactorTestSuite) TestDowngradeSessionsToAAL1RemovesAMRClaim() {
105+
cases := []struct {
106+
desc string
107+
newFactor func(u *User) *Factor
108+
authMethod AuthenticationMethod
109+
}{
110+
{
111+
desc: "phone",
112+
newFactor: func(u *User) *Factor { return NewPhoneFactor(u, "+15555555555", "") },
113+
authMethod: MFAPhone,
114+
},
115+
{
116+
desc: "webauthn",
117+
newFactor: func(u *User) *Factor { return NewWebAuthnFactor(u, "webauthnfactor") },
118+
authMethod: MFAWebAuthn,
119+
},
120+
{
121+
desc: "totp",
122+
newFactor: func(u *User) *Factor { return NewTOTPFactor(u, "totpfactor") },
123+
authMethod: TOTPSignIn,
124+
},
125+
}
126+
127+
for i, c := range cases {
128+
ts.Run(c.desc, func() {
129+
user, err := NewUser("", fmt.Sprintf("downgrade-%d@example.com", i), "secret", "test", nil)
130+
require.NoError(ts.T(), err)
131+
require.NoError(ts.T(), ts.db.Create(user))
132+
133+
factor := c.newFactor(user)
134+
require.NoError(ts.T(), factor.SetSecret("secretkey", false, "", ""))
135+
require.NoError(ts.T(), ts.db.Create(factor))
136+
require.NoError(ts.T(), factor.UpdateStatus(ts.db, FactorStateVerified))
137+
138+
session, err := NewSession(user.ID, &factor.ID)
139+
require.NoError(ts.T(), err)
140+
require.NoError(ts.T(), ts.db.Create(session))
141+
require.NoError(ts.T(), AddClaimToSession(ts.db, session.ID, c.authMethod))
142+
require.NoError(ts.T(), session.UpdateAALAndAssociatedFactor(ts.db, AAL2, &factor.ID))
143+
144+
// the claim upgrades the session to AAL2.
145+
loaded, err := FindSessionByID(ts.db, session.ID, false)
146+
require.NoError(ts.T(), err)
147+
aal, _, err := loaded.CalculateAALAndAMR(user)
148+
require.NoError(ts.T(), err)
149+
require.Equal(ts.T(), AAL2, aal)
150+
151+
require.NoError(ts.T(), factor.DowngradeSessionsToAAL1(ts.db))
152+
153+
downgraded, err := FindSessionByID(ts.db, session.ID, false)
154+
require.NoError(ts.T(), err)
155+
require.Equal(ts.T(), AAL1.String(), downgraded.GetAAL())
156+
require.Nil(ts.T(), downgraded.FactorID)
157+
158+
aal, _, err = downgraded.CalculateAALAndAMR(user)
159+
require.NoError(ts.T(), err)
160+
require.Equal(ts.T(), AAL1, aal)
161+
for _, claim := range downgraded.AMRClaims {
162+
require.NotEqual(ts.T(), c.authMethod.String(), claim.GetAuthenticationMethod())
163+
}
164+
})
165+
}
166+
}

0 commit comments

Comments
 (0)