diff --git a/docs/architecture/magic-link-auth.md b/docs/architecture/magic-link-auth.md index f4151eec..3528ead4 100644 --- a/docs/architecture/magic-link-auth.md +++ b/docs/architecture/magic-link-auth.md @@ -131,6 +131,10 @@ Every denial or rejection in the magic-link path emits a structured event on the | `auth.magic_link_redeem` | `redeemed`, `token_not_found`, `token_used`, `token_expired`, `account_deactivated` | `MagicLinkInviteService::redeem` | | `user_profile.rejected` | `external_no_relationship`, `target_external_hidden`, `target_hidden` | `AuthApplicationService::get_user_profile` | | `grants.email_invite` | `rate_limited` | `grant_handler::create_grant` | +| `authz.external_user_blocked` | `internal_only_surface` | `require_internal_user_layer` (CalDAV / CardDAV / WebDAV) | +| `auth.nc_basic_rejected` | `external_user` | `basic_auth_middleware` (NC Basic-Auth surface) | +| `auth.app_password_create_rejected`| `external_user` | `create_app_password` | +| `groups.search_rejected` | `external_user` | `search_groups` | The convention (see CLAUDE.md § Authorization) is: any branch that denies or rejects a request **must** emit an audit event before returning the user-facing response. Anti-enumeration is preserved at the API surface (uniform response shape, 404 not 403), and the true reason is recorded only in the audit channel. @@ -155,16 +159,33 @@ The per-IP backstop respects `OXICLOUD_TRUST_PROXY_CIDR` for client IP resolutio ## Defence-in-depth boundary protections -External users are a new principal kind, and several pre-existing surfaces would over-share once they appeared. Three protections close those gaps: +External users are a new principal kind, and several pre-existing surfaces would over-share once they appeared. The protections fall in two layers — service-level filters that every surface inherits, and route-level layers that close protocol surfaces with no semantic meaning for externals. + +### Service-layer filters (every surface inherits these) 1. **Subject groups reject external members.** `subject_group_service.rs::add_member` short-circuits if the candidate user has `is_external = TRUE`. Otherwise an admin could add `alice@example.com` to "Engineering", which later receives a grant on internal-only resources — silent privilege escalation. Mirrors the no-external-admins enforcement. 2. **System contacts hide externals by default.** `auth_service.list_users` and `auth_service.search_users` take `include_external: bool`, defaulting to `false`. The share modal autocomplete (via `/api/address-books/system/contacts`) therefore never surfaces external users to internal callers, and external users never see internal users at the address book layer. 3. **External users are excluded from the Internal virtual group.** `pg_acl_engine.rs::expand_user` no longer inserts `INTERNAL_GROUP_ID` for users with `is_external = TRUE`. The group's name finally honours its semantics; every grant addressed to "all internal users" is now genuinely internal-only. -These three protections all activate at the **service layer**, so every protocol surface (REST, WebDAV, CalDAV, NextCloud) inherits them automatically. +### Route-level lockouts (close protocol surfaces upfront) + +External users have no calendar, no address book, no home folder, and (by design) no persistent credential. The protocol surfaces that assume those things are closed to them at the middleware layer — before any handler runs: + +4. **`/caldav/*`, `/carddav/*`, `/webdav/*`** are wrapped with `require_internal_user_layer` in `main.rs`. The layer runs after `auth_middleware`, reads the populated `CurrentUser` from request extensions, calls `require_internal_user` once per request, and 403s + audit-logs on rejection. PROPFIND / REPORT / OPTIONS — every DAV verb is closed. +5. **NextCloud `/remote.php/*` and `/ocs/*`** are gated inside `basic_auth_middleware`: after a successful app-password match, a follow-up lookup checks `is_external` and returns 401 if true. This is belt-and-braces — externals can't create app passwords in the first place (next item) — but it covers users who later flip to `is_external` after creating one. +6. **`POST /api/auth/app-passwords` is closed.** App passwords are persistent credentials; the magic-link-eligibility rule (`has_login_credential`) assumes externals have **no other credential configured**. Letting an external mint an app password would break that invariant and would also be the only way to authenticate them on the NC surface. 403 + audit on rejection. +7. **`GET /api/groups/search` is closed.** Group names aren't strictly secret, but externals have no legitimate use for the share-dialog autocomplete (they can't be added to groups anyway). Pre-existing safeguards from the user-lifecycle work continue to apply: the DB CHECK constraints `users_external_not_admin` and `users_external_no_storage`, and the `HomeFolderLifecycleHook` short-circuit that skips home-folder provisioning for externals. +### Why protocol-level instead of handler-level + +The route layer is one `require_internal_user_layer` per nest rather than one check per handler. Three reasons: + +- **Coverage.** Every DAV verb (and every NC OCS endpoint) is gated in one place. New handlers added under the same nest inherit the protection automatically. +- **Cost.** The layer hits the DB once per request (already cached in moka under the hood); a per-handler check would do the same work without the reuse. +- **Auditability.** A single audit event (`authz.external_user_blocked` with the request `path`) covers the whole subtree. Operators can grep one `event=` value across all DAV traffic. + ## Kill switches and feature scoping | Knob | What it does | diff --git a/src/interfaces/api/handlers/app_password_handler.rs b/src/interfaces/api/handlers/app_password_handler.rs index c3bc702f..012b2144 100644 --- a/src/interfaces/api/handlers/app_password_handler.rs +++ b/src/interfaces/api/handlers/app_password_handler.rs @@ -24,12 +24,42 @@ pub fn app_password_routes() -> Router> { /// POST /api/auth/app-passwords — Create a new app password. /// /// Returns the plain-text password ONCE. The user must copy it immediately. +/// +/// External users are rejected with 403: app passwords are persistent +/// credentials, and the magic-link-eligibility rule (`has_login_credential`) +/// is built on the assumption that externals have NO other credential +/// configured. Letting an external mint an app password would break that +/// invariant — and the Basic-Auth surface (`/remote.php/*`, `/ocs/*`) +/// has no semantic meaning for them anyway. See the +/// [magic-link auth architecture page] for the full visibility model. +/// +/// [magic-link auth architecture page]: ../../../../docs/architecture/magic-link-auth.md async fn create_app_password( State(state): State>, user: AuthUser, Json(request): Json, ) -> Result, AppError> { + // Gate externals BEFORE we touch the app_password_service. The + // service treats every authenticated caller equally; the policy + // that externals can't hold persistent credentials lives here. + if let Some(auth_svc) = state.auth_service.as_ref() + && let Err(err) = crate::interfaces::middleware::user::require_internal_user( + &auth_svc.auth_application_service, + user.id, + ) + .await + { + tracing::info!( + target: "audit", + event = "auth.app_password_create_rejected", + reason = "external_user", + caller_id = %user.id, + "👮🏻‍♂️ External user blocked from creating an app password" + ); + return Err(err); + } + let service = state .app_password_service .as_ref() diff --git a/src/interfaces/api/handlers/subject_group_handler.rs b/src/interfaces/api/handlers/subject_group_handler.rs index af1bbb9d..be3e41c1 100644 --- a/src/interfaces/api/handlers/subject_group_handler.rs +++ b/src/interfaces/api/handlers/subject_group_handler.rs @@ -258,9 +258,28 @@ pub async fn search_groups( headers: HeaderMap, Query(q): Query, ) -> Result { - // Any authenticated user can discover groups for the share dialog — - // membership lists remain admin-only via list_members. - let (_caller_id, role) = require_authenticated(&state, &headers).await?; + // Any authenticated INTERNAL user can discover groups for the + // share dialog — membership lists remain admin-only via + // list_members. External users have no business enumerating + // groups; defence-in-depth on top of the ReBAC layer (which + // already prevents them from being added to any group anyway). + let (caller_id, role) = require_authenticated(&state, &headers).await?; + if let Some(auth_svc) = state.auth_service.as_ref() + && let Err(err) = crate::interfaces::middleware::user::require_internal_user( + &auth_svc.auth_application_service, + caller_id, + ) + .await + { + tracing::info!( + target: "audit", + event = "groups.search_rejected", + reason = "external_user", + caller_id = %caller_id, + "👮🏻‍♂️ External user blocked from /api/groups/search" + ); + return Err(err); + } let can_manage = role == "admin"; let svc = service(&state)?; // The share-dialog autocomplete doesn't render a member-count chip, so diff --git a/src/interfaces/middleware/user.rs b/src/interfaces/middleware/user.rs index bd4083bc..ddce6387 100644 --- a/src/interfaces/middleware/user.rs +++ b/src/interfaces/middleware/user.rs @@ -21,11 +21,17 @@ //! [`super::admin`] — that variant exists because some handlers take //! `headers: HeaderMap` directly instead of `AuthUser`. +use axum::extract::{Request, State}; use axum::http::StatusCode; +use axum::middleware::Next; +use axum::response::{IntoResponse, Response}; +use std::sync::Arc; use uuid::Uuid; use crate::application::services::auth_application_service::AuthApplicationService; +use crate::common::di::AppState; use crate::interfaces::errors::AppError; +use crate::interfaces::middleware::auth::CurrentUser; /// Require the caller to be an internal user. Returns `Ok(())` for /// internal callers, `Err(403)` for externals. @@ -90,3 +96,55 @@ pub async fn require_admin_user( } Ok(()) } + +/// Axum middleware layer that blocks external users from a whole route +/// subtree. Apply via `.layer(from_fn_with_state(state, require_internal_user_layer))` +/// on the protocol nests (CalDAV / CardDAV / WebDAV) that have no +/// semantic meaning for externals — they own no calendars, no address +/// books, no home folder. +/// +/// Must run AFTER the auth middleware so `CurrentUser` is in the +/// request extensions; in tower order that means the auth layer is +/// added LAST (outermost). If the layer fires on an unauthenticated +/// path (no `CurrentUser` populated), it simply passes through — the +/// inner handler is then responsible for the 401, and we don't blanket- +/// 403 traffic the auth layer would have rejected anyway. +/// +/// Emits an `authz.external_user_blocked` audit event on rejection so +/// operators can spot which surfaces externals are probing. +pub async fn require_internal_user_layer( + State(state): State>, + request: Request, + next: Next, +) -> Response { + let caller_id = request + .extensions() + .get::>() + .map(|cu| cu.id); + + let (Some(caller_id), Some(svc)) = ( + caller_id, + state + .auth_service + .as_ref() + .map(|s| &*s.auth_application_service), + ) else { + // No auth populated, or auth disabled globally — pass through. + return next.run(request).await; + }; + + if let Err(err) = require_internal_user(svc, caller_id).await { + let path = request.uri().path().to_owned(); + tracing::info!( + target: "audit", + event = "authz.external_user_blocked", + reason = "internal_only_surface", + caller_id = %caller_id, + path = %path, + "👮🏻‍♂️ External user blocked from internal-only route subtree" + ); + return err.into_response(); + } + + next.run(request).await +} diff --git a/src/interfaces/nextcloud/basic_auth_middleware.rs b/src/interfaces/nextcloud/basic_auth_middleware.rs index cf57a2fd..4ebeb140 100644 --- a/src/interfaces/nextcloud/basic_auth_middleware.rs +++ b/src/interfaces/nextcloud/basic_auth_middleware.rs @@ -89,6 +89,31 @@ pub async fn basic_auth_middleware( if let Some(auth_svc) = state.auth_service.as_ref() { auth_svc.login_lockout.record_success(&username); } + // External users must never authenticate against the NC + // surface — that whole subtree (WebDAV files, uploads, + // trashbin, OCS user info, sharees autocomplete, etc.) has + // no semantic meaning for a magic-link-only principal, and + // an app password would be a persistent credential + // bypassing the magic-link-eligibility rule. POST + // /api/auth/app-passwords also gates externals upfront; + // this is the belt-and-braces check in case one slipped + // through (e.g. user later flipped to is_external). + if let Some(auth_svc) = state.auth_service.as_ref() + && let Ok(user) = auth_svc + .auth_application_service + .get_user_by_id(user_id) + .await + && user.is_external + { + tracing::info!( + target: "audit", + event = "auth.nc_basic_rejected", + reason = "external_user", + user_id = %user_id, + "👮🏻‍♂️ External user attempted NC Basic auth — rejected" + ); + return Err(NextcloudAuthError::Unauthorized); + } request.extensions_mut().insert(Arc::new(CurrentUser { id: user_id, username: uname, diff --git a/src/main.rs b/src/main.rs index b40dfb5e..96cf18c5 100644 --- a/src/main.rs +++ b/src/main.rs @@ -378,19 +378,41 @@ async fn main() -> Result<(), Box> { auth_middleware, )); - // CalDAV/CardDAV/WebDAV with auth middleware (merged, not nested) - let caldav_protected = caldav_router.layer(axum::middleware::from_fn_with_state( - app_state.clone(), - auth_middleware, - )); - let carddav_protected = carddav_router.layer(axum::middleware::from_fn_with_state( - app_state.clone(), - auth_middleware, - )); - let webdav_protected = webdav_router.layer(axum::middleware::from_fn_with_state( - app_state.clone(), - auth_middleware, - )); + // CalDAV/CardDAV/WebDAV with auth + internal-only middleware + // (merged, not nested). External users have no calendar, no + // address book, and no home folder — locking them out of these + // protocol subtrees in one place avoids leaking the protocol + // surface to a principal kind that can do nothing with it. The + // `require_internal_user_layer` runs AFTER auth (tower order: + // later .layer() = outermost = runs first). + use oxicloud::interfaces::middleware::user::require_internal_user_layer; + let caldav_protected = caldav_router + .layer(axum::middleware::from_fn_with_state( + app_state.clone(), + require_internal_user_layer, + )) + .layer(axum::middleware::from_fn_with_state( + app_state.clone(), + auth_middleware, + )); + let carddav_protected = carddav_router + .layer(axum::middleware::from_fn_with_state( + app_state.clone(), + require_internal_user_layer, + )) + .layer(axum::middleware::from_fn_with_state( + app_state.clone(), + auth_middleware, + )); + let webdav_protected = webdav_router + .layer(axum::middleware::from_fn_with_state( + app_state.clone(), + require_internal_user_layer, + )) + .layer(axum::middleware::from_fn_with_state( + app_state.clone(), + auth_middleware, + )); // Magic-link redemption — public, no CSRF, no rate limit (the token IS // the credential and `mark_used` is single-use). PR 12 will add a diff --git a/tests/api/external_users.hurl b/tests/api/external_users.hurl index db666627..fd5bb049 100644 --- a/tests/api/external_users.hurl +++ b/tests/api/external_users.hurl @@ -239,6 +239,48 @@ Authorization: Bearer {{bob_access_token}} HTTP 404 +# 11f — bob CANNOT create an app password. Externals are +# magic-link-only; an app password would be a persistent +# credential bypassing has_login_credential(). +POST {{base_url}}/api/auth/app-passwords +Authorization: Bearer {{bob_access_token}} +Content-Type: application/json +{ "label": "rogue" } + +HTTP 403 + +# 11g — bob CANNOT enumerate groups via the share-dialog endpoint. +# Defence-in-depth on top of the ReBAC layer (externals can't +# be group members today anyway). +GET {{base_url}}/api/groups/search?q=any +Authorization: Bearer {{bob_access_token}} + +HTTP 403 + +# 11h — bob CANNOT reach the WebDAV protocol surface. He has no home +# folder, so the protocol has no semantic meaning for him. +# Layered before the handler so even malformed PROPFIND is +# rejected upfront. +PROPFIND {{base_url}}/webdav/ +Authorization: Bearer {{bob_access_token}} +Depth: 0 + +HTTP 403 + +# 11i — bob CANNOT reach the CalDAV surface. No calendar. +PROPFIND {{base_url}}/caldav/ +Authorization: Bearer {{bob_access_token}} +Depth: 0 + +HTTP 403 + +# 11j — bob CANNOT reach the CardDAV surface. No personal address book. +PROPFIND {{base_url}}/carddav/ +Authorization: Bearer {{bob_access_token}} +Depth: 0 + +HTTP 403 + # ───────────────────────────────────────────────────────────── # Step 12 — /api/users/{id} happy path (Alice → Bob). diff --git a/tests/common/server.env b/tests/common/server.env index f4f37a83..2123c57d 100644 --- a/tests/common/server.env +++ b/tests/common/server.env @@ -15,7 +15,7 @@ OXICLOUD_ENABLE_MUSIC=true OXICLOUD_EXPOSE_SYSTEM_USERS=true OXICLOUD_WOPI_ENABLED=false OXICLOUD_OIDC_ENABLED=false -RUST_LOG=warn +RUST_LOG="warn,audit=info" #RUST_LOG=debug #RUST_LOG=info