Repository navigation
fix(auth): enforce four-eyes principle on purchase approve - #1422
Merged
Merged
Conversation
cristim
added
triaged
Member
Author
|
@coderabbitai review |
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
✅ Action performedReview finished.
|
) The Approve button was visible for a user's own queued purchase even when their role lacked approve-any or approve-own permission. Ownership alone was incorrectly treated as sufficient authorization. Root causes fixed: - canApprovePendingRow() in history.ts now requires an explicit approve-own permission before consulting ownership; approve-any continues to bypass the ownership check as before. - DefaultUserPermissions() no longer includes approve-own:purchases. The Standard Users group must not self-approve by default; an explicit custom group grant is required (four-eyes principle). - Migration 000086 removes approve-own from the seeded Standard Users Postgres group for existing deployments. Numbered 000086 to sit after the in-flight 000084 (#1277) and 000085 (#808), which merge first. - permissions.generated.ts regenerated to match the updated Go default. Regression tests added (fail-before / pass-after): - Frontend: "user without any approve permission sees NO Approve button even on their own rows (issue #1407 four-eyes)" in src/__tests__/history-approve-button.test.ts - Backend: TestHandler_approvePurchase_RejectsCreatorWithoutApprovePermission in internal/api/handler_purchases_test.go -- asserts 403 when the creator calls approve without holding approve-any or approve-own. - Migration: TestMigration_RemoveApproveOwnFromStandardUsers (integration) confirms 000086 removes approve-own while retaining cancel-own/retry-own, and that its down migration restores approve-own. - Unit: DefaultUserPermissions four-eyes guard in types_test.go.
cristim
force-pushed
the
fix/1407-approve-button-ownership-gate
branch
from
July 16, 2026 21:06
1c54f30 to
e78223f
Compare
Member
Author
|
Renumbered migration At the time this PR was opened,
Landing this PR at Changes in this push (force-with-lease):
Verification (all exit 0): The actual four-eyes fix (frontend gate + |
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
cristim
added a commit
that referenced
this pull request
Jul 16, 2026
Round-3 money-path review fixes for the Sell-on-Marketplace flow (#808). Price floor (was arithmetically wrong): - Enforce the floor PER TIER on the one-time sale Price. AWS PriceScheduleSpecification.Price is the lump-sum a buyer pays when TermMonths remain, not a per-month rate; the old code compared Price*TermMonths against the floor, over-crediting a cheap schedule up to ~term-fold so a $5 tier passed a $60 floor on a $1,200 residual. - Reject any tier whose TermMonths exceeds the RI's remaining months (was unvalidated and arbitrarily inflatable). - Per-instance basis: purchase_history.UpfrontCost is the row total for all Count instances, but Marketplace prices are per instance, so divide the residual by Count in both the floor and the default schedule. - Upfront-only residual: drop recurring (monthly) cost from the residual basis. In the RI Marketplace the buyer assumes recurring charges after transfer, so the seller recovers only the upfront remainder; including recurring overpriced partial-upfront RIs and made no-upfront RIs unpriceable. - Extract marketplaceResidualPerUnit as the shared per-unit basis so the default schedule and the floor cannot drift. Reachability: render the Sell button for completed AWS EC2 rows whose offering_class is empty (unknown), not just "standard". CUDly stamps "convertible" on its own EC2 purchases and externally-created Standard RIs arrive with an empty class until the backend lazily populates it; gating the UI on "standard" alone made the feature unreachable end to end. Provider and service are now checked so unknown-class non-EC2 rows never show the button. Nits: - Use SDK enum constants string(ec2types.OfferingClassType{Standard,Convertible}) instead of raw "standard"/"convertible" literals in the offering-class gate, the isKnownNonStandardOfferingClass predicate, and the purchase-history stamp. - Route populateOfferingClass errors through mapAWSMarketplaceError so an AWS client fault (e.g. InvalidReservedInstancesID.NotFound) surfaces as a 400, not a generic 500. Migration: renumber 000085 -> 000087 to stay above #1422's 000086 (main tops at pre-84; #1422 takes 000086, so this PR takes 000087). Regression tests (fail-before / pass-after verified): - TestResolveMarketplacePriceSchedule_BelowFloorRejected: a {12mo, $5} tier on a $1,200 per-unit residual (Count=1) is now rejected; passed under the old Price*TermMonths floor. - TestResolveMarketplacePriceSchedule_PerUnitFloor: on a $1,200 row-total with Count=3 the per-unit floor is $20, so $19 is rejected and $21 accepted. - TestResolveMarketplacePriceSchedule_TermExceedsRemainingRejected: a tier term beyond the remaining months is rejected. - history-marketplace-sell-button.test.ts: the Sell button renders for a completed AWS EC2 row with an empty offering_class and stays hidden for convertible, non-EC2, non-AWS, active-listing, and anonymous cases. - TestMigration087_MarketplaceColumns: fresh DB migrates cleanly through 000087 and the three columns round-trip.
Member
Author
|
Merged to main after independent adversarial security review (verdict SHIP, four-eyes invariant verified at the backend enforcement point + fail-before/pass-after) and all CI green. Enforces four-eyes: removes approve-own from DefaultUserPermissions and strips it from the seeded Standard Users group (migration 000086), so a user can no longer approve their own purchase without an explicit approve permission; frontend Approve button gated to match. Migration 000086 is above main's max; the other in-flight migrations (#808=087, #1277 renumbered to 088) merge after it in ascending order. |
cristim
added a commit
that referenced
this pull request
Jul 16, 2026
Round-3 money-path review fixes for the Sell-on-Marketplace flow (#808). Price floor (was arithmetically wrong): - Enforce the floor PER TIER on the one-time sale Price. AWS PriceScheduleSpecification.Price is the lump-sum a buyer pays when TermMonths remain, not a per-month rate; the old code compared Price*TermMonths against the floor, over-crediting a cheap schedule up to ~term-fold so a $5 tier passed a $60 floor on a $1,200 residual. - Reject any tier whose TermMonths exceeds the RI's remaining months (was unvalidated and arbitrarily inflatable). - Per-instance basis: purchase_history.UpfrontCost is the row total for all Count instances, but Marketplace prices are per instance, so divide the residual by Count in both the floor and the default schedule. - Upfront-only residual: drop recurring (monthly) cost from the residual basis. In the RI Marketplace the buyer assumes recurring charges after transfer, so the seller recovers only the upfront remainder; including recurring overpriced partial-upfront RIs and made no-upfront RIs unpriceable. - Extract marketplaceResidualPerUnit as the shared per-unit basis so the default schedule and the floor cannot drift. Reachability: render the Sell button for completed AWS EC2 rows whose offering_class is empty (unknown), not just "standard". CUDly stamps "convertible" on its own EC2 purchases and externally-created Standard RIs arrive with an empty class until the backend lazily populates it; gating the UI on "standard" alone made the feature unreachable end to end. Provider and service are now checked so unknown-class non-EC2 rows never show the button. Nits: - Use SDK enum constants string(ec2types.OfferingClassType{Standard,Convertible}) instead of raw "standard"/"convertible" literals in the offering-class gate, the isKnownNonStandardOfferingClass predicate, and the purchase-history stamp. - Route populateOfferingClass errors through mapAWSMarketplaceError so an AWS client fault (e.g. InvalidReservedInstancesID.NotFound) surfaces as a 400, not a generic 500. Migration: renumber 000085 -> 000087 to stay above #1422's 000086 (main tops at pre-84; #1422 takes 000086, so this PR takes 000087). Regression tests (fail-before / pass-after verified): - TestResolveMarketplacePriceSchedule_BelowFloorRejected: a {12mo, $5} tier on a $1,200 per-unit residual (Count=1) is now rejected; passed under the old Price*TermMonths floor. - TestResolveMarketplacePriceSchedule_PerUnitFloor: on a $1,200 row-total with Count=3 the per-unit floor is $20, so $19 is rejected and $21 accepted. - TestResolveMarketplacePriceSchedule_TermExceedsRemainingRejected: a tier term beyond the remaining months is rejected. - history-marketplace-sell-button.test.ts: the Sell button renders for a completed AWS EC2 row with an empty offering_class and stays hidden for convertible, non-EC2, non-AWS, active-listing, and anonymous cases. - TestMigration087_MarketplaceColumns: fresh DB migrates cleanly through 000087 and the three columns round-trip.
This was referenced Jul 16, 2026
cristim
added a commit
that referenced
this pull request
Jul 17, 2026
Round-3 money-path review fixes for the Sell-on-Marketplace flow (#808). Price floor (was arithmetically wrong): - Enforce the floor PER TIER on the one-time sale Price. AWS PriceScheduleSpecification.Price is the lump-sum a buyer pays when TermMonths remain, not a per-month rate; the old code compared Price*TermMonths against the floor, over-crediting a cheap schedule up to ~term-fold so a $5 tier passed a $60 floor on a $1,200 residual. - Reject any tier whose TermMonths exceeds the RI's remaining months (was unvalidated and arbitrarily inflatable). - Per-instance basis: purchase_history.UpfrontCost is the row total for all Count instances, but Marketplace prices are per instance, so divide the residual by Count in both the floor and the default schedule. - Upfront-only residual: drop recurring (monthly) cost from the residual basis. In the RI Marketplace the buyer assumes recurring charges after transfer, so the seller recovers only the upfront remainder; including recurring overpriced partial-upfront RIs and made no-upfront RIs unpriceable. - Extract marketplaceResidualPerUnit as the shared per-unit basis so the default schedule and the floor cannot drift. Reachability: render the Sell button for completed AWS EC2 rows whose offering_class is empty (unknown), not just "standard". CUDly stamps "convertible" on its own EC2 purchases and externally-created Standard RIs arrive with an empty class until the backend lazily populates it; gating the UI on "standard" alone made the feature unreachable end to end. Provider and service are now checked so unknown-class non-EC2 rows never show the button. Nits: - Use SDK enum constants string(ec2types.OfferingClassType{Standard,Convertible}) instead of raw "standard"/"convertible" literals in the offering-class gate, the isKnownNonStandardOfferingClass predicate, and the purchase-history stamp. - Route populateOfferingClass errors through mapAWSMarketplaceError so an AWS client fault (e.g. InvalidReservedInstancesID.NotFound) surfaces as a 400, not a generic 500. Migration: renumber 000085 -> 000087 to stay above #1422's 000086 (main tops at pre-84; #1422 takes 000086, so this PR takes 000087). Regression tests (fail-before / pass-after verified): - TestResolveMarketplacePriceSchedule_BelowFloorRejected: a {12mo, $5} tier on a $1,200 per-unit residual (Count=1) is now rejected; passed under the old Price*TermMonths floor. - TestResolveMarketplacePriceSchedule_PerUnitFloor: on a $1,200 row-total with Count=3 the per-unit floor is $20, so $19 is rejected and $21 accepted. - TestResolveMarketplacePriceSchedule_TermExceedsRemainingRejected: a tier term beyond the remaining months is rejected. - history-marketplace-sell-button.test.ts: the Sell button renders for a completed AWS EC2 row with an empty offering_class and stays hidden for convertible, non-EC2, non-AWS, active-listing, and anonymous cases. - TestMigration087_MarketplaceColumns: fresh DB migrates cleanly through 000087 and the three columns round-trip.
cristim
added a commit
that referenced
this pull request
Jul 17, 2026
Round-3 money-path review fixes for the Sell-on-Marketplace flow (#808). Price floor (was arithmetically wrong): - Enforce the floor PER TIER on the one-time sale Price. AWS PriceScheduleSpecification.Price is the lump-sum a buyer pays when TermMonths remain, not a per-month rate; the old code compared Price*TermMonths against the floor, over-crediting a cheap schedule up to ~term-fold so a $5 tier passed a $60 floor on a $1,200 residual. - Reject any tier whose TermMonths exceeds the RI's remaining months (was unvalidated and arbitrarily inflatable). - Per-instance basis: purchase_history.UpfrontCost is the row total for all Count instances, but Marketplace prices are per instance, so divide the residual by Count in both the floor and the default schedule. - Upfront-only residual: drop recurring (monthly) cost from the residual basis. In the RI Marketplace the buyer assumes recurring charges after transfer, so the seller recovers only the upfront remainder; including recurring overpriced partial-upfront RIs and made no-upfront RIs unpriceable. - Extract marketplaceResidualPerUnit as the shared per-unit basis so the default schedule and the floor cannot drift. Reachability: render the Sell button for completed AWS EC2 rows whose offering_class is empty (unknown), not just "standard". CUDly stamps "convertible" on its own EC2 purchases and externally-created Standard RIs arrive with an empty class until the backend lazily populates it; gating the UI on "standard" alone made the feature unreachable end to end. Provider and service are now checked so unknown-class non-EC2 rows never show the button. Nits: - Use SDK enum constants string(ec2types.OfferingClassType{Standard,Convertible}) instead of raw "standard"/"convertible" literals in the offering-class gate, the isKnownNonStandardOfferingClass predicate, and the purchase-history stamp. - Route populateOfferingClass errors through mapAWSMarketplaceError so an AWS client fault (e.g. InvalidReservedInstancesID.NotFound) surfaces as a 400, not a generic 500. Migration: renumber 000085 -> 000087 to stay above #1422's 000086 (main tops at pre-84; #1422 takes 000086, so this PR takes 000087). Regression tests (fail-before / pass-after verified): - TestResolveMarketplacePriceSchedule_BelowFloorRejected: a {12mo, $5} tier on a $1,200 per-unit residual (Count=1) is now rejected; passed under the old Price*TermMonths floor. - TestResolveMarketplacePriceSchedule_PerUnitFloor: on a $1,200 row-total with Count=3 the per-unit floor is $20, so $19 is rejected and $21 accepted. - TestResolveMarketplacePriceSchedule_TermExceedsRemainingRejected: a tier term beyond the remaining months is rejected. - history-marketplace-sell-button.test.ts: the Sell button renders for a completed AWS EC2 row with an empty offering_class and stays hidden for convertible, non-EC2, non-AWS, active-listing, and anonymous cases. - TestMigration087_MarketplaceColumns: fresh DB migrates cleanly through 000087 and the three columns round-trip.
cristim
added a commit
that referenced
this pull request
Jul 17, 2026
…#292) (#808) * feat(marketplace): sell/cancel Standard RIs on AWS Marketplace - backend (closes #292) Add sell-on-Marketplace support for Standard Reserved Instances: - migration 000060: add offering_class, listing_id, listing_state to purchase_history - auth: sell-any and sell-own actions; sell-own added to DefaultUserPermissions - config: extend PurchaseHistoryRecord, StoreInterface with GetPurchaseHistoryByPurchaseID + UpdatePurchaseHistoryListing - ec2 client: CreateReservedInstancesListing, DescribeReservedInstancesListings, CancelReservedInstancesListing - api: handler_marketplace.go with marketplaceList/Cancel, authorizeSessionSell (sell-any/sell-own RBAC), default price schedule (95% of residual value) - router: POST /api/purchases/{id}/marketplace-list + /marketplace-cancel routes - all tests updated for the new interface methods and permission count * feat(marketplace): sell/cancel Standard RIs on AWS Marketplace - frontend (closes #292) Add Sell on Marketplace / Cancel listing buttons to purchase history rows: - types.ts: extend HistoryPurchase with offering_class, listing_id, listing_state - api/purchases.ts: createMarketplaceListing + cancelMarketplaceListing; re-export from api/index.ts - history.ts: canSellOnMarketplace / canCancelMarketplaceListing predicates; renderActionCell renders Sell/Cancel buttons for Standard RI completed rows; wireRowActionHandlers wires confirm-dialog + API + toast + reload for both buttons * fix(history): address CR wave4 frontend marketplace findings - canSellOnMarketplace: gate on remaining term >= 1 month computed from purchase timestamp + total term; matured Standard RIs no longer show Sell - sell click handler: open pricing/schedule modal before calling createMarketplaceListing so users see RI summary, default list price, and 12% fee breakdown before confirming - lineage actions cell: build trailingActions[] first then combine with lineage[] so Sell/Cancel buttons render on retry-descendant rows * fix(marketplace): address CR wave4 Go handler findings - Compute actual remaining months from purchase timestamp and total term via computeRemainingMonths; removes overpricing of older RIs in the default price schedule (was using full contract term) - Map AWS errors via mapAWSMarketplaceError: inspect smithy.APIError code and fault; client-fault errors return 4xx instead of blanket 502 - On DB failure after successful listing creation, attempt compensating rollback via CancelMarketplaceListing to prevent AWS/DB desync; return internal error rather than false success to the caller - Same DB-failure fix for cancel handler: return error on UpdatePurchaseHistoryListing failure so the caller knows the state is out of sync * fix(migrations): add settlement columns to 000060 marketplace migration Add listed_at, listing_price_schedule, listing_proceeds_received, and listing_fee_paid columns for marketplace settlement tracking. All nullable, consistent with existing offering_class/listing_id/listing_state nullability. Down migration updated to drop the new columns. * fix(ec2): validate non-empty listing ID from CreateReservedInstancesListing Return an error when AWS returns a listing with an empty ReservedInstancesListingId rather than propagating a blank ID that would silently make rollback and describe calls fail. * fix(pr-808): compile + CR findings + pre-commit Repair the stalled marketplace sell/cancel work: - Add the three missing marketplace methods to MockEC2Client so the ec2 package compiles against the extended EC2API interface (CreateReservedInstancesListing, DescribeReservedInstancesListings, CancelReservedInstancesListing). - Extract validateMarketplaceListRequest from marketplaceList to bring its cyclomatic complexity back under the gocyclo threshold. - Regenerate frontend/src/permissions.generated.ts to include the new sell-own:purchases permission (pre-commit codegen check). - Harden Describe/Cancel marketplace paths to fall back to the caller-supplied listing ID when AWS omits ReservedInstancesListingId, so downstream persistence never stores an empty ID (CR finding). * fix(marketplace): adapt to nullable PurchaseHistoryRecord.MonthlyCost After rebasing onto feat/multicloud-web-frontend, base PR #258 made PurchaseHistoryRecord.MonthlyCost a *float64 (nullable). The marketplace default price-schedule path passed it as a float64. Dereference it nil-safely at the call site, treating an absent monthly breakdown as a zero recurring contribution to residual value (per the nullable-not-zero convention: nil means "no breakdown recorded", which contributes nothing to the residual list price). * refactor(config): extract scanPurchaseHistoryRow to fix gocyclo budget queryPurchaseHistory reached cyclomatic complexity 11 (budget 10) after the nullable marketplace columns (offering_class, listing_id, listing_state) and the nullable monthly_cost handling were added to the per-row scan. Pull the scan plus nullable-column reconciliation into scanPurchaseHistoryRow so the parent function drops back under the limit. Behavior is unchanged. * fix(api/marketplace): prorate upfront cost to remaining term in default schedule When the caller omits a price schedule, the default was computing total value as (full_upfront + recurring * remaining_months). For an older RI this overprices the upfront component because only (remaining/original) of the upfront has not yet been amortized. Change to: upfront_remaining = upfront_cost * (remaining / original_term) total_value = upfront_remaining + recurring * remaining_months Pass row.Term as originalTerm through resolveMarketplacePriceSchedule. Addresses CR finding on handler_marketplace.go. * fix(marketplace): multi-count listing, handler tests, descope poller columns (closes #292 partial) Address adversarial-review gaps on the RI Marketplace sell/cancel PR: - Multi-count fix: CreateMarketplaceListing now lists every RI in the row (purchase_history.count) instead of a hardcoded InstanceCount=1, so a row of N Standard RIs lists all N. The EC2 client rejects a non-positive count before the outbound call; the handler passes row.Count (floored at 1 for legacy rows). Adds EC2 client tests for create (multi-count + guards), describe, and cancel via the SDK mock. - Tests: add handler_marketplace_test.go covering convertible -> 400, sell-own allow + deny (account scoping), duplicate active listing -> 409, AWS error mapping (client fault -> 400, server/unknown -> 502), DB-failure compensating rollback, and default-schedule proration math. - Descope poller: migration 000060 keeps only the columns the implemented flow reads/writes (offering_class, listing_id, listing_state). The settlement/poller columns (listed_at, listing_price_schedule, listing_proceeds_received, listing_fee_paid) are removed so the schema never carries columns nothing populates; they land with the poller in the #292 follow-up (#966). - Repair a stale Session.Role="admin" reference in a pre-existing purchases test (removed in the group-only auth migration) so the api test package compiles after rebasing onto the current base. * fix(migrations): renumber to 000065 to avoid base conflict 000060 is taken by 000060_cleanup_universal_plans on base. * fix(migrations): renumber marketplace migration to 000068 to clear base conflict The marketplace listing migration was numbered 000065, colliding with 000065_enforce_min_one_admin which has since landed on feat/multicloud-web-frontend. The check-migration-conflicts pre-commit hook fails on the merge ref with "Duplicate migration number(s): 000065". Renumber to 000068 (one past base's highest, 000067) so the number is unique on the merge ref, and update the two stale in-file comment references from 000060 to 000068. * fix(marketplace): reconcile mock + permission tests and gocyclo after #804 rebase Rebasing the marketplace sell/cancel work (#292) onto the in-app revocation base (#804) left integration gaps that only surface once both feature sets coexist: - internal/analytics/collector_test.go declared GetPurchaseHistoryByPurchaseID twice: #804 added the func-override version and the marketplace commit added a no-op stub. Drop the marketplace stub; keep the override and the unique UpdatePurchaseHistoryListing stub so the mock builds. - frontend permissions.test.ts asserted the user role grants 12 verbs; the merged DefaultUserPermissions now grants 13 (revoke-own from #804 plus sell-own from #292). Add sell-own:purchases to the expected set. - store_postgres.go: merging the revocation columns (#290) into the marketplace gocyclo refactor pushed scanPurchaseHistoryRow and GetPurchaseHistoryByPurchaseID back over the cyclomatic budget. Extract the shared NULL reconciliation into a purchaseHistoryNullables struct + applyTo, so both readers stay under 10 and the NULL handling lives in one place. The conflict resolution itself (handlers, store SELECT/scan column order, auth defaults, mocks, frontend history buttons) was applied additively during the rebase; both feature sets are independent (marketplace sell/cancel vs revocation). Marketplace migration stays at 000068 (free gap vs the new base, which tops out at 000070). * fix(marketplace): gate sell actions on sell verbs and prorate default price (CR #808) Address CodeRabbit findings on the issue #292 marketplace sell/cancel flow: - Gate canSellOnMarketplace / canCancelMarketplaceListing on the sell-own / sell-any (or admin) verbs instead of bare sign-in, so the UX gate matches the backend authorizeSessionSell and avoids frontend/backend auth drift. Adds sell-own / sell-any to the Action union, mirroring the backend ActionSellOwn / ActionSellAny constants. - Prorate the upfront cost to its residual value in the sell modal's default price estimate (upfront * remaining/term), matching the backend resolveMarketplacePriceSchedule. Using the full upfront overstated the listing value for partially elapsed RIs. Also correct stale "migration 000060" comments (the columns ship in 000068) and add pgxmock coverage for GetPurchaseHistoryByPurchaseID asserting its distinct 25-column scan order (revocation_in_flight at position 22). * fix(marketplace): name fee/discount constants and restore gocyclo baseline - Introduce awsMarketplaceFeePercent (12), awsMarketplaceNetFactor (0.88), and awsMarketplaceBuyerDiscountFactor (0.95) as named Go constants in handler_marketplace.go; replace the matching TS literals with AWS_MARKETPLACE_FEE_PERCENT / AWS_MARKETPLACE_NET_FACTOR / AWS_MARKETPLACE_BUYER_DISCOUNT in history.ts. Eliminates silent ratios on a money-affecting display path per the no-hardcoded-magic-values rule. - Restore store_postgres_recommendations.go to main's lower-complexity form (recEffectiveSavingsPct + recOnDemandBaseline split); the rebase picked up an inlined variant from an earlier PR commit that cyclomatic-scored 12. * refactor(api): derive awsMarketplaceNetFactor from fee percent Replace the hardcoded 0.88 literal with a computed constant expression (1 - awsMarketplaceFeePercent/100.0) so the net factor stays in sync with the fee percent automatically. The Go constant evaluator uses arbitrary precision, so the resulting float64 value is identical to the literal it replaces; no money-math change. * fix(marketplace): guard concurrent RI listing creates with atomic slot claim Two concurrent marketplace-list requests for the same RI could both pass the read-only listing_state check in validateMarketplaceListRequest and both call AWS CreateReservedInstancesListing (each with a fresh ClientToken, so AWS does not dedup them), leaving two live listings for one RI and overwriting the DB row. Reserve the listing slot with a single atomic conditional UPDATE (ClaimMarketplaceListingSlot, modeled on FlipPurchaseRevocationInFlight) before the AWS call so exactly one racing request proceeds; the loser gets a 409. The claim is released back to the prior state on every post-claim failure path so a failed attempt never leaves the row stuck in the transient pending state. Also on this money path: - reject an implausibly large purchase count instead of silently truncating it into int32 (gosec G115) so the wrong number of RIs can never be listed; - add listing-state constants matching the AWS ListingStatus enum and use them in place of scattered string literals; - fix US-locale spellings flagged by misspell in comments and log/error text. Adds regression tests reproducing the concurrent-create race (claim loses -> 409, no AWS call) and the claim-error path, plus release-on-failure assertions. Closes #292 CR concurrent-create finding. * fix(marketplace): resolve 3 blocking defects in #808 RI sell flow Defect 1 -- migration number collision: Renumber marketplace migration from 000068 (already deployed, skipped by golang-migrate on prod) to 000084 (next free after main's 000083). Verified: main tops at 000083; #1277 uses 000082; no open PR holds 000084. All "000068" references in SQL comments and types.go updated to 000084. Defect 2 -- offering_class never written: CUDly's EC2 client hardwires OfferingClassTypeConvertible; savePurchaseHistory never stamped the field, so every row landed with offering_class=NULL and the Sell button never rendered. Fix: (a) stamp offering_class='convertible' in savePurchaseHistory for all AWS EC2 purchases so future CUDly-bought rows are correctly classified at write time; (b) add FetchOfferingClass to the marketplaceEC2Client interface and implement it via DescribeReservedInstances so the marketplace-list handler can lazily populate offering_class for pre-migration rows and for externally-created Standard RIs (purchased before CUDly existed); (c) add StampOfferingClass to ConfigStore + PostgresStore + mock to persist the fetched class so subsequent requests skip the extra AWS call; (d) a new populateOfferingClass helper in the handler ties it together -- it runs when offering_class is empty after validateMarketplaceListRequest and before the definitive "standard only" gate. How a 'standard' row comes to exist: an externally-created Standard RI will get its offering_class populated from AWS DescribeReservedInstances on the first POST .../marketplace-list call and the value persisted for future calls. Rows purchased by CUDly are stamped 'convertible' at write time (CUDly only ever buys Convertible EC2 RIs). Defect 3 -- $0 price guardrail gap: resolveMarketplacePriceSchedule accepted Price >= 0, allowing zero-dollar listings. Fix: reject Price <= 0 with a clear error; add a named constant awsMarketplaceMinPriceFloorFraction (5%) and a total-schedule floor check so a schedule that sums to less than 5% of the prorated residual value is also rejected with an explicit message. Extract floor check into checkSuppliedScheduleFloor and the class-check predicate into isKnownNonStandardOfferingClass to keep all touched functions below gocyclo 10. Regression tests: - TestMigration084_MarketplaceColumns: integration test proves the three new columns exist and round-trip after migrating a fresh DB through 000084. - TestMarketplaceList_EmptyOfferingClassFetchedStandard: proves an externally-created Standard RI (offering_class="") is listable after the lazy-populate path fetches and stamps 'standard' from AWS. - TestMarketplaceList_EmptyOfferingClassFetchedConvertible: proves the gate still rejects when AWS reports 'convertible' even if DB had no class. - TestResolveMarketplacePriceSchedule_ZeroPriceRejected: Price=0 rejected. - TestResolveMarketplacePriceSchedule_BelowFloorRejected: sub-floor rejected. * fix(marketplace): renumber migration 000084 -> 000085 to avoid #1277 collision PR #1277 (cancelled->canceled rename) was concurrently renumbered to 000084 and merges before #808. Once it lands, main will hold 000084, so #808's 000084 would collide on the merge ref (pre-commit --all-files migration check) and give golang-migrate two version-84 migrations. Since #1277 takes the lower number and merges first, #808 moves to 000085. - git mv the up/down/test migration files 000084_* -> 000085_* - update the "-- Migration" header in the up.sql and "-- Revert migration" in the down.sql to 000085 - update every 000084 reference in comments: config/types.go (x3), config/interfaces.go, config/store_postgres.go, api/handler_marketplace.go (x2), providers/aws/services/ec2/client.go - rename TestMigration084_MarketplaceColumns -> TestMigration085_* and its fixture identifiers (ri-085-test, ril-085-test) Also fix the integration-test fixture surfaced by running it against a real testcontainer PG: plan_id is a UUID FK, so the literal 'plan-085' failed with SQLSTATE 22P02. The INSERT now lists only NOT-NULL-without-default columns plus the three new marketplace columns and omits plan_id/plan_name/ramp_step/ source (nullable or defaulted), so the test needs no purchase_plans fixture. Verified: migrations apply cleanly through version 85 and the three columns round-trip. * fix(marketplace): correct RI listing price-floor math and reachability Round-3 money-path review fixes for the Sell-on-Marketplace flow (#808). Price floor (was arithmetically wrong): - Enforce the floor PER TIER on the one-time sale Price. AWS PriceScheduleSpecification.Price is the lump-sum a buyer pays when TermMonths remain, not a per-month rate; the old code compared Price*TermMonths against the floor, over-crediting a cheap schedule up to ~term-fold so a $5 tier passed a $60 floor on a $1,200 residual. - Reject any tier whose TermMonths exceeds the RI's remaining months (was unvalidated and arbitrarily inflatable). - Per-instance basis: purchase_history.UpfrontCost is the row total for all Count instances, but Marketplace prices are per instance, so divide the residual by Count in both the floor and the default schedule. - Upfront-only residual: drop recurring (monthly) cost from the residual basis. In the RI Marketplace the buyer assumes recurring charges after transfer, so the seller recovers only the upfront remainder; including recurring overpriced partial-upfront RIs and made no-upfront RIs unpriceable. - Extract marketplaceResidualPerUnit as the shared per-unit basis so the default schedule and the floor cannot drift. Reachability: render the Sell button for completed AWS EC2 rows whose offering_class is empty (unknown), not just "standard". CUDly stamps "convertible" on its own EC2 purchases and externally-created Standard RIs arrive with an empty class until the backend lazily populates it; gating the UI on "standard" alone made the feature unreachable end to end. Provider and service are now checked so unknown-class non-EC2 rows never show the button. Nits: - Use SDK enum constants string(ec2types.OfferingClassType{Standard,Convertible}) instead of raw "standard"/"convertible" literals in the offering-class gate, the isKnownNonStandardOfferingClass predicate, and the purchase-history stamp. - Route populateOfferingClass errors through mapAWSMarketplaceError so an AWS client fault (e.g. InvalidReservedInstancesID.NotFound) surfaces as a 400, not a generic 500. Migration: renumber 000085 -> 000087 to stay above #1422's 000086 (main tops at pre-84; #1422 takes 000086, so this PR takes 000087). Regression tests (fail-before / pass-after verified): - TestResolveMarketplacePriceSchedule_BelowFloorRejected: a {12mo, $5} tier on a $1,200 per-unit residual (Count=1) is now rejected; passed under the old Price*TermMonths floor. - TestResolveMarketplacePriceSchedule_PerUnitFloor: on a $1,200 row-total with Count=3 the per-unit floor is $20, so $19 is rejected and $21 accepted. - TestResolveMarketplacePriceSchedule_TermExceedsRemainingRejected: a tier term beyond the remaining months is rejected. - history-marketplace-sell-button.test.ts: the Sell button renders for a completed AWS EC2 row with an empty offering_class and stays hidden for convertible, non-EC2, non-AWS, active-listing, and anonymous cases. - TestMigration087_MarketplaceColumns: fresh DB migrates cleanly through 000087 and the three columns round-trip.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
approve-anyorapprove-ownpermission -- ownership alone was incorrectly treated as sufficient authorization.canApprovePendingRow()inhistory.tsnow requires an explicitapprove-own(orapprove-any) permission;DefaultUserPermissions()no longer grantsapprove-own:purchases; migration 000084 removes it from the seeded Standard Users group.Files changed
frontend/src/history.tscanAccess('approve-own', 'purchases')gate before ownership checkfrontend/src/permissions.generated.tsapprove-own:purchasesfrom USER_PERMS (regenerated)internal/auth/types.goapprove-own:purchasesfromDefaultUserPermissions()internal/database/postgres/migrations/000084_*.sqlfrontend/src/__tests__/history-approve-button.test.tsinternal/api/handler_purchases_test.gointernal/auth/types_test.gointernal/auth/service_group_test.gofrontend/src/__tests__/permissions.test.tsSecurity regression tests (fail-before / pass-after)
user without any approve permission sees NO Approve button even on their own rows (issue #1407 four-eyes)--src/__tests__/history-approve-button.test.tsTestHandler_approvePurchase_RejectsCreatorWithoutApprovePermission--internal/api/handler_purchases_test.go(asserts 403 when creator calls approve withoutapprove-anyorapprove-own)TestDefaultPermissions--internal/auth/types_test.goTest plan
go build ./...-- exit 0go test ./internal/auth/... ./internal/api/...-- 2247 passedgo test ./...-- 4880 passednpm test(frontend) -- 2564 passed, 78 suitesgolangci-lint run --new-from-rev=origin/main-- exit 0, no new issuesgocyclo -over 10-- no new functions from this change