Merge pull request #595 from supabase/j0_minor_mfa_fixes

Minor MFA fixes
supabase · Aug 8, 2022 · ca67dcf · ca67dcf
2 parents 9cf6fac + a4af59f
commit ca67dcf
Show file tree

Hide file tree

Showing 2 changed files with 21 additions and 5 deletions.
diff --git a/api/mfa.go b/api/mfa.go
@@ -41,7 +41,6 @@ type VerifyFactorParams struct {
 type ChallengeFactorResponse struct {
 	ID        string `json:"id"`
 	CreatedAt string `json:"created_at"`
-	UpdatedAt string `json:"updated_at"`
 	ExpiresAt string `json:"expires_at"`
 }
 
@@ -73,6 +72,9 @@ func (a *API) EnrollFactor(w http.ResponseWriter, r *http.Request) error {
 	if (params.FactorType != "totp") && (params.FactorType != "webauthn") {
 		return unprocessableEntityError("FactorType needs to be either 'totp' or 'webauthn'")
 	}
+	if params.Issuer == "" {
+		return unprocessableEntityError("Issuer is required")
+	}
 	// TODO(Joel): Review this portion when email is no longer a primary key
 	key, err := totp.Generate(totp.GenerateOpts{
 		Issuer:      params.Issuer,
@@ -190,7 +192,7 @@ func (a *API) VerifyFactor(w http.ResponseWriter, r *http.Request) error {
 	err = a.db.Transaction(func(tx *storage.Connection) error {
 		if err = models.NewAuditLogEntry(tx, instanceID, user, models.VerifyFactorAction, r.RemoteAddr, map[string]interface{}{
 			"factor_id":    factor.ID,
-			"challenge_id": params.ChallengeID,
+			"challenge_id": challenge.ID,
 		}); err != nil {
 			return err
 		}

diff --git a/api/mfa_test.go b/api/mfa_test.go
@@ -56,7 +56,7 @@ func (ts *MFATestSuite) TestEnrollFactor() {
 		FriendlyName string
 		FactorType   string
 		Issuer       string
-		expectedCode int
+		ExpectedCode int
 	}{
 		{
 			"TOTP: Factor has friendly name",
@@ -72,6 +72,20 @@ func (ts *MFATestSuite) TestEnrollFactor() {
 			"supabase.com",
 			http.StatusOK,
 		},
+		{
+			"TOTP: No issuer",
+			"john",
+			"totp",
+			"",
+			http.StatusUnprocessableEntity,
+		},
+		{
+			"Invalid factor type",
+			"bob",
+			"",
+			"john.com",
+			http.StatusUnprocessableEntity,
+		},
 	}
 	for _, c := range cases {
 		ts.Run(c.desc, func() {
@@ -88,13 +102,13 @@ func (ts *MFATestSuite) TestEnrollFactor() {
 			req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token))
 			req.Header.Set("Content-Type", "application/json")
 			ts.API.handler.ServeHTTP(w, req)
-			require.Equal(ts.T(), http.StatusOK, w.Code)
+			require.Equal(ts.T(), c.ExpectedCode, w.Code)
 
 			factors, err := models.FindFactorsByUser(ts.API.db, user)
 			ts.Require().NoError(err)
 			latestFactor := factors[len(factors)-1]
 			require.Equal(ts.T(), models.FactorDisabledState, latestFactor.Status)
-			if c.FriendlyName != "" {
+			if c.FriendlyName != "" && c.ExpectedCode == http.StatusOK {
 				require.Equal(ts.T(), c.FriendlyName, latestFactor.FriendlyName)
 			}
 		})