-
Notifications
You must be signed in to change notification settings - Fork 38
feat: add MFA backup recovery codes #219
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| package models | ||
|
|
||
| import ( | ||
| "time" | ||
|
|
||
| "github.com/google/uuid" | ||
| "gorm.io/gorm" | ||
| ) | ||
|
|
||
| type BackupCode struct { | ||
| ID string `gorm:"type:uuid;primaryKey"` | ||
| UserID string `gorm:"type:uuid;not null;index"` | ||
| CodeHash string `gorm:"not null"` | ||
| Used bool `gorm:"default:false"` | ||
| CreatedAt time.Time | ||
| DeletedAt gorm.DeletedAt `gorm:"index"` | ||
| } | ||
|
|
||
| func (b *BackupCode) BeforeCreate(tx *gorm.DB) error { | ||
| if b.ID == "" { | ||
| b.ID = uuid.New().String() | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (BackupCode) TableName() string { | ||
| return "backup_codes" | ||
|
Comment on lines
+10
to
+27
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟠 Major 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Verify whether generated API docs include the new backup-code fields/endpoint contract.
fd -i 'swagger|openapi|docs' .
rg -n --hidden -S 'backupCode|backupCodes|/api/auth/login/mfa' docs internal || trueRepository: roshankumar0036singh/auth-server Length of output: 1766 🏁 Script executed: # Check the MFA handler for Swagger annotations
sed -n '900,1050p' internal/handler/auth_handler.goRepository: roshankumar0036singh/auth-server Length of output: 2967 🏁 Script executed: # Check if BackupCode model is documented in Swagger schema
rg -A 20 '"BackupCode"' docs/swagger.json || trueRepository: roshankumar0036singh/auth-server Length of output: 170 🏁 Script executed: # Check the MFA response structure in Swagger
rg -B 5 -A 15 'backupCodes' docs/swagger.json || trueRepository: roshankumar0036singh/auth-server Length of output: 170 🏁 Script executed: # Check the MFALoginResponse DTO structure
rg -A 10 'type MFALoginResponse' internal/dto/auth_dto.goRepository: roshankumar0036singh/auth-server Length of output: 170 🏁 Script executed: # Check what fields are in the MFA response DTO
sed -n '80,120p' internal/dto/auth_dto.goRepository: roshankumar0036singh/auth-server Length of output: 1497 🏁 Script executed: # Check the complete LoginResponse DTO structure
rg -A 20 'type LoginResponse struct' internal/dto/auth_dto.goRepository: roshankumar0036singh/auth-server Length of output: 1029 🏁 Script executed: # Check if there's a specific MFA login response DTO
rg -B 2 -A 10 'MFALogin.*Response' internal/dto/auth_dto.goRepository: roshankumar0036singh/auth-server Length of output: 170 🏁 Script executed: # Check if BackupCode model is documented in the generated Swagger schema
cat docs/swagger.json | grep -i 'backupcode\|backup_code' | head -20Repository: roshankumar0036singh/auth-server Length of output: 170 🏁 Script executed: # Check the actual LoginResponse definition in swagger.json
rg -B 3 -A 15 'LoginResponse' docs/swagger.json | head -50Repository: roshankumar0036singh/auth-server Length of output: 170 🏁 Script executed: # Check handler annotation more broadly around the MFA endpoints
rg -B 5 'LoginMFA' internal/handler/auth_handler.go | grep -E '@|func'Repository: roshankumar0036singh/auth-server Length of output: 395 🏁 Script executed: # Check what utils.Response structure looks like and how it's used in Swagger
rg -A 5 'type Response struct' internal/utils/ || rg -A 5 'type Response struct' . --type go | head -20Repository: roshankumar0036singh/auth-server Length of output: 538 🏁 Script executed: # Check if there are other handlers that document specific response structures
rg '`@Success`.*\{object\}' internal/handler/auth_handler.go | head -10Repository: roshankumar0036singh/auth-server Length of output: 570 🏁 Script executed: # Verify if LoginResponse should have BackupCodes field
rg -B 5 -A 15 'VerifyLoginMFA' internal/service/auth_service.go | head -50Repository: roshankumar0036singh/auth-server Length of output: 1135 🏁 Script executed: # Check when/how backup codes are returned - in enable MFA or login?
rg -B 5 -A 15 'BackupCode.*Create\|Generate.*Backup' internal/service/auth_service.go | head -60Repository: roshankumar0036singh/auth-server Length of output: 170 🏁 Script executed: # Check the MFA enable endpoint handler to see if it documents BackupCodes
rg -B 8 'EnableMFA' internal/handler/auth_handler.go | grep -E '@|func' | head -20Repository: roshankumar0036singh/auth-server Length of output: 377 🏁 Script executed: # Check if docs need to include BackupCode model schema
rg '"BackupCode"' docs/swagger.yamlRepository: roshankumar0036singh/auth-server Length of output: 170 🏁 Script executed: # Check if MFAEnableResponse is referenced in any Swagger annotation
rg 'MFAEnableResponse|MFALoginResponse' internal/handler/auth_handler.goRepository: roshankumar0036singh/auth-server Length of output: 170 🏁 Script executed: # Let's see the full EnableMFA and GenerateBackupCodes handlers
sed -n '850,950p' internal/handler/auth_handler.goRepository: roshankumar0036singh/auth-server Length of output: 3321 🏁 Script executed: # Check the service method to confirm what it returns
rg -B 3 -A 20 'func.*GenerateBackupCodes' internal/service/auth_service.goRepository: roshankumar0036singh/auth-server Length of output: 170 🏁 Script executed: # Check what EnableMFA service returns
rg -B 2 -A 20 'func.*EnableMFA' internal/service/auth_service.go | head -40Repository: roshankumar0036singh/auth-server Length of output: 1137 🏁 Script executed: # Check if swagger.json has any response definitions for MFA endpoints
rg -A 30 '"/api/auth/mfa/enable"' docs/swagger.jsonRepository: roshankumar0036singh/auth-server Length of output: 1050 🏁 Script executed: # Confirm the actual return type of EnableMFA
ast-grep outline internal/service/auth_service.go --match 'EnableMFA' --view expandedRepository: roshankumar0036singh/auth-server Length of output: 361 🏁 Script executed: # Check if BackupCode model has any Swagger annotations
cat internal/models/backup_code.goRepository: roshankumar0036singh/auth-server Length of output: 730 🏁 Script executed: # Verify the complete definitions section in swagger.json
rg '"definitions":|"components":' docs/swagger.json -A 5Repository: roshankumar0036singh/auth-server Length of output: 351 🏁 Script executed: # Check if there are Swagger struct tags on BackupCode model
sed -n '1,50p' internal/models/backup_code.goRepository: roshankumar0036singh/auth-server Length of output: 730 🏁 Script executed: # Check if other models have Swagger documentation/struct tags
rg 'swagger:' internal/models/ | head -10Repository: roshankumar0036singh/auth-server Length of output: 170 🏁 Script executed: # Check if MFAEnableResponse is in the swagger definitions
rg '"MFAEnableResponse"' docs/swagger.jsonRepository: roshankumar0036singh/auth-server Length of output: 170 🏁 Script executed: # Check what the Makefile target 'swagger' does
cat Makefile | grep -A 10 'swagger'Repository: roshankumar0036singh/auth-server Length of output: 572 Update Swagger annotations in MFA handlers to reference specific response DTOs, then run The 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,38 @@ | ||||||||||||||||||||||||||||||||||
| package repository | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||||||||||||
| "errors" | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| "github.com/roshankumar0036singh/auth-server/internal/models" | ||||||||||||||||||||||||||||||||||
| "gorm.io/gorm" | ||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| var ErrBackupCodeNotFound = errors.New("backup code not found") | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| type BackupCodeRepository struct { | ||||||||||||||||||||||||||||||||||
| db *gorm.DB | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| func NewBackupCodeRepository(db *gorm.DB) *BackupCodeRepository { | ||||||||||||||||||||||||||||||||||
| return &BackupCodeRepository{db: db} | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| func (r *BackupCodeRepository) Create(code *models.BackupCode) error { | ||||||||||||||||||||||||||||||||||
| return r.db.Create(code).Error | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| func (r *BackupCodeRepository) FindByUserID(userID string) ([]models.BackupCode, error) { | ||||||||||||||||||||||||||||||||||
| var codes []models.BackupCode | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| err := r.db. | ||||||||||||||||||||||||||||||||||
| Where("user_id = ? AND used = ?", userID, false). | ||||||||||||||||||||||||||||||||||
| Find(&codes).Error | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| return codes, err | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| func (r *BackupCodeRepository) MarkUsed(id string) error { | ||||||||||||||||||||||||||||||||||
| return r.db.Model(&models.BackupCode{}). | ||||||||||||||||||||||||||||||||||
| Where("id = ?", id). | ||||||||||||||||||||||||||||||||||
| Update("used", true).Error | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+34
to
+37
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟠 Major 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Verify MarkUsed behavior and all call sites.
rg -n --type=go 'func \(r \*BackupCodeRepository\) MarkUsed|ErrBackupCodeNotFound|MarkUsed\(' internal/repository internal/serviceRepository: roshankumar0036singh/auth-server Length of output: 469 🏁 Script executed: #!/bin/bash
# Get the full BackupCodeRepository implementation
cat -n internal/repository/backup_code_repository.goRepository: roshankumar0036singh/auth-server Length of output: 1273 🏁 Script executed: #!/bin/bash
# Check the call site in auth_service.go around line 465
sed -n '450,480p' internal/service/auth_service.go | cat -nRepository: roshankumar0036singh/auth-server Length of output: 1096 🏁 Script executed: #!/bin/bash
# Check the BackupCode model structure
find . -name "*.go" -type f | xargs grep -l "type BackupCode" | head -5Repository: roshankumar0036singh/auth-server Length of output: 251 🏁 Script executed: #!/bin/bash
# Check the BackupCode model structure
cat -n internal/models/backup_code.goRepository: roshankumar0036singh/auth-server Length of output: 926 🏁 Script executed: #!/bin/bash
# Get more context around the call site to understand the full flow
sed -n '440,490p' internal/service/auth_service.go | cat -nRepository: roshankumar0036singh/auth-server Length of output: 1840 Make backup-code consumption atomic in Concurrent requests can both pass verification and reuse a one-time backup code. The current implementation updates by Add an Suggested fix func (r *BackupCodeRepository) MarkUsed(id string) error {
- return r.db.Model(&models.BackupCode{}).
- Where("id = ?", id).
- Update("used", true).Error
+ tx := r.db.Model(&models.BackupCode{}).
+ Where("id = ? AND used = ?", id, false).
+ Update("used", true)
+ if tx.Error != nil {
+ return tx.Error
+ }
+ if tx.RowsAffected == 0 {
+ return ErrBackupCodeNotFound
+ }
+ return nil
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,6 +48,7 @@ | |
| auditService *AuditService | ||
| mfaService *MFAService | ||
| config *config.Config | ||
| backupCodeRepo *repository.BackupCodeRepository | ||
| } | ||
|
|
||
| func NewAuthService( | ||
|
|
@@ -61,6 +62,7 @@ | |
| auditService *AuditService, | ||
| mfaService *MFAService, | ||
| cfg *config.Config, | ||
| backupCodeRepo *repository.BackupCodeRepository, | ||
| ) *AuthService { | ||
| return &AuthService{ | ||
| userRepo: userRepo, | ||
|
|
@@ -73,6 +75,7 @@ | |
| auditService: auditService, | ||
| mfaService: mfaService, | ||
| config: cfg, | ||
| backupCodeRepo: backupCodeRepo, | ||
| } | ||
| } | ||
| func (s *AuthService) getRefreshTokenExpiry() time.Duration { | ||
|
|
@@ -346,6 +349,27 @@ | |
| if !s.mfaService.ValidateMFA(user.MFASecret, code) { | ||
| return ErrInvalidMFACode | ||
| } | ||
| for i := 0; i < 10; i++ { | ||
| code := s.tokenService.GenerateRandomString(8) | ||
|
|
||
| hash, err := bcrypt.GenerateFromPassword( | ||
| []byte(code), | ||
| bcrypt.DefaultCost, | ||
| ) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| backup := &models.BackupCode{ | ||
| UserID: userID, | ||
| CodeHash: string(hash), | ||
| Used: false, | ||
| } | ||
|
|
||
| if err := s.backupCodeRepo.Create(backup); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
Comment on lines
+352
to
+372
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift Generated backup codes are discarded instead of being returned once. Line 352-Line 372 creates plaintext backup codes and only stores hashes, but this method returns Suggested direction-func (s *AuthService) VerifyEnableMFA(userID, code string) error {
+func (s *AuthService) VerifyEnableMFA(userID, code string) ([]string, error) {
+ backupCodes := make([]string, 0, 10)
...
for i := 0; i < 10; i++ {
code := s.tokenService.GenerateRandomString(8)
+ backupCodes = append(backupCodes, code)
...
- return err
+ return nil, err
}
}
...
- return nil
+ return backupCodes, nil
}🤖 Prompt for AI Agents |
||
|
|
||
| // Enable MFA | ||
| if err := s.userRepo.Update(userID, map[string]interface{}{ | ||
|
|
@@ -399,7 +423,7 @@ | |
| // short-lived MFA-pending token issued by the password step (Login), so the | ||
| // password cannot be bypassed, and rate-limits code attempts to prevent | ||
| // brute-forcing the 6-digit TOTP. | ||
| func (s *AuthService) VerifyLoginMFA(mfaToken, code, ipAddress, userAgent string) (*dto.LoginResponse, error) { | ||
|
Check failure on line 426 in internal/service/auth_service.go
|
||
| ctx := context.Background() | ||
|
|
||
| userID, err := s.tokenService.ValidateMFAToken(mfaToken) | ||
|
|
@@ -425,13 +449,33 @@ | |
| return nil, errors.New("MFA not enabled for this user") | ||
| } | ||
|
|
||
| if !s.mfaService.ValidateMFA(user.MFASecret, code) { | ||
| s.cacheService.IncrementMFAAttempts(ctx, userID) | ||
| if err := s.auditService.LogEvent(&user.ID, "MFA_LOGIN_FAILED", "USER", user.ID, ipAddress, userAgent, nil); err != nil { | ||
| log.Printf("failed to write MFA_LOGIN_FAILED audit log for user %s: %v", user.ID, err) | ||
| valid := s.mfaService.ValidateMFA(user.MFASecret, code) | ||
|
|
||
| if !valid { | ||
| codes, err := s.backupCodeRepo.FindByUserID(userID) | ||
|
|
||
| if err == nil { | ||
| for _, bc := range codes { | ||
| if bcrypt.CompareHashAndPassword( | ||
| []byte(bc.CodeHash), | ||
| []byte(code), | ||
| ) == nil { | ||
|
|
||
| valid = true | ||
| _ = s.backupCodeRepo.MarkUsed(bc.ID) | ||
| break | ||
|
Comment on lines
+454
to
+466
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Only mark MFA valid after backup-code consumption succeeds. Line 464 sets Suggested fix- valid = true
- _ = s.backupCodeRepo.MarkUsed(bc.ID)
- break
+ if err := s.backupCodeRepo.MarkUsed(bc.ID); err == nil {
+ valid = true
+ break
+ }🤖 Prompt for AI Agents |
||
| } | ||
| } | ||
| return nil, ErrInvalidMFACode | ||
| } | ||
| } | ||
|
|
||
| if !valid { | ||
| s.cacheService.IncrementMFAAttempts(ctx, userID) | ||
| if err := s.auditService.LogEvent(&user.ID, "MFA_LOGIN_FAILED", "USER", user.ID, ipAddress, userAgent, nil); err != nil { | ||
| log.Printf("failed to write MFA_LOGIN_FAILED audit log for user %s: %v", user.ID, err) | ||
| } | ||
| return nil, ErrInvalidMFACode | ||
| } | ||
|
|
||
| s.cacheService.ResetMFAAttempts(ctx, userID) | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
Repository: roshankumar0036singh/auth-server
Length of output: 2119
🏁 Script executed:
Repository: roshankumar0036singh/auth-server
Length of output: 835
🏁 Script executed:
Repository: roshankumar0036singh/auth-server
Length of output: 726
🏁 Script executed:
Repository: roshankumar0036singh/auth-server
Length of output: 1304
🏁 Script executed:
Repository: roshankumar0036singh/auth-server
Length of output: 2355
Codefield blocks backup-code-only authentication requests.The
Codefield hasbinding:"required", so requests with onlybackupCodecannot pass JSON validation. Additionally, the handler doesn't passreq.BackupCodetoVerifyLoginMFA, and the service signature doesn't accept a backup code parameter—currently, backup codes are only checked as a fallback when TOTP validation fails.To support backup-code-only authentication as an alternative MFA method:
Required changes
type MFALoginRequest struct { MFAToken string `json:"mfaToken" binding:"required"` - Code string `json:"code" binding:"required,len=6"` + Code string `json:"code,omitempty" binding:"omitempty,len=6"` BackupCode string `json:"backupCode,omitempty"` }Then update the handler to pass
req.BackupCodeto the service, and updateVerifyLoginMFAto accept and handle the backup code parameter, enforcing that exactly one ofcodeorbackupCodeis provided.🤖 Prompt for AI Agents