[SimEdit]: Define edit auth policy for simulation updates#229
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an explicit “managed-content edit” authorization policy across SimBoard by
introducing an editor role, persisting verified E3SM GitHub org membership
state on users, enforcing edit gating on simulation update routes, and updating
frontend UX to reflect read-only vs editable states.
Changes:
- Backend: persist E3SM org membership verification state, refresh it on GitHub
OAuth login (when determinable), and enforce a shared
can_edit_managed_contentpolicy for simulation PATCH updates. - Frontend: gate “Edit” affordances on
user.can_edit_managed_content, and show
clearer signed-out vs signed-in-but-readonly messaging. - Tests/docs: add policy unit tests, OAuth membership-check tests, and document
expected 401 vs 403 behavior for protected edits.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/types/user.ts | Extends the frontend User contract with membership + edit-capability flags. |
| frontend/src/features/simulations/SimulationDetailsPage.tsx | Switches edit gating from “authenticated” to server-provided can_edit_managed_content. |
| frontend/src/features/simulations/components/SimulationDetailsView.tsx | Adds UX for read-only messaging and a “Log In to Edit” action. |
| backend/app/features/user/models.py | Adds EDITOR role and persists membership verification fields on User. |
| backend/app/features/user/manager.py | Introduces shared can_edit_managed_content policy helper and membership refresh helper. |
| backend/app/features/user/schemas/user.py | Exposes has_verified_e3sm_membership and computed can_edit_managed_content in UserRead. |
| backend/app/features/user/auth/oauth.py | Requests the read:org scope needed for org membership verification. |
| backend/app/features/user/api/oauth.py | Fetches membership state during OAuth callback and persists it when conclusive. |
| backend/app/features/simulation/api.py | Enforces managed-content edit policy for simulation PATCH (403 for insufficient privileges). |
| backend/migrations/versions/20260611_090000_add_editor_role_and_github_membership_state.py | Adds editor enum value plus new persisted membership columns on users. |
| backend/README.md | Documents the managed-content authorization policy and expected HTTP statuses. |
| backend/tests/features/user/test_manager.py | Adds unit tests for can_edit_managed_content policy behavior. |
| backend/tests/features/user/schemas/test_user.py | Adds schema tests for computed can_edit_managed_content. |
| backend/tests/features/user/api/test_oauth.py | Adds test coverage for membership fetching + non-downgrade behavior on API failures. |
| backend/tests/features/simulation/test_api.py | Updates simulation PATCH tests to reflect new edit policy and expected 403s. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbe3e689e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
backend/app/features/user/schemas/user.py:31
passwordis declared asstrbut defaults toNone. This makes the runtime value/type inconsistent and produces an OpenAPI schema where the field is not nullable even though the comment says it is optional for OAuth. Use an explicit optional type instead.
# password is optional for OAuth.
password: Annotated[str | None, "The user's password"] = None
backend/app/features/user/api/oauth.py:140
- GitHub org membership is fetched before validating
account_email/state. That can trigger an unnecessary external GitHub API call on early-failure paths (missing email/state), increasing latency and rate-limit pressure. Fetch membership only after these basic validations pass.
token, state = access_token_state
account_id, account_email = await GITHUB_OAUTH_CLIENT.get_id_email(
token["access_token"]
)
if account_email is None:
raise HTTPException(
Description
Simplifies managed-content edit authorization so normal users can edit when they have verified E3SM GitHub organization membership, while admins retain access and service accounts remain read-only.
What Changed
Checklist
Validation
make backend-testcd backend && .venv/bin/pytest tests/features/user/api/test_oauth.py tests/features/user/test_manager.py --cov=app.features.user.api.oauth --cov=app.features.user.manager --cov-report=term-missingDeployment Notes
USER.