Skip to content

sec(api): restrict SourceIdentity to admin sessions in GET /api/config (closes #407) - #530

Merged
cristim merged 2 commits into
feat/multicloud-web-frontendfrom
sec/config-cloud-identity-admin-only
Jun 6, 2026
Merged

sec(api): restrict SourceIdentity to admin sessions in GET /api/config (closes #407)#530
cristim merged 2 commits into
feat/multicloud-web-frontendfrom
sec/config-cloud-identity-admin-only

Conversation

@cristim

@cristim cristim commented May 19, 2026

Copy link
Copy Markdown
Member

Summary

  • GET /api/config returned SourceIdentity (AWS account ID, Azure tenant ID/subscription ID, GCP project ID) to any authenticated user. Non-admin roles should not see the host cloud identity.
  • Change getConfig to accept the request and call requireAdmin. Only populate SourceIdentity for admin sessions; non-admin responses get SourceIdentity: nil.
  • SourceCloud (the plain string "aws"/"azure"/"gcp") is still returned to all authenticated users as it is needed for dashboard feature gating.
  • Update the router wrapper and all test call sites; add regression tests confirming non-admin sessions receive nil SourceIdentity.

Test plan

  • go test ./internal/api/... ./internal/auth/... passes (1572 tests)
  • TestHandler_getConfig_SourceIdentity_AdminOnly/regression_#407 asserts result.SourceIdentity == nil for non-admin
  • Pre-commit hooks pass

Closes #407

Summary by CodeRabbit

Release Notes

  • Bug Fixes
    • Configuration endpoint now properly restricts sensitive identity information to authorized admin users only. Non-admin users will no longer have access to identity details in configuration responses, improving security and enforcing proper permission boundaries.

Review Change Stack

@coderabbitai

coderabbitai Bot commented May 19, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@cristim, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 21 minutes and 57 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2755dc69-4ff9-4475-84c3-5ed5d4a742cc

📥 Commits

Reviewing files that changed from the base of the PR and between 1302b71 and 4141366.

📒 Files selected for processing (3)
  • internal/api/handler_config.go
  • internal/api/handler_config_test.go
  • internal/api/router.go
📝 Walkthrough

Walkthrough

The handler getConfig is updated to accept the incoming Lambda request parameter alongside context, enabling request-scoped permission checks. SourceIdentity is now conditionally populated only for admin users via requireAdmin(ctx, req). The router adapter and all tests are updated to pass the request object through the call chain.

Changes

Admin-only SourceIdentity for config endpoint

Layer / File(s) Summary
Handler signature and conditional SourceIdentity
internal/api/handler_config.go, internal/api/router.go
Handler getConfig signature adds req parameter; response construction conditionally sets SourceIdentity only when requireAdmin(ctx, req) succeeds. Router adapter forwards the request to the updated handler.
Test updates and SourceIdentity_AdminOnly regression test
internal/api/handler_config_test.go
Primary test setup constructs MockAuthService and authenticated LambdaFunctionURLRequest; test calls are updated to pass the request. Error case tests use empty requests. New TestHandler_getConfig_SourceIdentity_AdminOnly regression test verifies SourceIdentity is populated for admin sessions and nil for non-admin sessions.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • LeanerCloud/CUDly#146: Modifies SourceIdentity production by adding AWS partition parsing alongside this PR's conditional SourceIdentity gating for non-admin requests.

Poem

🔐 Request comes in, request goes through,
Admin check decides what's overdue—
SourceIdentity: yours if admin ring true,
else vanish away in the shadows of blue. 🐰

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main security change: restricting SourceIdentity field to admin sessions in the GET /api/config endpoint, directly addressing issue #407.
Linked Issues check ✅ Passed The PR implements the minimal fix approach from issue #407 by conditionally populating SourceIdentity only for admin sessions via requireAdmin check, achieving the primary objective of preventing non-admin access to host cloud identity.
Out of Scope Changes check ✅ Passed All code changes are directly related to the stated objective: handler method signature updates, conditional SourceIdentity population, test updates, and router adapter changes—all necessary to implement the security restriction.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch sec/config-cloud-identity-admin-only

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

@cristim cristim added priority/p2 Backlog-worthy severity/medium Moderate harm urgency/this-sprint Within the current sprint impact/internal Team-internal only effort/xs Trivial / one-liner type/security Security finding triaged Item has been triaged labels May 19, 2026
@cristim

cristim commented May 19, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 19, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim

cristim commented May 19, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 19, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim

cristim commented May 20, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Full review triggered.

@cristim

cristim commented May 22, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 22, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim

cristim commented May 22, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 22, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim

cristim commented May 27, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 27, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim

cristim commented May 27, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented May 27, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Full review triggered.

@cristim

cristim commented May 27, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented May 27, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Full review triggered.

@cristim

cristim commented May 28, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented May 28, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Full review triggered.

@cristim

cristim commented May 30, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented May 30, 2026

Copy link
Copy Markdown
Contributor

Rate Limit Exceeded

@cristim have exceeded the limit for the number of chat messages per hour. Please wait 54 minutes and 6 seconds before sending another message.

@cristim

cristim commented Jun 4, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

cristim added 2 commits June 5, 2026 12:44
closes #407)

GET /api/config returned SourceIdentity (AWS account ID, Azure tenant ID /
subscription ID, GCP project ID) to any authenticated user. Non-admin
roles should not have access to the host cloud identity.

Change getConfig to accept the request so it can call requireAdmin. Only
populate SourceIdentity when the caller is an admin; non-admin responses
get SourceIdentity: nil. SourceCloud (the cloud name string "aws"/"azure")
is still returned to all authenticated users as it is needed by the
dashboard for feature gating.

Update all call sites (router wrapper, existing tests). Add regression
tests confirming non-admin sessions receive a nil SourceIdentity.
…ionAPI for admin gate, assert SourceIdentity admin-only

- Remove Session.Role field from all Session literals in handler_config_test.go
  (Role was removed in #907/#912; role-based short-circuit replaced by group
  membership via HasPermissionAPI)
- Add mockAuth.grantAdmin() to TestHandler_getConfig so the admin permission
  check in requireAdmin is satisfied
- Add explicit HasPermissionAPI stub for "admin-user" in
  TestHandler_getConfig_SourceIdentity_AdminOnly (required now that the admin
  path goes through HasPermissionAPI, not a Role field)
- Replace no-op `_ = result.SourceIdentity` with require.NotNil assertion
  (admin sessions must receive SourceIdentity per issue #407)
- Non-admin subtest already asserts SourceIdentity == nil via assert.Nil
@cristim
cristim force-pushed the sec/config-cloud-identity-admin-only branch from 1302b71 to 4141366 Compare June 5, 2026 10:45
@cristim

cristim commented Jun 5, 2026

Copy link
Copy Markdown
Member Author

Fix commit summary (4141366):

Gap 1 - Removed Session.Role fields: Dropped Role: "admin" from the adminSession literal in TestHandler_getConfig (line ~22) and removed Role: "admin" / Role: "user" from both session literals in TestHandler_getConfig_SourceIdentity_AdminOnly (lines ~374-375). The Role field was removed from api.Session in #907/#912 and the compiler rejected it.

Gap 2 - HasPermissionAPI stubs for admin gate: Added mockAuth.grantAdmin() to TestHandler_getConfig so the requireAdmin call (which now routes through HasPermissionAPI(admin, *) after #907) is satisfied. Added a targeted mockAuth.On("HasPermissionAPI", mock.Anything, "admin-user", ...) stub in TestHandler_getConfig_SourceIdentity_AdminOnly to cover the admin subtest path. The non-admin subtest already had its .Return(false, nil) stub.

Gap 3 - Assert the invariant: Replaced the no-op _ = result.SourceIdentity in the admin subtest with require.NotNil(t, result.SourceIdentity) to positively assert that admins receive the field. The non-admin subtest already asserts result.SourceIdentity == nil.

Verification: go vet ./internal/api/ clean, gofmt -l clean, go build ./... clean, go test ./internal/api/ -count=1 1461/1461 passed, SKIP=terraform_validate pre-commit run --all-files all passed.

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

@cristim: The fix commit looks solid. All three gaps are well-addressed:

1461/1461 passing with a clean pre-commit run is a good signal. Triggering a fresh review now.


🧠 Learnings used
Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-05T15:44:54.463Z
Learning: In the LeanerCloud/CUDly repository, when a CodeRabbit review flags something as "outside-diff" that was already implemented in an earlier commit of the same PR, it is likely a stale-snapshot artifact where the review diff was generated against a pre-fix view. The inline comment convention `(CR pass on PR #<number> Finding N)` is used by the maintainer (cristim) to mark lines that were changed in direct response to a prior CodeRabbit review pass.
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim
cristim merged commit ede61ad into feat/multicloud-web-frontend Jun 6, 2026
4 checks passed
@cristim
cristim deleted the sec/config-cloud-identity-admin-only branch July 27, 2026 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/xs Trivial / one-liner impact/internal Team-internal only priority/p2 Backlog-worthy severity/medium Moderate harm triaged Item has been triaged type/security Security finding urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant