diff --git a/crates/manager-core/src/api.rs b/crates/manager-core/src/api.rs index 72965bf..e79a798 100644 --- a/crates/manager-core/src/api.rs +++ b/crates/manager-core/src/api.rs @@ -270,10 +270,13 @@ async fn delete_script( Path(id): Path, ) -> Result { let script = state.repo.get(id).await?.ok_or(ApiError::NotFound(id))?; + // Delete is gated tighter than Save: editors can edit scripts but + // only app_admin / instance admin / owner can remove them. See + // blueprint §11.6. require( state.authz.as_ref(), &principal, - Capability::AppWriteScript(script.app_id), + Capability::AppAdmin(script.app_id), ) .await?; state.repo.delete(id).await?; diff --git a/crates/manager-core/src/apps_api.rs b/crates/manager-core/src/apps_api.rs index 9b7849d..7c2d103 100644 --- a/crates/manager-core/src/apps_api.rs +++ b/crates/manager-core/src/apps_api.rs @@ -143,8 +143,8 @@ pub struct AppLookupResponse { pub redirect_to: Option, /// The caller's role on this app, used by the dashboard to decide /// whether to render admin-only surfaces (Members tab, settings). - /// `Owner` maps to `app_admin`, `Admin` to `editor` (both implicit - /// per blueprint §11.6); `Member` carries its explicit + /// `Owner` and `Admin` both map to `app_admin` (implicit per + /// blueprint §11.6); `Member` carries its explicit /// `app_members.role`. pub my_role: Option, } @@ -226,16 +226,15 @@ async fn get_app( /// Compute the caller's effective `AppRole` on a specific app. Mirrors /// the implicit-grant logic in `authz::role_grants` but returns the /// role itself (for UI gating) rather than a yes/no decision. `Owner` -/// is implicit `AppAdmin` everywhere; `Admin` is implicit `Editor` -/// everywhere; `Member` consults `app_members`. +/// and `Admin` are both implicit `AppAdmin` everywhere; `Member` +/// consults `app_members`. async fn compute_my_role( authz: &dyn AuthzRepo, principal: &Principal, app_id: AppId, ) -> Result, AppsApiError> { match principal.instance_role { - InstanceRole::Owner => Ok(Some(AppRole::AppAdmin)), - InstanceRole::Admin => Ok(Some(AppRole::Editor)), + InstanceRole::Owner | InstanceRole::Admin => Ok(Some(AppRole::AppAdmin)), InstanceRole::Member => Ok(authz.membership(principal.user_id, app_id).await?), } } diff --git a/crates/manager-core/src/authz.rs b/crates/manager-core/src/authz.rs index d066030..1da5709 100644 --- a/crates/manager-core/src/authz.rs +++ b/crates/manager-core/src/authz.rs @@ -199,21 +199,14 @@ async fn role_grants( } } -/// Admin is implicit `editor` on every app (per blueprint §11.6). They -/// can create apps and manage users, but NOT touch instance-wide -/// settings or take app-admin-only actions on apps they're not -/// explicitly app_admin of. Everything not in this set falls through -/// to deny (`InstanceManageSettings`, `AppManageDomains`, `AppAdmin`). +/// Admin is implicit `app_admin` on every app (per blueprint §11.6). +/// They can create apps, manage users, and take any app-scoped action +/// on any app without an explicit `app_members` row — single-human +/// installs would otherwise need to add themselves to every new app. +/// Only `InstanceManageSettings` (sandbox ceiling, etc.) stays +/// owner-only. const fn admin_grants(cap: Capability) -> bool { - matches!( - cap, - Capability::InstanceCreateApp - | Capability::InstanceManageUsers - | Capability::AppRead(_) - | Capability::AppWriteScript(_) - | Capability::AppWriteRoute(_) - | Capability::AppLogRead(_) - ) + !matches!(cap, Capability::InstanceManageSettings) } /// Member has zero instance authority. App authority requires an @@ -357,10 +350,23 @@ mod tests { } #[tokio::test] - async fn admin_cannot_manage_instance_settings_or_app_admin_actions() { + async fn admin_cannot_manage_instance_settings() { + let repo = InMemoryAuthzRepo::default(); + let p = principal(InstanceRole::Admin); + assert_eq!( + can(&repo, &p, Capability::InstanceManageSettings) + .await + .unwrap(), + Decision::Deny, + ); + } + + #[tokio::test] + async fn admin_is_implicit_app_admin_on_every_app() { let repo = InMemoryAuthzRepo::default(); let p = principal(InstanceRole::Admin); let app = AppId::new(); + // Instance-scoped allowances. assert_eq!( can(&repo, &p, Capability::InstanceCreateApp).await.unwrap(), Decision::Allow, @@ -371,36 +377,22 @@ mod tests { .unwrap(), Decision::Allow, ); - assert_eq!( - can(&repo, &p, Capability::InstanceManageSettings) - .await - .unwrap(), - Decision::Deny, - ); - // Editor-like grants succeed - assert_eq!( - can(&repo, &p, Capability::AppWriteScript(app)) - .await - .unwrap(), - Decision::Allow, - ); - assert_eq!( - can(&repo, &p, Capability::AppWriteRoute(app)) - .await - .unwrap(), - Decision::Allow, - ); - // App-admin grants do not - assert_eq!( - can(&repo, &p, Capability::AppManageDomains(app)) - .await - .unwrap(), - Decision::Deny, - ); - assert_eq!( - can(&repo, &p, Capability::AppAdmin(app)).await.unwrap(), - Decision::Deny, - ); + // Editor-like + app-admin grants both succeed without any + // app_members row. + for cap in [ + Capability::AppRead(app), + Capability::AppWriteScript(app), + Capability::AppWriteRoute(app), + Capability::AppLogRead(app), + Capability::AppManageDomains(app), + Capability::AppAdmin(app), + ] { + assert_eq!( + can(&repo, &p, cap).await.unwrap(), + Decision::Allow, + "admin denied app-scoped capability {cap:?}" + ); + } } #[tokio::test] @@ -474,6 +466,29 @@ mod tests { ); } + /// Editors hold `AppWriteScript` (Save) but **not** `AppAdmin` + /// (Delete). The script-delete handler gates on the latter so the + /// API can't be tricked into letting an editor remove the script + /// they were only allowed to edit. + #[tokio::test] + async fn editor_can_write_scripts_but_not_delete_them() { + let repo = InMemoryAuthzRepo::default(); + let p = principal(InstanceRole::Member); + let app = AppId::new(); + repo.grant(p.user_id, app, AppRole::Editor).await; + + assert!(can(&repo, &p, Capability::AppWriteScript(app)) + .await + .unwrap() + .is_allow()); + // Delete is gated on AppAdmin in the handler — editors must be + // denied here for that gate to bite. + assert_eq!( + can(&repo, &p, Capability::AppAdmin(app)).await.unwrap(), + Decision::Deny, + ); + } + #[tokio::test] async fn member_with_app_admin_role_can_do_app_admin_actions() { let repo = InMemoryAuthzRepo::default(); diff --git a/crates/picloud/tests/authz.rs b/crates/picloud/tests/authz.rs index 4b0f1a6..ecb4040 100644 --- a/crates/picloud/tests/authz.rs +++ b/crates/picloud/tests/authz.rs @@ -293,7 +293,7 @@ async fn owner_access_matrix(pool: PgPool) { #[ignore = "needs DATABASE_URL pointing at a running Postgres"] #[sqlx::test(migrations = "../manager-core/migrations")] -async fn admin_can_manage_users_but_not_app_admin_settings(pool: PgPool) { +async fn admin_is_implicit_app_admin_on_every_app(pool: PgPool) { let s = boot(pool.clone()).await; seed_user(&s.pool, "alice", "alice-pw", InstanceRole::Admin).await; let token = login_token(&s.server, "alice", "alice-pw").await; @@ -305,24 +305,32 @@ async fn admin_can_manage_users_but_not_app_admin_settings(pool: PgPool) { .await .assert_status_ok(); - // Allowed: read default app (admin is implicit editor everywhere). + // Allowed: read default app — admin is implicit app_admin + // everywhere (per blueprint §11.6). s.server .get("/api/v1/admin/apps/default") .add_header("authorization", format!("Bearer {token}")) .await .assert_status_ok(); - // Allowed: write scripts (implicit editor). + // Allowed: write scripts. let script = create_script_via_api(&s.server, &token, s.default_app, "admin-write").await; assert!(script["id"].is_string()); - // Denied: delete the default app (AppAdmin only). - let denied = s - .server + // Allowed: list app members (AppAdmin gate). Pre-3.5.x this + // 403'd; now it's the same allow as the owner sees. + s.server + .get("/api/v1/admin/apps/default/members") + .add_header("authorization", format!("Bearer {token}")) + .await + .assert_status_ok(); + + // Allowed: delete the default app (AppAdmin). + s.server .delete("/api/v1/admin/apps/default") .add_header("authorization", format!("Bearer {token}")) - .await; - assert_eq!(denied.status_code(), axum::http::StatusCode::FORBIDDEN); + .await + .assert_status(axum::http::StatusCode::NO_CONTENT); } #[ignore = "needs DATABASE_URL pointing at a running Postgres"] @@ -735,7 +743,7 @@ async fn my_role_field_matches_caller_role(pool: PgPool) { "owner reports app_admin" ); - // Admin → implicit editor everywhere. + // Admin → implicit app_admin everywhere (post-§11.6 update). seed_user(&s.pool, "alice", "alice-pw", InstanceRole::Admin).await; let admin_token = login_token(&s.server, "alice", "alice-pw").await; let r = s @@ -746,8 +754,8 @@ async fn my_role_field_matches_caller_role(pool: PgPool) { r.assert_status_ok(); assert_eq!( r.json::()["my_role"].as_str(), - Some("editor"), - "admin reports editor" + Some("app_admin"), + "admin reports app_admin" ); // Member with explicit `viewer` membership → viewer. diff --git a/serverless_cloud_blueprint.md b/serverless_cloud_blueprint.md index fba8dc6..4dee843 100644 --- a/serverless_cloud_blueprint.md +++ b/serverless_cloud_blueprint.md @@ -1049,7 +1049,7 @@ pub struct Principal { | Role | Powers | |---|---| | `owner` | full instance control, manage other owners, implicit `app_admin` on every app. Multiple owners allowed. | -| `admin` | create apps, invite users, implicit `editor` on every app. Cannot manage instance-wide settings or other owners. | +| `admin` | create apps, invite users, implicit `app_admin` on every app. Cannot manage instance-wide settings (sandbox ceiling, etc.) or other owners. | | `member` | invited into specific apps only. Cannot create apps, cannot invite. **Strict isolation enforced at SQL** — list endpoints `WHERE app_id IN (SELECT app_id FROM app_members WHERE user_id = $1)`; the API never returns apps a member isn't part of. | The current Phase 3a `admin_users` rows all become `owner` via `DEFAULT 'owner'` on the new column. Multi-owner installs get a startup `tracing::warn!` listing the active owner usernames so the operator can demote extras via `PATCH /api/v1/admin/admins/{id}`. @@ -1058,11 +1058,13 @@ The current Phase 3a `admin_users` rows all become `owner` via `DEFAULT 'owner'` | Role | Grants | |---|---| -| `app_admin` | settings, domain claims, delete app | -| `editor` | CRUD on scripts, routes, sandbox config | +| `app_admin` | settings, domain claims, delete app, **delete scripts** | +| `editor` | create + edit scripts, routes, sandbox config (no script delete) | | `viewer` | read scripts + execution logs | -Implicit grants from instance role: every `owner` is `app_admin` on every app; every `admin` is `editor` on every app. Explicit `app_members` rows are the only path for `member` users. +Implicit grants from instance role: every `owner` and every `admin` is `app_admin` on every app — a single-human install would otherwise have to add itself to each new app's `app_members`. Explicit `app_members` rows are the only path for `member` users. + +Script **save** uses `AppWriteScript` (editor+); script **delete** uses `AppAdmin` (app_admin+). Editors can iterate on a script's source freely but cannot remove it — destructive cleanup stays with the role that also owns the app. ### Auth Methods — Same Principal, Different Extractor