Skip to content

fix(admin): allow pricing recalculation without master key#291

Merged
SantiagoDePolonia merged 1 commit into
mainfrom
fix/make-recalculating-without-a-key
Apr 29, 2026
Merged

fix(admin): allow pricing recalculation without master key#291
SantiagoDePolonia merged 1 commit into
mainfrom
fix/make-recalculating-without-a-key

Conversation

@SantiagoDePolonia

@SantiagoDePolonia SantiagoDePolonia commented Apr 29, 2026

Copy link
Copy Markdown
Contributor

Summary

  • make usage pricing recalculation follow the same admin API auth policy as other admin endpoints
  • remove the route-only auth override that blocked recalculation in no-master-key unsafe mode
  • add component, e2e, integration, and release-e2e coverage for no-master-key recalculation

Verification

  • curl sweep against isolated binary with GOMODEL_MASTER_KEY unset before and after the fix
  • go test ./cmd/... ./internal/... ./config/... -count=1
  • go test -tags=e2e ./tests/e2e/... -count=1
  • go test -tags=integration ./tests/integration/... -count=1
  • tests/e2e/run-release-e2e.sh --list
  • pre-commit hook: make test-race, go mod tidy, hot-path performance guard, make lint

Summary by CodeRabbit

  • Bug Fixes

    • The admin usage pricing recalculation endpoint no longer requires authentication when the master key is unset, allowing automatic price adjustments to be processed without authorization headers.
  • Tests

    • Added end-to-end and integration tests to validate pricing recalculation functionality and verify correct authentication behavior across different configuration states.

@greptile-apps

greptile-apps Bot commented Apr 29, 2026

Copy link
Copy Markdown

Greptile Summary

This PR removes the RequireConfiguredAuthMiddleware override on POST /admin/api/v1/usage/recalculate-pricing, making it follow the same auth policy as all other admin API endpoints: no auth required when GOMODEL_MASTER_KEY is unset, and standard bearer-token auth required when a master key is configured. Test coverage is added at the component, e2e, integration, and release-e2e levels to document the new behavior.

Confidence Score: 5/5

Safe to merge — the change is a consistent application of the existing admin auth policy with thorough multi-layer test coverage.

All findings are P2 or absent. The core logic change is minimal (one route argument removed, one function deleted) and is already the pattern every other admin endpoint follows. Tests cover both the no-master-key open path and the master-key authenticated path at unit, e2e, integration, and release-e2e levels.

No files require special attention.

Important Files Changed

Filename Overview
internal/server/auth.go Removes the RequireConfiguredAuthMiddleware function — its logic was a per-route override that is now obsolete.
internal/server/http.go Drops the per-route middleware argument on recalculate-pricing; the endpoint now relies solely on the global admin auth policy.
internal/server/http_test.go Replaces the old 401-expectation test with two new tests: one verifying unauthenticated access succeeds when no master key is set, another verifying 401/200 behaviour when a master key is configured.
tests/e2e/admin_test.go Adds TestAdminAPI_PricingRecalculationNoMasterKey_E2E exercising the endpoint through an in-process httptest server without a master key.
tests/integration/admin_test.go Adds TestAdminPricingRecalculationNoMasterKey_PostgreSQL testing the full stack against a real PostgreSQL instance with usage tracking enabled.
tests/integration/setup_test.go Adds UsagePricingRecalculationEnabled to TestServerConfig and threads it through to config.UsageConfig.PricingRecalculationEnabled.
tests/e2e/release-e2e-scenarios.md Documents scenario S90: a curl sweep against the no-master-key gateway to verify unauthenticated pricing recalculation.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[POST /admin/api/v1/usage/recalculate-pricing] --> B{MasterKey == empty?}
    B -- Yes --> C[Add /admin/api/v1/* to skip paths]
    C --> D[Path matches skip list → allow]
    D --> H[RecalculateUsagePricing handler]
    B -- No --> E{Auth header present?}
    E -- No --> F[401 Unauthorized]
    E -- Yes --> G{Token valid?}
    G -- No --> F
    G -- Yes --> H
    H --> I[200 OK]
Loading

Reviews (1): Last reviewed commit: "fix(admin): allow pricing recalculation ..." | Re-trigger Greptile

@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@coderabbitai

coderabbitai Bot commented Apr 29, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: dd7d20aa-0740-49cb-9fe7-ac78a4f66a94

📥 Commits

Reviewing files that changed from the base of the PR and between d5b4936 and 8757c81.

📒 Files selected for processing (7)
  • internal/server/auth.go
  • internal/server/http.go
  • internal/server/http_test.go
  • tests/e2e/admin_test.go
  • tests/e2e/release-e2e-scenarios.md
  • tests/integration/admin_test.go
  • tests/integration/setup_test.go
💤 Files with no reviewable changes (1)
  • internal/server/auth.go

📝 Walkthrough

Walkthrough

The PR removes the RequireConfiguredAuthMiddleware authentication wrapper and updates the admin pricing recalculation endpoint to bypass authentication enforcement when the master key is not configured, allowing unauthenticated requests in that scenario. Supporting unit, integration, and E2E tests validate the new behavior.

Changes

Cohort / File(s) Summary
Authentication middleware removal
internal/server/auth.go
Deletes the RequireConfiguredAuthMiddleware function that explicitly enforced authentication on routes regardless of global skip paths.
Route protection changes
internal/server/http.go
Updates the admin pricing recalculation endpoint to be mounted directly without the removed middleware, altering authentication requirements for this endpoint.
Unit test refactoring
internal/server/http_test.go
Replaces single 401-expectation test with dual test cases using a pricing recalculator stub; validates both unauthenticated (no master key) and authenticated (with master key) access paths and recalculation invocations.
E2E and integration tests
tests/e2e/admin_test.go, tests/integration/admin_test.go
Adds new test suite entries: E2E admin test with stubbed pricing recalculator verifying 200 response and correct invocation count; PostgreSQL integration test validating end-to-end recalculation flow without master key protection.
Test scenario documentation
tests/e2e/release-e2e-scenarios.md
Documents new E2E scenario S90 validating pricing recalculation API without Authorization header.
Test configuration
tests/integration/setup_test.go
Adds UsagePricingRecalculationEnabled flag to TestServerConfig and wires it into application config.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

release:internal

Poem

🐰 An auth wall once guarded each door,
But now some guests needn't knock anymore,
The middleware vanished with a swift sweep,
Pricing recalc flows, no secrets to keep,
Tests bloom like carrots to verify all!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and concisely describes the main change: removing authentication requirement for the pricing recalculation endpoint when no master key is configured, allowing it to work in no-master-key mode.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/make-recalculating-without-a-key

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

@SantiagoDePolonia SantiagoDePolonia merged commit 89ce22e into main Apr 29, 2026
19 checks passed
@SantiagoDePolonia SantiagoDePolonia deleted the fix/make-recalculating-without-a-key branch June 11, 2026 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants