Skip to content

Commit

Permalink
fix: expose factor type on challenge (#1709)
Browse files Browse the repository at this point in the history
## What kind of change does this PR introduce?

Expose factor type on challenge so that developers can know whether it
is a phone or TOTP factor they have challenged and redirect to
appropriate page or perform relevant action

Also helps us distinguish between the two so we can prevent unintended
behaviour (e.g. the use of `challengeAndVerify` with a phone factor)


As typescript follow structural typing I think addition of a new field
should be fine. We will test this before proceeding. However, putting it
out there first for early review or consideration

Update - tested and should be fine
  • Loading branch information
J0 committed Aug 5, 2024
1 parent 29cbeb7 commit e1a21a3
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 1 deletion.
3 changes: 3 additions & 0 deletions internal/api/mfa.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ type VerifyFactorParams struct {

type ChallengeFactorResponse struct {
ID uuid.UUID `json:"id"`
Type string `json:"type"`
ExpiresAt int64 `json:"expires_at"`
}

Expand Down Expand Up @@ -363,6 +364,7 @@ func (a *API) challengePhoneFactor(w http.ResponseWriter, r *http.Request) error
}
return sendJSON(w, http.StatusOK, &ChallengeFactorResponse{
ID: challenge.ID,
Type: factor.FactorType,
ExpiresAt: challenge.GetExpiryTime(config.MFA.ChallengeExpiryDuration).Unix(),
})
}
Expand Down Expand Up @@ -395,6 +397,7 @@ func (a *API) challengeTOTPFactor(w http.ResponseWriter, r *http.Request) error

return sendJSON(w, http.StatusOK, &ChallengeFactorResponse{
ID: challenge.ID,
Type: factor.FactorType,
ExpiresAt: challenge.GetExpiryTime(config.MFA.ChallengeExpiryDuration).Unix(),
})
}
Expand Down
11 changes: 10 additions & 1 deletion internal/api/mfa_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -322,11 +322,17 @@ func (ts *MFATestSuite) TestMultipleEnrollsCleanupExpiredFactors() {
require.Equal(ts.T(), 3, len(ts.TestUser.Factors))
}

func (ts *MFATestSuite) TestChallengeFactor() {
func (ts *MFATestSuite) TestChallengeTOTPFactor() {
// Test Factor is a TOTP Factor
f := ts.TestUser.Factors[0]
token := ts.generateAAL1Token(ts.TestUser, &ts.TestSession.ID)
w := performChallengeFlow(ts, f.ID, token)
challengeResp := ChallengeFactorResponse{}
require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&challengeResp))

require.Equal(ts.T(), http.StatusOK, w.Code)
require.Equal(ts.T(), challengeResp.Type, models.TOTP)

}

func (ts *MFATestSuite) TestChallengeSMSFactor() {
Expand Down Expand Up @@ -372,6 +378,9 @@ func (ts *MFATestSuite) TestChallengeSMSFactor() {
for _, tc := range cases {
ts.Run(tc.desc, func() {
w := performSMSChallengeFlow(ts, f.ID, token, tc.channel)
challengeResp := ChallengeFactorResponse{}
require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&challengeResp))
require.Equal(ts.T(), challengeResp.Type, models.Phone)
require.Equal(ts.T(), tc.expectedCode, w.Code, tc.desc)
})
}
Expand Down

0 comments on commit e1a21a3

Please sign in to comment.