diff --git a/.superpowers/sdd/2026-07-27-platform-admin-full-audit/task-5-report.md b/.superpowers/sdd/2026-07-27-platform-admin-full-audit/task-5-report.md index 1d23035..f240018 100644 --- a/.superpowers/sdd/2026-07-27-platform-admin-full-audit/task-5-report.md +++ b/.superpowers/sdd/2026-07-27-platform-admin-full-audit/task-5-report.md @@ -24,3 +24,14 @@ ## Concerns - Approval status values are accepted as non-empty strings to preserve the existing status model; a future workflow may want an explicit state-transition policy (for example, only `pending -> approved|rejected`). + +## Fix round 1: P1 approval transition guard + +### RED / GREEN + +- RED: approval accepted arbitrary non-empty states, updated every matching identity regardless of its current status, and did not compare the JWT operator with the applicant. The new tests reproduced invalid state acceptance, repeat processing, self-approval, and a concurrent second decision after a pending read. +- GREEN: only `approved` and `rejected` requests proceed. The approval must still be `pending`, the applicant cannot decide it, and the update predicate is `identity AND status = pending`. A zero-row conditional update is rejected and does not append an operation audit. + +### Verification + +- `go test ./internal/logic/platform -run 'TestApproveAudit' -count=1 -v` — PASS diff --git a/backend/api/internal/logic/platform/audit.go b/backend/api/internal/logic/platform/audit.go index e2f4398..1918ab8 100644 --- a/backend/api/internal/logic/platform/audit.go +++ b/backend/api/internal/logic/platform/audit.go @@ -14,6 +14,8 @@ import ( "gorm.io/gorm" ) +var errApprovalNotProcessable = errors.New("approval is not processable") + func approvalValues(status, opinion, operatorIdentity string) map[string]any { return map[string]any{ "status": status, @@ -40,6 +42,10 @@ func ApproveAudit(ctx *gin.Context) { infra.Response.Error(ctx, errcode.ErrInvalidArgument) return } + if request.Status != "approved" && request.Status != "rejected" { + infra.Response.Error(ctx, errcode.ErrInvalidArgument) + return + } values := approvalValues(request.Status, request.Opinion, claims.Identity) var approval models.AudApproval @@ -47,6 +53,9 @@ func ApproveAudit(ctx *gin.Context) { if err := transaction.Where("identity = ?", ctx.Param("identity")).First(&approval).Error; err != nil { return err } + if approval.Status != "pending" || approval.ApplicantIdentity == claims.Identity { + return errApprovalNotProcessable + } before, err := json.Marshal(gin.H{ "status": approval.Status, "opinion": approval.Opinion, "handler_identity": approval.HandlerIdentity, "handled_at": approval.HandledAt, @@ -54,10 +63,10 @@ func ApproveAudit(ctx *gin.Context) { if err != nil { return err } - if result := transaction.Model(&models.AudApproval{}).Where("identity = ?", approval.Identity).Updates(values); result.Error != nil { + if result := transaction.Model(&models.AudApproval{}).Where("identity = ? AND status = ?", approval.Identity, "pending").Updates(values); result.Error != nil { return result.Error } else if result.RowsAffected == 0 { - return gorm.ErrRecordNotFound + return errApprovalNotProcessable } after, err := json.Marshal(values) if err != nil { @@ -74,6 +83,10 @@ func ApproveAudit(ctx *gin.Context) { }).Error }) if err != nil { + if errors.Is(err, errApprovalNotProcessable) { + infra.Response.Error(ctx, errcode.ErrInvalidArgument) + return + } if errors.Is(err, gorm.ErrRecordNotFound) { infra.Response.Error(ctx, errcode.ErrRecordNotFound) return diff --git a/backend/api/internal/logic/platform/audit_test.go b/backend/api/internal/logic/platform/audit_test.go index e18513c..024d5cd 100644 --- a/backend/api/internal/logic/platform/audit_test.go +++ b/backend/api/internal/logic/platform/audit_test.go @@ -8,8 +8,10 @@ import ( "testing" "time" + "git.apinb.com/bsm-sdk/core/errcode" "git.apinb.com/bsm-sdk/core/types" "github.com/DATA-DOG/go-sqlmock" + "google.golang.org/grpc/status" ) func TestApprovalValuesOnlyChangesApprovalFieldsAndRecordsOperator(t *testing.T) { @@ -34,8 +36,8 @@ func TestApproveAuditOnlyUpdatesApprovalFieldsAndAppendsOperationAudit(t *testin WithArgs("approval-a", 1). WillReturnRows(sqlmock.NewRows([]string{"id", "identity", "created_at", "updated_at", "status", "version", "business_type", "business_identity", "applicant_identity", "opinion", "handler_identity", "handled_at"}). AddRow(uint64(1), "approval-a", now, now, "pending", 1, "refund", "payment-a", "applicant-a", "", "", nil)) - mock.ExpectExec(regexp.QuoteMeta(`UPDATE "aud_approval" SET "handled_at"=$1,"handler_identity"=$2,"opinion"=$3,"status"=$4,"updated_at"=$5 WHERE identity = $6`)). - WithArgs(sqlmock.AnyArg(), "operator-a", "accepted", "approved", sqlmock.AnyArg(), "approval-a"). + mock.ExpectExec(regexp.QuoteMeta(`UPDATE "aud_approval" SET "handled_at"=$1,"handler_identity"=$2,"opinion"=$3,"status"=$4,"updated_at"=$5 WHERE identity = $6 AND status = $7`)). + WithArgs(sqlmock.AnyArg(), "operator-a", "accepted", "approved", sqlmock.AnyArg(), "approval-a", "pending"). WillReturnResult(sqlmock.NewResult(0, 1)) mock.ExpectQuery(regexp.QuoteMeta(`INSERT INTO "aud_operation_log" ("identity","created_at","updated_at","status","version","operator_identity","action","object_type","object_identity","before_data","after_data") VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9,$10,$11) RETURNING "id"`)). WithArgs(sqlmock.AnyArg(), sqlmock.AnyArg(), sqlmock.AnyArg(), "enabled", 1, "operator-a", "approve", "aud_approval", "approval-a", jsonContaining(`"status":"pending"`), jsonContaining(`"handler_identity":"operator-a"`)). @@ -53,6 +55,78 @@ func TestApproveAuditOnlyUpdatesApprovalFieldsAndAppendsOperationAudit(t *testin } } +func TestApproveAuditRejectsStatusesOutsideApprovedAndRejected(t *testing.T) { + for _, decision := range []string{"pending", "archived"} { + t.Run(decision, func(t *testing.T) { + _, mock := setupPlatformRoleDatabase(t) + ctx, recorder := updateContext(http.MethodPost, "/audit/aud_approval/approval-a/approve", "approval-a", []byte(`{"status":"`+decision+`"}`)) + ctx.Set("Auth", &types.JwtClaims{Identity: "operator-a"}) + + ApproveAudit(ctx) + + assertResponseCode(t, recorder, int32(status.Code(errcode.ErrInvalidArgument))) + assertMockExpectations(t, mock) + }) + } +} + +func TestApproveAuditRejectsAlreadyHandledApproval(t *testing.T) { + _, mock := setupPlatformRoleDatabase(t) + mock.ExpectBegin() + mock.ExpectQuery(regexp.QuoteMeta(`SELECT * FROM "aud_approval" WHERE identity = $1 ORDER BY "aud_approval"."id" LIMIT $2`)). + WithArgs("approval-a", 1). + WillReturnRows(approvalRows("approval-a", "approved", "applicant-a")) + mock.ExpectRollback() + + ctx, recorder := updateContext(http.MethodPost, "/audit/aud_approval/approval-a/approve", "approval-a", []byte(`{"status":"approved"}`)) + ctx.Set("Auth", &types.JwtClaims{Identity: "operator-a"}) + ApproveAudit(ctx) + + assertResponseCode(t, recorder, int32(status.Code(errcode.ErrInvalidArgument))) + assertMockExpectations(t, mock) +} + +func TestApproveAuditRejectsTheApplicant(t *testing.T) { + _, mock := setupPlatformRoleDatabase(t) + mock.ExpectBegin() + mock.ExpectQuery(regexp.QuoteMeta(`SELECT * FROM "aud_approval" WHERE identity = $1 ORDER BY "aud_approval"."id" LIMIT $2`)). + WithArgs("approval-a", 1). + WillReturnRows(approvalRows("approval-a", "pending", "operator-a")) + mock.ExpectRollback() + + ctx, recorder := updateContext(http.MethodPost, "/audit/aud_approval/approval-a/approve", "approval-a", []byte(`{"status":"approved"}`)) + ctx.Set("Auth", &types.JwtClaims{Identity: "operator-a"}) + ApproveAudit(ctx) + + assertResponseCode(t, recorder, int32(status.Code(errcode.ErrInvalidArgument))) + assertMockExpectations(t, mock) +} + +func TestApproveAuditRejectsAConcurrentSecondDecision(t *testing.T) { + _, mock := setupPlatformRoleDatabase(t) + mock.ExpectBegin() + mock.ExpectQuery(regexp.QuoteMeta(`SELECT * FROM "aud_approval" WHERE identity = $1 ORDER BY "aud_approval"."id" LIMIT $2`)). + WithArgs("approval-a", 1). + WillReturnRows(approvalRows("approval-a", "pending", "applicant-a")) + mock.ExpectExec(regexp.QuoteMeta(`UPDATE "aud_approval" SET "handled_at"=$1,"handler_identity"=$2,"opinion"=$3,"status"=$4,"updated_at"=$5 WHERE identity = $6 AND status = $7`)). + WithArgs(sqlmock.AnyArg(), "operator-a", "", "approved", sqlmock.AnyArg(), "approval-a", "pending"). + WillReturnResult(sqlmock.NewResult(0, 0)) + mock.ExpectRollback() + + ctx, recorder := updateContext(http.MethodPost, "/audit/aud_approval/approval-a/approve", "approval-a", []byte(`{"status":"approved"}`)) + ctx.Set("Auth", &types.JwtClaims{Identity: "operator-a"}) + ApproveAudit(ctx) + + assertResponseCode(t, recorder, int32(status.Code(errcode.ErrInvalidArgument))) + assertMockExpectations(t, mock) +} + +func approvalRows(identity, approvalStatus, applicant string) *sqlmock.Rows { + now := time.Now().UTC() + return sqlmock.NewRows([]string{"id", "identity", "created_at", "updated_at", "status", "version", "business_type", "business_identity", "applicant_identity", "opinion", "handler_identity", "handled_at"}). + AddRow(uint64(1), identity, now, now, approvalStatus, 1, "refund", "payment-a", applicant, "", "", nil) +} + type jsonContaining string func (expected jsonContaining) Match(value driver.Value) bool {