fix: resolve 3 code review issues in allow_user_refund
1. PrepareRefund: block refund on provider instance lookup failure instead of silently skipping permission check (medium severity) 2. UpdateProviderInstance: allow enabling refund_enabled and allow_user_refund in the same request by checking req.RefundEnabled value before falling back to DB read 3. ExecuteRefund: only revoke subscription on ErrAdjustWouldExpire, abort on other errors (DB failure, not found) instead of unconditionally revoking
This commit is contained in:
@@ -231,10 +231,18 @@ func (s *PaymentConfigService) UpdateProviderInstance(ctx context.Context, id in
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
if req.AllowUserRefund != nil {
|
if req.AllowUserRefund != nil {
|
||||||
// Only allow enabling when refund_enabled is true
|
// Only allow enabling when refund_enabled is (or will be) true
|
||||||
if *req.AllowUserRefund {
|
if *req.AllowUserRefund {
|
||||||
inst, err := s.entClient.PaymentProviderInstance.Get(ctx, id)
|
refundEnabled := false
|
||||||
if err == nil && inst.RefundEnabled {
|
if req.RefundEnabled != nil {
|
||||||
|
refundEnabled = *req.RefundEnabled
|
||||||
|
} else {
|
||||||
|
inst, err := s.entClient.PaymentProviderInstance.Get(ctx, id)
|
||||||
|
if err == nil {
|
||||||
|
refundEnabled = inst.RefundEnabled
|
||||||
|
}
|
||||||
|
}
|
||||||
|
if refundEnabled {
|
||||||
u.SetAllowUserRefund(true)
|
u.SetAllowUserRefund(true)
|
||||||
}
|
}
|
||||||
} else {
|
} else {
|
||||||
|
|||||||
@@ -2,6 +2,7 @@ package service
|
|||||||
|
|
||||||
import (
|
import (
|
||||||
"context"
|
"context"
|
||||||
|
"errors"
|
||||||
"fmt"
|
"fmt"
|
||||||
"log/slog"
|
"log/slog"
|
||||||
"math"
|
"math"
|
||||||
@@ -93,15 +94,16 @@ func (s *PaymentService) PrepareRefund(ctx context.Context, oid int64, amt float
|
|||||||
// Check provider instance allows admin refund
|
// Check provider instance allows admin refund
|
||||||
inst, instErr := s.getOrderProviderInstance(ctx, o)
|
inst, instErr := s.getOrderProviderInstance(ctx, o)
|
||||||
if instErr != nil {
|
if instErr != nil {
|
||||||
slog.Warn("refund: provider instance not found", "orderID", oid, "error", instErr)
|
slog.Warn("refund: provider instance lookup failed", "orderID", oid, "error", instErr)
|
||||||
|
return nil, nil, infraerrors.InternalServer("PROVIDER_LOOKUP_FAILED", "failed to look up payment provider for this order")
|
||||||
}
|
}
|
||||||
if inst != nil && !inst.RefundEnabled {
|
if inst == nil {
|
||||||
return nil, nil, infraerrors.Forbidden("REFUND_DISABLED", "refund is not enabled for this provider")
|
|
||||||
}
|
|
||||||
if inst == nil && instErr == nil {
|
|
||||||
// Legacy order without provider_instance_id — block refund
|
// Legacy order without provider_instance_id — block refund
|
||||||
return nil, nil, infraerrors.Forbidden("REFUND_DISABLED", "refund is not available for this order")
|
return nil, nil, infraerrors.Forbidden("REFUND_DISABLED", "refund is not available for this order")
|
||||||
}
|
}
|
||||||
|
if !inst.RefundEnabled {
|
||||||
|
return nil, nil, infraerrors.Forbidden("REFUND_DISABLED", "refund is not enabled for this provider")
|
||||||
|
}
|
||||||
if math.IsNaN(amt) || math.IsInf(amt, 0) {
|
if math.IsNaN(amt) || math.IsInf(amt, 0) {
|
||||||
return nil, nil, infraerrors.BadRequest("INVALID_AMOUNT", "invalid refund amount")
|
return nil, nil, infraerrors.BadRequest("INVALID_AMOUNT", "invalid refund amount")
|
||||||
}
|
}
|
||||||
@@ -183,10 +185,17 @@ func (s *PaymentService) ExecuteRefund(ctx context.Context, p *RefundPlan) (*Ref
|
|||||||
if !s.hasAuditLog(ctx, p.OrderID, "REFUND_ROLLBACK_FAILED") {
|
if !s.hasAuditLog(ctx, p.OrderID, "REFUND_ROLLBACK_FAILED") {
|
||||||
_, err := s.subscriptionSvc.ExtendSubscription(ctx, p.SubscriptionID, -p.SubDaysToDeduct)
|
_, err := s.subscriptionSvc.ExtendSubscription(ctx, p.SubscriptionID, -p.SubDaysToDeduct)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
slog.Info("subscription deduction would expire, revoking", "orderID", p.OrderID, "subID", p.SubscriptionID, "days", p.SubDaysToDeduct)
|
if errors.Is(err, ErrAdjustWouldExpire) {
|
||||||
if revokeErr := s.subscriptionSvc.RevokeSubscription(ctx, p.SubscriptionID); revokeErr != nil {
|
// Deduction would expire the subscription — revoke it entirely
|
||||||
|
slog.Info("subscription deduction would expire, revoking", "orderID", p.OrderID, "subID", p.SubscriptionID, "days", p.SubDaysToDeduct)
|
||||||
|
if revokeErr := s.subscriptionSvc.RevokeSubscription(ctx, p.SubscriptionID); revokeErr != nil {
|
||||||
|
s.restoreStatus(ctx, p)
|
||||||
|
return nil, fmt.Errorf("revoke subscription: %w", revokeErr)
|
||||||
|
}
|
||||||
|
} else {
|
||||||
|
// Other errors (DB failure, not found) — abort refund
|
||||||
s.restoreStatus(ctx, p)
|
s.restoreStatus(ctx, p)
|
||||||
return nil, fmt.Errorf("revoke subscription: %w", revokeErr)
|
return nil, fmt.Errorf("deduct subscription days: %w", err)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
} else {
|
} else {
|
||||||
|
|||||||
Reference in New Issue
Block a user