fix(platform): guard approval transitions
This commit is contained in:
@@ -24,3 +24,14 @@
|
|||||||
## Concerns
|
## 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`).
|
- 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
|
||||||
|
|||||||
@@ -14,6 +14,8 @@ import (
|
|||||||
"gorm.io/gorm"
|
"gorm.io/gorm"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
var errApprovalNotProcessable = errors.New("approval is not processable")
|
||||||
|
|
||||||
func approvalValues(status, opinion, operatorIdentity string) map[string]any {
|
func approvalValues(status, opinion, operatorIdentity string) map[string]any {
|
||||||
return map[string]any{
|
return map[string]any{
|
||||||
"status": status,
|
"status": status,
|
||||||
@@ -40,6 +42,10 @@ func ApproveAudit(ctx *gin.Context) {
|
|||||||
infra.Response.Error(ctx, errcode.ErrInvalidArgument)
|
infra.Response.Error(ctx, errcode.ErrInvalidArgument)
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
if request.Status != "approved" && request.Status != "rejected" {
|
||||||
|
infra.Response.Error(ctx, errcode.ErrInvalidArgument)
|
||||||
|
return
|
||||||
|
}
|
||||||
|
|
||||||
values := approvalValues(request.Status, request.Opinion, claims.Identity)
|
values := approvalValues(request.Status, request.Opinion, claims.Identity)
|
||||||
var approval models.AudApproval
|
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 {
|
if err := transaction.Where("identity = ?", ctx.Param("identity")).First(&approval).Error; err != nil {
|
||||||
return err
|
return err
|
||||||
}
|
}
|
||||||
|
if approval.Status != "pending" || approval.ApplicantIdentity == claims.Identity {
|
||||||
|
return errApprovalNotProcessable
|
||||||
|
}
|
||||||
before, err := json.Marshal(gin.H{
|
before, err := json.Marshal(gin.H{
|
||||||
"status": approval.Status, "opinion": approval.Opinion,
|
"status": approval.Status, "opinion": approval.Opinion,
|
||||||
"handler_identity": approval.HandlerIdentity, "handled_at": approval.HandledAt,
|
"handler_identity": approval.HandlerIdentity, "handled_at": approval.HandledAt,
|
||||||
@@ -54,10 +63,10 @@ func ApproveAudit(ctx *gin.Context) {
|
|||||||
if err != nil {
|
if err != nil {
|
||||||
return err
|
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
|
return result.Error
|
||||||
} else if result.RowsAffected == 0 {
|
} else if result.RowsAffected == 0 {
|
||||||
return gorm.ErrRecordNotFound
|
return errApprovalNotProcessable
|
||||||
}
|
}
|
||||||
after, err := json.Marshal(values)
|
after, err := json.Marshal(values)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
@@ -74,6 +83,10 @@ func ApproveAudit(ctx *gin.Context) {
|
|||||||
}).Error
|
}).Error
|
||||||
})
|
})
|
||||||
if err != nil {
|
if err != nil {
|
||||||
|
if errors.Is(err, errApprovalNotProcessable) {
|
||||||
|
infra.Response.Error(ctx, errcode.ErrInvalidArgument)
|
||||||
|
return
|
||||||
|
}
|
||||||
if errors.Is(err, gorm.ErrRecordNotFound) {
|
if errors.Is(err, gorm.ErrRecordNotFound) {
|
||||||
infra.Response.Error(ctx, errcode.ErrRecordNotFound)
|
infra.Response.Error(ctx, errcode.ErrRecordNotFound)
|
||||||
return
|
return
|
||||||
|
|||||||
@@ -8,8 +8,10 @@ import (
|
|||||||
"testing"
|
"testing"
|
||||||
"time"
|
"time"
|
||||||
|
|
||||||
|
"git.apinb.com/bsm-sdk/core/errcode"
|
||||||
"git.apinb.com/bsm-sdk/core/types"
|
"git.apinb.com/bsm-sdk/core/types"
|
||||||
"github.com/DATA-DOG/go-sqlmock"
|
"github.com/DATA-DOG/go-sqlmock"
|
||||||
|
"google.golang.org/grpc/status"
|
||||||
)
|
)
|
||||||
|
|
||||||
func TestApprovalValuesOnlyChangesApprovalFieldsAndRecordsOperator(t *testing.T) {
|
func TestApprovalValuesOnlyChangesApprovalFieldsAndRecordsOperator(t *testing.T) {
|
||||||
@@ -34,8 +36,8 @@ func TestApproveAuditOnlyUpdatesApprovalFieldsAndAppendsOperationAudit(t *testin
|
|||||||
WithArgs("approval-a", 1).
|
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"}).
|
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))
|
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`)).
|
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").
|
WithArgs(sqlmock.AnyArg(), "operator-a", "accepted", "approved", sqlmock.AnyArg(), "approval-a", "pending").
|
||||||
WillReturnResult(sqlmock.NewResult(0, 1))
|
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"`)).
|
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"`)).
|
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
|
type jsonContaining string
|
||||||
|
|
||||||
func (expected jsonContaining) Match(value driver.Value) bool {
|
func (expected jsonContaining) Match(value driver.Value) bool {
|
||||||
|
|||||||
Reference in New Issue
Block a user