diff --git a/docs/api-v2-guide.md b/docs/api-v2-guide.md index 9d133f9..be45847 100644 --- a/docs/api-v2-guide.md +++ b/docs/api-v2-guide.md @@ -525,10 +525,16 @@ scopes are checked: | Scope | Admits | |---|---| -| `write` | any mutation: playlists, favorites, ratings, the queue, bookmarks, shares, scrobbles, scans, issuing an OAuth code | +| `write` | any mutation: playlists, favorites, ratings, the queue, bookmarks, shares, scrobbles, scans | | `admin` | the administrative routes, and everything `write` admits | -Reading needs no scope, so a token naming neither is read-only. **A scope this +Reading needs no scope, so a token naming neither is read-only. **Minting a +credential needs no scope either — it needs the absence of one**: the +authorization code flow at `POST /api/v2/oauth/authorize` is refused to any +token carrying a scope list at all, because the session it returns is +unrestricted and nothing on the grant records what asked for it. A narrowed +credential must not be able to mint a broader one. Pairing a device is done +from a session, which is what the flow is for. **A scope this server does not know grants nothing**, which is why `catalog:read` reads and does no more: there is no vocabulary to learn, only these two names to use. diff --git a/docs/rfcs/RFC-002-waveflow-server-v2.md b/docs/rfcs/RFC-002-waveflow-server-v2.md index 3e68046..298392f 100644 --- a/docs/rfcs/RFC-002-waveflow-server-v2.md +++ b/docs/rfcs/RFC-002-waveflow-server-v2.md @@ -70,7 +70,9 @@ Genre matching is one rule across every method. `getGenres` groups by `genre.can `getArtistInfo` and `getArtistInfo2` resolve the requested artist through tenant-scoped catalogue access and return their standard empty containers until artist biography enrichment is implemented. This preserves compatibility with clients such as DSub without fabricating biography or similar-artist metadata. `getAlbumInfo` and `getAlbumInfo2` behave the same way for albums, for the same reason: Feishin and Symfonium call them as soon as an album page opens, and an unimplemented-method error there reads to the client as a broken album rather than as absent enrichment. They carry one real value, the album's release identifier, as their `musicBrainzId` element; notes and biography images stay absent because WaveFlow queries no remote source. `AlbumInfo` predates the presence rule and its members are elements rather than attributes, so an album with no release id omits the element instead of sending it empty. -API tokens are restricted by their scopes on every route, not only on the administrative ones. `Access` names what a route needs — `Read`, `Write` or `Admin` — and is chosen at the single point where the caller is resolved, so a route cannot exist without answering the question. An empty scope list is unrestricted, which is what sessions, OAuth grants and tokens issued without scopes carry; a non-empty list grants only what it names, and a name the server does not know grants nothing, so no vocabulary has to be enumerated. `admin` implies `write`, because a credential trusted to create accounts is not usefully barred from creating a playlist. Issuing a token stays administrative on both surfaces: a token carries the authority of the account it belongs to, so who may mint one is a question about the instance. +API tokens are restricted by their scopes on every route, not only on the administrative ones. `Access` names what a route needs — `Read`, `Write`, `Admin` or `Unrestricted` — and is chosen at the single point where the caller is resolved, so a route cannot exist without answering the question. An empty scope list is unrestricted, which is what sessions, OAuth grants and tokens issued without scopes carry; a non-empty list grants only what it names, and a name the server does not know grants nothing, so no vocabulary has to be enumerated. `admin` implies `write`, because a credential trusted to create accounts is not usefully barred from creating a playlist. Issuing a token stays administrative on both surfaces: a token carries the authority of the account it belongs to, so who may mint one is a question about the instance. + +Minting a credential is `Unrestricted`, which is a restriction and not a role: only a credential carrying no scope list at all may do it. The authorization code flow returns a session with the account's whole authority, and neither the authorization row nor the session records what asked for it, so a `write` token could reach `POST /api/v2/oauth/authorize` and come back holding a session that answered to `Admin` — an escalation in two requests, from a token deliberately issued without `admin`. The category error was treating "mutate user data" and "issue a reference to this account" as one authority. The durable fix is to carry the scopes through the grant, so that a session is never broader than what issued it; that needs a column on `oauth_authorization` and on `session`, and until it exists this closes the path without a migration. An ordinary account still pairs its own devices from its own session, which is what the flow is for. `playlist.owner` and `share.username` carry the authenticated username. Both collections are read scoped to their owner, so no other name is reachable; the empty string previously emitted made Feishin treat every playlist as another account's and refuse to edit it. diff --git a/src/http.rs b/src/http.rs index 3049540..8de950c 100644 --- a/src/http.rs +++ b/src/http.rs @@ -1109,7 +1109,11 @@ pub async fn oauth_authorize( ) -> Result, ApiError> { // The browser session is the proof of identity; the consent screen is a // route of the embedded client, so this is a JSON call rather than a form. - let user = authenticated(&state, &headers, Access::Write).await?; + // + // Unrestricted rather than Write: this mints a credential, and the one it + // mints carries the account's whole authority. A `write` token reaching + // here came back holding a session that answered to `Admin`. + let user = authenticated(&state, &headers, Access::Unrestricted).await?; let redirect_to = state .services .authorize_native_client( @@ -2237,11 +2241,30 @@ fn bearer_token(headers: &HeaderMap) -> Option<&str> { pub(crate) enum Access { /// Reads the caller's own catalogue and user data. Read, - /// Writes on the caller's behalf: playlists, favorites, ratings, the queue, - /// bookmarks, shares, scrobbles, scans, and issuing an OAuth code. + /// Writes on the caller's behalf: playlists, favorites, ratings, the + /// queue, bookmarks, shares, scrobbles and scans. Write, /// Acts on the instance: accounts, libraries, memberships, credentials. Admin, + /// Mints a new credential from this one. + /// + /// Only an unrestricted credential may, because the credential minted is + /// itself unrestricted: a session issued through the authorization code + /// flow carries the account's whole authority, and nothing on the grant + /// records what asked for it. Letting a narrowed credential mint one is + /// how a token widens itself — `write` was enough to reach the code flow, + /// and the session that came back answered to `Admin`. + /// + /// This is a restriction, not a role: an ordinary account pairs its own + /// devices from its own session, which is what the flow is for. What it + /// refuses is doing so on behalf of a credential that was deliberately + /// narrowed. + /// + /// The durable fix is to carry the scopes through the grant, so a session + /// is never broader than what issued it. That needs a column on + /// `oauth_authorization` and on `session`; until then, this closes the + /// path without a migration. + Unrestricted, } /// The scope that admits the administrative routes. @@ -2270,6 +2293,9 @@ impl Access { Self::Read => true, Self::Write => holds(WRITE_SCOPE) || holds(ADMIN_SCOPE), Self::Admin => holds(ADMIN_SCOPE), + // Unreachable: the early return above already answered for every + // empty list, and a non-empty one is by definition restricted. + Self::Unrestricted => false, } } } @@ -2278,9 +2304,12 @@ impl Access { /// what the route is about to do. /// /// Both halves of administrative authority live here: an active administrator, -/// on a credential that has not been narrowed away from it. Being an -/// administrator does not widen a token, and a token cannot promote an -/// ordinary account. +/// on a credential that has not been narrowed away from it. A token cannot +/// promote an ordinary account, and an administrator's token is not widened by +/// whose account it belongs to. +/// +/// It could widen itself, once. Minting a credential is [`Access::Unrestricted`] +/// for that reason, and not merely a write. pub(crate) async fn authenticated( state: &AppState, headers: &HeaderMap, diff --git a/tests/v2_foundations.rs b/tests/v2_foundations.rs index a5bcdd4..a4714d1 100644 --- a/tests/v2_foundations.rs +++ b/tests/v2_foundations.rs @@ -7899,12 +7899,50 @@ async fn pkce_authorization_grants_a_native_session_exactly_once() { let (_temp, config, state) = test_app().await; let password = "correct horse battery staple"; let hash = security::hash_password(password).unwrap(); - state + let user = state .db .create_account("pkce-user", &hash, AccountRole::Admin, now_ms()) .await .unwrap(); - let router = waveflow_server::app(&config, state); + // One indexed track, so the scoped token can be shown writing rather + // than merely not being refused. + let music = config.data_dir.join("pkce-music"); + std::fs::create_dir_all(&music).unwrap(); + let library = state + .db + .create_library( + user, + "Pkce", + &std::fs::canonicalize(&music).unwrap(), + LibraryVisibility::Private, + now_ms(), + ) + .await + .unwrap(); + let scan = state + .db + .create_scan_job(library, Some(user), "manual") + .await + .unwrap(); + state.db.start_scan_job(scan, 1).await.unwrap(); + let mut input = browse_input(700, "Paired", "Handshake", "Loopback", Some(1), Some(1)); + input.relative_path = "pkce-0.flac".into(); + input.quick_hash = format!("{:064x}", 71_000); + input.full_hash = format!("{:064x}", 72_000); + state + .db + .apply_catalog_track(library, scan, &input, None, false) + .await + .unwrap(); + state.db.finish_scan_job(scan, 0).await.unwrap(); + let track = state + .services + .catalog_snapshot(user, &[]) + .await + .unwrap() + .songs[0] + .id; + let router = waveflow_server::app(&config, state.clone()); let token = login_token(&router, "pkce-user", password).await; let verifier = "H1r8mQ2xY7pL4vC0nB6zK9tW3sD5gJ8fA2eR7uI1oP4"; @@ -7954,6 +7992,57 @@ async fn pkce_authorization_grants_a_native_session_exactly_once() { StatusCode::UNAUTHORIZED ); + // A narrowed credential cannot mint an unrestricted one. The session the + // code flow returns carries the account's whole authority and records + // nothing about what asked for it, so a `write` token reaching this route + // came back holding a session that answered to `Admin`: two requests from + // a token deliberately issued without `admin`. + let scoped = json_body( + router + .clone() + .oneshot( + Request::post("/api/v2/admin/users/pkce-user/tokens") + .header("authorization", format!("Bearer {token}")) + .header("content-type", "application/json") + .body(Body::from( + serde_json::json!({"name": "agent", "scopes": ["write"]}).to_string(), + )) + .unwrap(), + ) + .await + .unwrap(), + ) + .await; + let scoped = scoped["secret"].as_str().expect("the secret").to_owned(); + assert_eq!( + authorize(grant.clone(), Some(scoped.clone())) + .await + .status(), + StatusCode::FORBIDDEN + ); + // It is the narrowing that refuses, not the account: the same account's + // session still grants, and the token still writes what it was given. + let writes = router + .clone() + .oneshot( + Request::builder() + .method(Method::PUT) + .uri(format!("/api/v2/ratings/track/{track}")) + .header("authorization", format!("Bearer {scoped}")) + .header("content-type", "application/json") + .body(Body::from(serde_json::json!({"rating": 3}).to_string())) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(writes.status(), StatusCode::NO_CONTENT); + // And the write landed: a route that had quietly become a no-op would + // still answer without refusing. + let ratings = state.services.ratings(user).await.unwrap(); + assert_eq!(ratings.len(), 1); + assert_eq!(ratings[0].entity_id, track); + assert_eq!(ratings[0].rating, 3); + // A redirect that could carry the code off the machine is refused. let mut remote = grant.clone(); remote["redirect_uri"] = "http://evil.example.com/cb".into();