fix: correct admin privilege escalation via team membership#3418
Open
Ziinc wants to merge 18 commits into
Open
fix: correct admin privilege escalation via team membership#3418Ziinc wants to merge 18 commits into
Ziinc wants to merge 18 commits into
Conversation
The query was joining Team and TeamUser, allowing any team member of an admin user to pass the admin check. Admin status is now determined solely by the authenticated user's own admin flag. https://claude.ai/code/session_01UmrctTfmxLZVgEq91EKfGb
Adds Admin.grant_admin/1, a POST route, controller action, and a "make admin" button (with browser confirm dialog) next to the existing "become" button for each non-admin account in /admin/accounts. https://claude.ai/code/session_01UmrctTfmxLZVgEq91EKfGb
Adds defense-in-depth: the business logic now verifies the granter has
admin: true, returning {:error, :unauthorized} if not. Combines the two
idempotency tests and adds a non-admin granter test.
https://claude.ai/code/session_01UmrctTfmxLZVgEq91EKfGb
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
…seError
Users.get/1 returns nil for unknown IDs. The nil clause now returns
{:error, :not_found} so the controller's error branch is always reached.
https://claude.ai/code/session_01UmrctTfmxLZVgEq91EKfGb
- Guard become_account against nil user (prevents FunctionClauseError on unknown IDs) - Add revoke_admin/2 with self-revocation guard, route, controller action, and UI button - Add audit logging (granter/target IDs + emails) to grant_admin, revoke_admin, become_account - Re-fetch granter from DB in grant_admin/revoke_admin controller actions to prevent stale-cache bypass https://claude.ai/code/session_01UmrctTfmxLZVgEq91EKfGb
- Add AdminAuth.on_mount/4 that re-checks Admin.admin?/1 from the
session email on every LiveView mount/reconnect, closing the window
where a revoked admin could still trigger ClusterLive shutdown events
- Wrap /admin/cluster and /admin/partner in live_session :admin using
@common_on_mount_hooks ++ @admin_hooks
- Add nil-granter clauses to grant_admin/2 and revoke_admin/2 so a
deleted-user granter returns {:error, :not_found} instead of crashing
- Handle {:error, :not_found} explicitly in both controller actions with
an "Account not found." flash rather than the generic error
https://claude.ai/code/session_01UmrctTfmxLZVgEq91EKfGb
Adds a count_admins/0 check before revoking — returns {:error, :last_admin}
when only one admin remains, preventing a full UI lockout. The controller
surfaces this as "Cannot revoke the last admin account."
https://claude.ai/code/session_01UmrctTfmxLZVgEq91EKfGb
- Controller: replace Users.get(conn.assigns.user.id) with Users.get_by(email: get_session(conn, :current_email)) in grant_admin and revoke_admin so the acting admin is always the session owner, not the team-context owner that SetTeamContext places in conn.assigns.user - Admin.revoke_admin/2: replace non-atomic count_admins()+Repo.update() with a transaction that locks all admin rows (FOR UPDATE) before counting, preventing two concurrent revocations from both observing count >= 2 and both committing https://claude.ai/code/session_01UmrctTfmxLZVgEq91EKfGb
…fies granter in DB Two security fixes: 1. become_account logged conn.assigns.user (set by SetTeamContext to the team owner) instead of the actual logged-in admin. Audit trails could attribute impersonation to an unrelated user. Now fetches the actor from the session email, consistent with grant_admin / revoke_admin. 2. revoke_admin passed an in-memory granter struct to the transaction. A window existed between Users.get_by/1 and the transaction where the granter's admin flag could be revoked, allowing a now-revoked admin to still revoke others. The transaction now re-checks granter's admin status from the locked DB snapshot before proceeding. https://claude.ai/code/session_01UmrctTfmxLZVgEq91EKfGb
Mirrors the pattern from revoke_admin: locks the granter's row with
FOR UPDATE before updating, so a revocation that races between the
controller's Users.get_by fetch and the Repo.update is detected and
rejected with {:error, :unauthorized} rather than silently succeeding.
https://claude.ai/code/session_01UmrctTfmxLZVgEq91EKfGb
…_admin
Logger is not transactional. Previously, audit entries were written before
Repo.update/1, so a failed update (or rollback) would leave a permanent log
record of a privilege change that never persisted to the DB. Moved both
Logger.info calls into the {:ok, user} branch so they only fire when the
database write succeeds.
https://claude.ai/code/session_01UmrctTfmxLZVgEq91EKfGb
| |> delete_session(:last_switched_team_id) | ||
| |> delete_resp_cookie("_logflare_user_id") | ||
| |> delete_resp_cookie("_logflare_team_user_id") | ||
| case Users.get(id) do |
Contributor
There was a problem hiding this comment.
🟡 Severity: MEDIUM
The become_account action replaces an admin's session with an arbitrary user's session but is routed via GET. Phoenix's protect_from_forgery only covers non-GET verbs, and no SameSite cookie attribute is configured. An attacker can trigger impersonation by embedding <img src="/admin/accounts/VICTIM_ID/become"> on any page an authenticated admin visits.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: Change the become_account route from GET to POST so that Phoenix's built-in CSRF protection applies to this state-mutating action. This requires two coordinated changes:
-
In
lib/logflare_web/router.ex(line 382), change:get("/accounts/:id/become", AdminController, :become_account)
to:
post("/accounts/:id/become", AdminController, :become_account)
-
In
lib/logflare_web/templates/admin/accounts.html.heex(line 35), addmethod: :postto the link helper so Phoenix automatically generates an inline form with a CSRF token:<small>{link("become", to: Routes.admin_path(@conn, :become_account, account.id), method: :post, data: [confirm: "Impersonate #{account.email}?"], class: "btn btn-danger btn-sm")}</small>
Using method: :post in Phoenix's link/2 helper causes it to render a small <form> with a hidden _csrf_token field, which means the protect_from_forgery plug will verify the token on every impersonation request. This prevents cross-site GET-based CSRF attacks (e.g., via <img src="/admin/accounts/VICTIM_ID/become">).
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
2 participants
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.
This PR tightens up admin security
metadata.auditkeypre-merge todos:
closes PRODSEC-36
https://claude.ai/code/session_01UmrctTfmxLZVgEq91EKfGb