From a01e6517097862a09ac5c268bb1e40c76e701b7e Mon Sep 17 00:00:00 2001 From: efrilm Date: Fri, 9 Oct 2026 22:59:27 +0700 Subject: [PATCH] fix(customer-auth): refuse a wrong login with 304 and limit attempts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A wrong password or an unknown phone number was answered 900 (HTTP 500), like a server error, so clients could only tell them apart by the cause text. Both are now 304 (HTTP 400) from customer_auth_service with one cause, "invalid phone number or password", so a login does not tell which numbers have an account. A customer who never set a password gets 304 "customer not properly registered". Anything else stays 900. Login is limited per phone number: 5 attempts in 15 minutes, counted in Redis before the password is checked, so attempts sent at once all count, and for numbers without a customer too. The sixth is refused with 429 and data.locked_until, even with the right password, until the window ends. A successful login starts the count again. Since the number is normalized first, 0812… and 62812… count as one. When Redis fails, logins go on unlimited and the error is logged. There is no limit per IP: the client IP comes from X-Forwarded-For, which anyone can set while no trusted proxies are configured. Tested over HTTP with fakes; the Redis commands were checked against miniredis outside the repo, not against a real Redis. Co-Authored-By: Claude Opus 5.5 --- internal/app/app.go | 4 +- internal/constants/error.go | 1 + internal/handler/customer_auth_handler.go | 28 +- .../handler/customer_auth_handler_test.go | 245 ++++++++++++++++++ internal/processor/customer_auth_processor.go | 76 +++++- .../customer_login_attempt_repository.go | 51 ++++ 6 files changed, 389 insertions(+), 16 deletions(-) create mode 100644 internal/handler/customer_auth_handler_test.go create mode 100644 internal/repository/customer_login_attempt_repository.go diff --git a/internal/app/app.go b/internal/app/app.go index 8df41ce..af88462 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -307,6 +307,7 @@ type repositories struct { campaignRepo repository.CampaignRepository campaignRuleRepo repository.CampaignRuleRepository customerAuthRepo repository.CustomerAuthRepository + customerLoginAttemptRepo repository.CustomerLoginAttemptRepository otpRepo repository.OtpRepository sessionRepo repository.SessionRepository txManager *repository.TxManager @@ -359,6 +360,7 @@ func (a *App) initRepositories() *repositories { campaignRepo: repository.NewCampaignRepository(a.db), campaignRuleRepo: repository.NewCampaignRuleRepository(a.db), customerAuthRepo: repository.NewCustomerAuthRepository(a.db), + customerLoginAttemptRepo: repository.NewCustomerLoginAttemptRepository(a.redisClient), otpRepo: repository.NewOtpRepository(a.db), sessionRepo: repository.NewSessionRepository(a.redisClient), txManager: repository.NewTxManager(a.db), @@ -488,7 +490,7 @@ func (a *App) initProcessors(cfg *config.Config, repos *repositories) *processor omsetTrackerProcessor: processor.NewOmsetTrackerProcessor(repos.omsetTrackerRepo), campaignProcessor: processor.NewCampaignProcessor(repos.campaignRepo), campaignRuleProcessor: processor.NewCampaignRuleProcessor(repos.campaignRuleRepo), - customerAuthProcessor: processor.NewCustomerAuthProcessor(repos.customerAuthRepo, otpProcessor, repos.otpRepo, cfg.GetCustomerJWTSecret(), cfg.GetCustomerJWTExpiresTTL()), + customerAuthProcessor: processor.NewCustomerAuthProcessor(repos.customerAuthRepo, repos.customerLoginAttemptRepo, otpProcessor, repos.otpRepo, cfg.GetCustomerJWTSecret(), cfg.GetCustomerJWTExpiresTTL()), customerPointsProcessor: processor.NewCustomerPointsProcessor(processor.NewWalletQueryProcessor(repos.walletQueryRepo, processor.NewLoyaltySettingsProcessor(repos.loyaltySettingsRepo, repos.txManager))), otpProcessor: otpProcessor, fileClient: fileClient, diff --git a/internal/constants/error.go b/internal/constants/error.go index 05193ea..b7e52ec 100644 --- a/internal/constants/error.go +++ b/internal/constants/error.go @@ -67,6 +67,7 @@ const ( EnakGameServiceEntity = "enakgame_service" LoyaltySettingsServiceEntity = "loyalty_settings_service" CustomerPinServiceEntity = "customer_pin_service" + CustomerAuthServiceEntity = "customer_auth_service" ) var HttpErrorMap = map[string]int{ diff --git a/internal/handler/customer_auth_handler.go b/internal/handler/customer_auth_handler.go index 126d4bc..e748f9d 100644 --- a/internal/handler/customer_auth_handler.go +++ b/internal/handler/customer_auth_handler.go @@ -1,9 +1,12 @@ package handler import ( + "errors" + "apskel-pos-be/internal/constants" "apskel-pos-be/internal/contract" "apskel-pos-be/internal/logger" + "apskel-pos-be/internal/processor" "apskel-pos-be/internal/service" "apskel-pos-be/internal/util" "apskel-pos-be/internal/validator" @@ -161,13 +164,36 @@ func (h *CustomerAuthHandler) Login(c *gin.Context) { response, err := h.customerAuthService.Login(ctx, &req) if err != nil { logger.FromContext(c.Request.Context()).WithError(err).Error("CustomerAuthHandler::Login -> service call failed") - util.HandleResponse(c.Writer, c.Request, contract.BuildErrorResponse([]*contract.ResponseError{contract.NewResponseError(constants.InternalServerErrorCode, constants.RequestEntity, err.Error())}), "CustomerAuthHandler::Login") + util.HandleResponse(c.Writer, c.Request, loginErrorResponse(err), "CustomerAuthHandler::Login") return } util.HandleResponse(c.Writer, c.Request, contract.BuildSuccessResponse(response), "CustomerAuthHandler::Login") } +// loginErrorResponse answers a phone number with too many attempts with 429 and when it +// may try again, a refused login with 304 and its reason, and anything else with 900. +func loginErrorResponse(err error) *contract.Response { + var locked *processor.CustomerLoginLockedError + if errors.As(err, &locked) { + return &contract.Response{ + Success: false, + Data: map[string]interface{}{"locked_until": locked.Until}, + Errors: []*contract.ResponseError{contract.NewResponseError(constants.TooManyRequestsErrorCode, constants.CustomerAuthServiceEntity, locked.Error())}, + } + } + for _, refused := range []error{processor.ErrCustomerLoginInvalid, processor.ErrCustomerNotRegistered} { + if errors.Is(err, refused) { + return contract.BuildErrorResponse([]*contract.ResponseError{ + contract.NewResponseError(constants.ValidationErrorCode, constants.CustomerAuthServiceEntity, refused.Error()), + }) + } + } + return contract.BuildErrorResponse([]*contract.ResponseError{ + contract.NewResponseError(constants.InternalServerErrorCode, constants.RequestEntity, err.Error()), + }) +} + func (h *CustomerAuthHandler) ResendOtp(c *gin.Context) { ctx := c.Request.Context() diff --git a/internal/handler/customer_auth_handler_test.go b/internal/handler/customer_auth_handler_test.go new file mode 100644 index 0000000..206a9c8 --- /dev/null +++ b/internal/handler/customer_auth_handler_test.go @@ -0,0 +1,245 @@ +package handler + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/gin-gonic/gin" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/crypto/bcrypt" + + "apskel-pos-be/internal/entities" + applogger "apskel-pos-be/internal/logger" + "apskel-pos-be/internal/processor" + "apskel-pos-be/internal/service" + "apskel-pos-be/internal/validator" +) + +// customerAuthRepoFake holds customers by phone number. Only the login lookup is used. +type customerAuthRepoFake struct { + customers map[string]*entities.Customer + err error +} + +func (f *customerAuthRepoFake) GetCustomerByPhoneNumber(_ context.Context, phone string) (*entities.Customer, error) { + if f.err != nil { + return nil, f.err + } + return f.customers[phone], nil +} + +func (f *customerAuthRepoFake) GetCustomerByID(context.Context, string) (*entities.Customer, error) { + return nil, nil +} +func (f *customerAuthRepoFake) CreateCustomer(context.Context, *entities.Customer) error { return nil } +func (f *customerAuthRepoFake) UpdateCustomer(context.Context, *entities.Customer) error { return nil } +func (f *customerAuthRepoFake) CheckPhoneNumberExists(context.Context, string) (bool, error) { + return false, nil +} +func (f *customerAuthRepoFake) SetCustomerPassword(context.Context, string, string) error { + return nil +} +func (f *customerAuthRepoFake) OrganizationExists(context.Context, uuid.UUID) (bool, error) { + return false, nil +} +func (f *customerAuthRepoFake) OrganizationIDs(context.Context, int) ([]uuid.UUID, error) { + return nil, nil +} + +// loginAttemptsFake counts attempts per phone number in one window that never ends. +type loginAttemptsFake struct { + counts map[string]int64 + err error +} + +func (f *loginAttemptsFake) Hit(_ context.Context, phone string, window time.Duration) (int64, time.Duration, error) { + if f.err != nil { + return 0, 0, f.err + } + f.counts[phone]++ + return f.counts[phone], window, nil +} + +func (f *loginAttemptsFake) Reset(_ context.Context, phone string) error { + delete(f.counts, phone) + return nil +} + +const ( + loginTestPassword = "rahasia123" + loginTestRegistered = "6281234561234" + loginTestUnfinished = "6281234569999" +) + +type loginTest struct { + repo *customerAuthRepoFake + attempts *loginAttemptsFake + router *gin.Engine +} + +func newLoginTest(t *testing.T) *loginTest { + t.Helper() + applogger.Setup("fatal", "json") + hash, err := bcrypt.GenerateFromPassword([]byte(loginTestPassword), bcrypt.MinCost) + require.NoError(t, err) + hashStr := string(hash) + registered, unfinished := loginTestRegistered, loginTestUnfinished + birth := time.Date(2000, 1, 31, 0, 0, 0, 0, time.UTC) + lt := &loginTest{ + repo: &customerAuthRepoFake{customers: map[string]*entities.Customer{ + registered: {ID: uuid.New(), Name: "Budi", PhoneNumber: ®istered, BirthDate: &birth, PasswordHash: &hashStr}, + unfinished: {ID: uuid.New(), Name: "Sari", PhoneNumber: &unfinished}, + }}, + attempts: &loginAttemptsFake{counts: map[string]int64{}}, + } + h := NewCustomerAuthHandler( + service.NewCustomerAuthService(processor.NewCustomerAuthProcessor(lt.repo, lt.attempts, nil, nil, "test", 60)), + validator.NewCustomerAuthValidator(), + ) + gin.SetMode(gin.TestMode) + lt.router = gin.New() + lt.router.POST("/customer-auth/login", h.Login) + return lt +} + +func (lt *loginTest) login(t *testing.T, phone, password string) (int, map[string]any) { + t.Helper() + body, _ := json.Marshal(map[string]string{"phone_number": phone, "password": password}) + rec := httptest.NewRecorder() + lt.router.ServeHTTP(rec, httptest.NewRequest(http.MethodPost, "/customer-auth/login", bytes.NewReader(body))) + var out map[string]any + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &out)) + return rec.Code, out +} + +func firstLoginError(t *testing.T, out map[string]any) map[string]any { + t.Helper() + errs, ok := out["errors"].([]any) + require.True(t, ok, "errors: %v", out) + require.NotEmpty(t, errs) + return errs[0].(map[string]any) +} + +func TestCustomerLoginRefusalsAreValidationErrors(t *testing.T) { + lt := newLoginTest(t) + + for name, tc := range map[string]struct{ phone, password, cause string }{ + "wrong password": {loginTestRegistered, "salah", "invalid phone number or password"}, + "unknown phone": {"6280000000000", loginTestPassword, "invalid phone number or password"}, + "registration unfinished": {loginTestUnfinished, loginTestPassword, "customer not properly registered"}, + } { + t.Run(name, func(t *testing.T) { + status, out := lt.login(t, tc.phone, tc.password) + assert.Equal(t, http.StatusBadRequest, status) + e := firstLoginError(t, out) + assert.Equal(t, "304", e["code"]) + assert.Equal(t, "customer_auth_service", e["entity"]) + assert.Equal(t, tc.cause, e["cause"]) + }) + } + + t.Run("right password", func(t *testing.T) { + status, out := lt.login(t, loginTestRegistered, loginTestPassword) + require.Equal(t, http.StatusOK, status, "%v", out) + inner := out["data"].(map[string]any)["data"].(map[string]any) + assert.NotEmpty(t, inner["access_token"]) + }) + + t.Run("the number written another way finds the same customer", func(t *testing.T) { + for _, phone := range []string{"081234561234", "+62 812-3456-1234", "81234561234"} { + status, out := lt.login(t, phone, loginTestPassword) + require.Equal(t, http.StatusOK, status, "%s: %v", phone, out) + user := out["data"].(map[string]any)["data"].(map[string]any)["user"].(map[string]any) + assert.Equal(t, loginTestRegistered, user["phone_number"], phone) + } + }) + + t.Run("a number that is not an Indonesian mobile number", func(t *testing.T) { + status, out := lt.login(t, "021-1234567", loginTestPassword) + assert.Equal(t, http.StatusBadRequest, status) + e := firstLoginError(t, out) + assert.Equal(t, "304", e["code"]) + assert.Equal(t, "invalid phone number format", e["cause"]) + }) + + t.Run("a failing lookup is still a server error", func(t *testing.T) { + lt.repo.err = errors.New("connection refused") + defer func() { lt.repo.err = nil }() + status, out := lt.login(t, loginTestRegistered, loginTestPassword) + assert.Equal(t, http.StatusInternalServerError, status) + assert.Equal(t, "900", firstLoginError(t, out)["code"]) + }) +} + +func TestCustomerLoginLocksAfterTooManyAttempts(t *testing.T) { + t.Run("the sixth attempt is refused, even with the right password", func(t *testing.T) { + lt := newLoginTest(t) + for i := 0; i < 5; i++ { + status, _ := lt.login(t, loginTestRegistered, "salah") + require.Equal(t, http.StatusBadRequest, status, "attempt %d", i+1) + } + status, out := lt.login(t, loginTestRegistered, loginTestPassword) + assert.Equal(t, http.StatusTooManyRequests, status) + e := firstLoginError(t, out) + assert.Equal(t, "429", e["code"]) + assert.Equal(t, "customer_auth_service", e["entity"]) + data, ok := out["data"].(map[string]any) + require.True(t, ok, "data: %v", out) + until, err := time.Parse(time.RFC3339, data["locked_until"].(string)) + require.NoError(t, err) + assert.WithinDuration(t, time.Now().Add(15*time.Minute), until, time.Minute) + + // Another number is not affected. + status, _ = lt.login(t, loginTestUnfinished, loginTestPassword) + assert.Equal(t, http.StatusBadRequest, status) + }) + + t.Run("writing the number another way counts toward the same limit", func(t *testing.T) { + lt := newLoginTest(t) + for _, phone := range []string{"081234561234", "+6281234561234", "81234561234", "0812-3456-1234", loginTestRegistered} { + lt.login(t, phone, "salah") + } + status, _ := lt.login(t, "081234561234", loginTestPassword) + assert.Equal(t, http.StatusTooManyRequests, status) + }) + + t.Run("unknown numbers lock the same way", func(t *testing.T) { + lt := newLoginTest(t) + for i := 0; i < 5; i++ { + lt.login(t, "6280000000000", "salah") + } + status, _ := lt.login(t, "6280000000000", "salah") + assert.Equal(t, http.StatusTooManyRequests, status) + }) + + t.Run("a successful login starts the count again", func(t *testing.T) { + lt := newLoginTest(t) + for i := 0; i < 4; i++ { + lt.login(t, loginTestRegistered, "salah") + } + status, _ := lt.login(t, loginTestRegistered, loginTestPassword) + require.Equal(t, http.StatusOK, status) + for i := 0; i < 5; i++ { + status, _ := lt.login(t, loginTestRegistered, "salah") + require.Equal(t, http.StatusBadRequest, status, "attempt %d after the login", i+1) + } + }) + + t.Run("logins go on when the counter is down", func(t *testing.T) { + lt := newLoginTest(t) + lt.attempts.err = errors.New("redis: connection refused") + for i := 0; i < 6; i++ { + lt.login(t, loginTestRegistered, "salah") + } + status, _ := lt.login(t, loginTestRegistered, loginTestPassword) + assert.Equal(t, http.StatusOK, status) + }) +} diff --git a/internal/processor/customer_auth_processor.go b/internal/processor/customer_auth_processor.go index 02b59f9..2fc254b 100644 --- a/internal/processor/customer_auth_processor.go +++ b/internal/processor/customer_auth_processor.go @@ -2,12 +2,14 @@ package processor import ( "context" + "errors" "fmt" "strings" "time" "apskel-pos-be/internal/contract" "apskel-pos-be/internal/entities" + "apskel-pos-be/internal/logger" "apskel-pos-be/internal/models" "apskel-pos-be/internal/repository" "apskel-pos-be/internal/util" @@ -16,6 +18,32 @@ import ( "golang.org/x/crypto/bcrypt" ) +var ( + // ErrCustomerLoginInvalid means no customer has the phone number or the password is + // wrong. Which of the two is not told. + ErrCustomerLoginInvalid = errors.New("invalid phone number or password") + // ErrCustomerNotRegistered means the customer never set a password: registration + // stopped before its last step. + ErrCustomerNotRegistered = errors.New("customer not properly registered") +) + +// Login attempts a phone number may make before it has to wait, and the window they +// are counted in. A successful login starts the count again. +const ( + customerLoginMaxAttempts = 5 + customerLoginWindow = 15 * time.Minute +) + +// CustomerLoginLockedError means the phone number made too many login attempts and may +// try again at Until. +type CustomerLoginLockedError struct { + Until time.Time +} + +func (e *CustomerLoginLockedError) Error() string { + return fmt.Sprintf("too many login attempts, try again after %s", e.Until.Format(time.RFC3339)) +} + type CustomerAuthProcessor interface { CheckPhoneNumber(ctx context.Context, req *contract.CheckPhoneRequest) (*models.CheckPhoneResponse, error) StartRegistration(ctx context.Context, req *contract.RegisterStartRequest) (*models.RegisterStartResponse, error) @@ -26,20 +54,22 @@ type CustomerAuthProcessor interface { } type customerAuthProcessor struct { - customerAuthRepo repository.CustomerAuthRepository - otpProcessor OtpProcessor - otpRepo repository.OtpRepository - jwtSecret string - tokenTTLMinutes int + customerAuthRepo repository.CustomerAuthRepository + loginAttemptsRepo repository.CustomerLoginAttemptRepository + otpProcessor OtpProcessor + otpRepo repository.OtpRepository + jwtSecret string + tokenTTLMinutes int } -func NewCustomerAuthProcessor(customerAuthRepo repository.CustomerAuthRepository, otpProcessor OtpProcessor, otpRepo repository.OtpRepository, jwtSecret string, tokenTTLMinutes int) CustomerAuthProcessor { +func NewCustomerAuthProcessor(customerAuthRepo repository.CustomerAuthRepository, loginAttemptsRepo repository.CustomerLoginAttemptRepository, otpProcessor OtpProcessor, otpRepo repository.OtpRepository, jwtSecret string, tokenTTLMinutes int) CustomerAuthProcessor { return &customerAuthProcessor{ - customerAuthRepo: customerAuthRepo, - otpProcessor: otpProcessor, - otpRepo: otpRepo, - jwtSecret: jwtSecret, - tokenTTLMinutes: tokenTTLMinutes, + customerAuthRepo: customerAuthRepo, + loginAttemptsRepo: loginAttemptsRepo, + otpProcessor: otpProcessor, + otpRepo: otpRepo, + jwtSecret: jwtSecret, + tokenTTLMinutes: tokenTTLMinutes, } } @@ -344,6 +374,21 @@ func (p *customerAuthProcessor) SetPassword(ctx context.Context, req *contract.R } func (p *customerAuthProcessor) Login(ctx context.Context, req *contract.CustomerLoginRequest) (*models.CustomerLoginResponse, error) { + // Counted before the password is checked, so attempts sent at once all count, and + // for numbers without a customer too, so a refusal never tells which numbers have + // one. + attempts, left, err := p.loginAttemptsRepo.Hit(ctx, req.PhoneNumber, customerLoginWindow) + switch { + case err != nil: + // Without the counter, logins go on unlimited rather than stop for everyone. + logger.FromContext(ctx).WithError(err).Error("CustomerAuthProcessor::Login -> failed to count the attempt") + case attempts > customerLoginMaxAttempts: + if left <= 0 { + left = customerLoginWindow + } + return nil, &CustomerLoginLockedError{Until: time.Now().Add(left).UTC().Truncate(time.Second)} + } + // Get customer by phone number customer, err := p.customerAuthRepo.GetCustomerByPhoneNumber(ctx, req.PhoneNumber) if err != nil { @@ -351,16 +396,19 @@ func (p *customerAuthProcessor) Login(ctx context.Context, req *contract.Custome } if customer == nil { - return nil, fmt.Errorf("customer not found") + return nil, ErrCustomerLoginInvalid } if customer.PasswordHash == nil { - return nil, fmt.Errorf("customer not properly registered") + return nil, ErrCustomerNotRegistered } // Verify password if err := bcrypt.CompareHashAndPassword([]byte(*customer.PasswordHash), []byte(req.Password)); err != nil { - return nil, fmt.Errorf("invalid password") + return nil, ErrCustomerLoginInvalid + } + if err := p.loginAttemptsRepo.Reset(ctx, req.PhoneNumber); err != nil { + logger.FromContext(ctx).WithError(err).Error("CustomerAuthProcessor::Login -> failed to reset the attempts") } // Generate JWT tokens using customer JWT util diff --git a/internal/repository/customer_login_attempt_repository.go b/internal/repository/customer_login_attempt_repository.go new file mode 100644 index 0000000..8ce349f --- /dev/null +++ b/internal/repository/customer_login_attempt_repository.go @@ -0,0 +1,51 @@ +package repository + +import ( + "context" + "fmt" + "time" + + "github.com/redis/go-redis/v9" +) + +const customerLoginAttemptKeyPrefix = "customer_login:attempts:" + +// CustomerLoginAttemptRepository counts customer login attempts per phone number, so a +// password cannot be guessed by trying many. +type CustomerLoginAttemptRepository interface { + // Hit counts one attempt for the phone number. It returns the attempts counted in + // the current window, this one included, and how long the window still runs. A + // window starts at the first attempt after the previous one ended. + Hit(ctx context.Context, phoneNumber string, window time.Duration) (int64, time.Duration, error) + // Reset forgets the attempts of the phone number. + Reset(ctx context.Context, phoneNumber string) error +} + +type customerLoginAttemptRepository struct { + client *redis.Client +} + +func NewCustomerLoginAttemptRepository(client *redis.Client) CustomerLoginAttemptRepository { + return &customerLoginAttemptRepository{client: client} +} + +func (r *customerLoginAttemptRepository) Hit(ctx context.Context, phoneNumber string, window time.Duration) (int64, time.Duration, error) { + key := customerLoginAttemptKeyPrefix + phoneNumber + // One transaction, so the key never exists without its expiry: concurrent attempts + // all count, and a crash cannot leave a number locked for good. + pipe := r.client.TxPipeline() + pipe.SetNX(ctx, key, 0, window) + count := pipe.Incr(ctx, key) + left := pipe.PTTL(ctx, key) + if _, err := pipe.Exec(ctx); err != nil { + return 0, 0, fmt.Errorf("count login attempt: %w", err) + } + return count.Val(), left.Val(), nil +} + +func (r *customerLoginAttemptRepository) Reset(ctx context.Context, phoneNumber string) error { + if err := r.client.Del(ctx, customerLoginAttemptKeyPrefix+phoneNumber).Err(); err != nil { + return fmt.Errorf("reset login attempts: %w", err) + } + return nil +}