From 7e247830c963c06f62a1deea1a5bc8957359541c Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Mon, 4 Sep 2023 16:32:43 +0200 Subject: [PATCH] data-model: Make the user_id optional in the OAuth 2.0 sessions --- crates/data-model/src/oauth2/session.rs | 2 +- crates/graphql/src/lib.rs | 6 +-- crates/graphql/src/model/oauth.rs | 16 ++++-- .../graphql/src/mutations/oauth2_session.rs | 36 +++++++------ crates/graphql/src/query/viewer.rs | 4 +- crates/handlers/src/graphql/mod.rs | 22 +++++--- crates/handlers/src/oauth2/introspection.rs | 52 ++++++++++++------- crates/handlers/src/oauth2/revoke.rs | 40 +++++++------- crates/handlers/src/oauth2/userinfo.rs | 19 ++++++- crates/storage-pg/src/oauth2/access_token.rs | 1 - .../src/oauth2/authorization_grant.rs | 1 - crates/storage-pg/src/oauth2/refresh_token.rs | 1 - crates/storage-pg/src/oauth2/session.rs | 5 +- frontend/schema.graphql | 2 +- frontend/src/gql/graphql.ts | 2 +- frontend/src/gql/schema.ts | 9 ++-- 16 files changed, 133 insertions(+), 85 deletions(-) diff --git a/crates/data-model/src/oauth2/session.rs b/crates/data-model/src/oauth2/session.rs index 58268b4a..ae55db4a 100644 --- a/crates/data-model/src/oauth2/session.rs +++ b/crates/data-model/src/oauth2/session.rs @@ -66,7 +66,7 @@ pub struct Session { pub id: Ulid, pub state: SessionState, pub created_at: DateTime, - pub user_id: Ulid, + pub user_id: Option, pub user_session_id: Option, pub client_id: Ulid, pub scope: Scope, diff --git a/crates/graphql/src/lib.rs b/crates/graphql/src/lib.rs index 2a9d36c4..7d838f2f 100644 --- a/crates/graphql/src/lib.rs +++ b/crates/graphql/src/lib.rs @@ -63,7 +63,7 @@ pub enum Requester { BrowserSession(BrowserSession), /// The requester is a OAuth2 session, with an access token. - OAuth2Session(Session, User), + OAuth2Session(Session, Option), } trait OwnerId { @@ -90,7 +90,7 @@ impl OwnerId for mas_data_model::UserEmail { impl OwnerId for Session { fn owner_id(&self) -> Option { - Some(self.user_id) + self.user_id } } @@ -126,7 +126,7 @@ impl Requester { fn user(&self) -> Option<&User> { match self { Self::BrowserSession(session) => Some(&session.user), - Self::OAuth2Session(_session, user) => Some(user), + Self::OAuth2Session(_session, user) => user.as_ref(), Self::Anonymous => None, } } diff --git a/crates/graphql/src/model/oauth.rs b/crates/graphql/src/model/oauth.rs index 532058d5..87c506c7 100644 --- a/crates/graphql/src/model/oauth.rs +++ b/crates/graphql/src/model/oauth.rs @@ -22,7 +22,7 @@ use ulid::Ulid; use url::Url; use super::{BrowserSession, NodeType, User}; -use crate::state::ContextExt; +use crate::{state::ContextExt, UserId}; /// The state of an OAuth 2.0 session. #[derive(Enum, Copy, Clone, Eq, PartialEq)] @@ -108,17 +108,25 @@ impl OAuth2Session { } /// User authorized for this session. - pub async fn user(&self, ctx: &Context<'_>) -> Result { + pub async fn user(&self, ctx: &Context<'_>) -> Result, async_graphql::Error> { let state = ctx.state(); + let Some(user_id) = self.0.user_id else { + return Ok(None); + }; + + if !ctx.requester().is_owner_or_admin(&UserId(user_id)) { + return Err(async_graphql::Error::new("Unauthorized")); + } + let mut repo = state.repository().await?; let user = repo .user() - .lookup(self.0.user_id) + .lookup(user_id) .await? .context("Could not load user")?; repo.cancel().await?; - Ok(User(user)) + Ok(Some(User(user))) } } diff --git a/crates/graphql/src/mutations/oauth2_session.rs b/crates/graphql/src/mutations/oauth2_session.rs index 375ba445..b21096e9 100644 --- a/crates/graphql/src/mutations/oauth2_session.rs +++ b/crates/graphql/src/mutations/oauth2_session.rs @@ -96,24 +96,26 @@ impl OAuth2SessionMutations { return Ok(EndOAuth2SessionPayload::NotFound); } - let user = repo - .user() - .lookup(session.user_id) - .await? - .context("Could not load user")?; + if let Some(user_id) = session.user_id { + let user = repo + .user() + .lookup(user_id) + .await? + .context("Could not load user")?; - // Scan the scopes of the session to find if there is any device that should be - // deleted from the Matrix server. - // TODO: this should be moved in a higher level "end oauth session" method. - // XXX: this might not be the right semantic, but it's the best we - // can do for now, since we're not explicitly storing devices for OAuth2 - // sessions. - for scope in &*session.scope { - if let Some(device) = Device::from_scope_token(scope) { - // Schedule a job to delete the device. - repo.job() - .schedule_job(DeleteDeviceJob::new(&user, &device)) - .await?; + // Scan the scopes of the session to find if there is any device that should be + // deleted from the Matrix server. + // TODO: this should be moved in a higher level "end oauth session" method. + // XXX: this might not be the right semantic, but it's the best we + // can do for now, since we're not explicitly storing devices for OAuth2 + // sessions. + for scope in &*session.scope { + if let Some(device) = Device::from_scope_token(scope) { + // Schedule a job to delete the device. + repo.job() + .schedule_job(DeleteDeviceJob::new(&user, &device)) + .await?; + } } } diff --git a/crates/graphql/src/query/viewer.rs b/crates/graphql/src/query/viewer.rs index 352803ba..11ef37ed 100644 --- a/crates/graphql/src/query/viewer.rs +++ b/crates/graphql/src/query/viewer.rs @@ -31,8 +31,8 @@ impl ViewerQuery { match requester { Requester::BrowserSession(session) => Viewer::user(session.user.clone()), - Requester::OAuth2Session(_session, user) => Viewer::user(user.clone()), - Requester::Anonymous => Viewer::anonymous(), + Requester::OAuth2Session(_session, Some(user)) => Viewer::user(user.clone()), + Requester::OAuth2Session(_, None) | Requester::Anonymous => Viewer::anonymous(), } } diff --git a/crates/handlers/src/graphql/mod.rs b/crates/handlers/src/graphql/mod.rs index 16fc692c..7ab4a0fe 100644 --- a/crates/handlers/src/graphql/mod.rs +++ b/crates/handlers/src/graphql/mod.rs @@ -29,6 +29,7 @@ use futures_util::TryStreamExt; use headers::{authorization::Bearer, Authorization, ContentType, HeaderValue}; use hyper::header::CACHE_CONTROL; use mas_axum_utils::{cookies::CookieJar, FancyError, SessionInfo, SessionInfoExt}; +use mas_data_model::User; use mas_graphql::{Requester, Schema}; use mas_matrix::HomeserverConnection; use mas_policy::{InstantiateError, Policy, PolicyFactory}; @@ -204,13 +205,22 @@ async fn get_requester( .await? .ok_or(RouteError::LoadFailed)?; - let user = repo - .user() - .lookup(session.user_id) - .await? - .ok_or(RouteError::LoadFailed)?; + // Load the user if there is one + let user = if let Some(user_id) = session.user_id { + let user = repo + .user() + .lookup(user_id) + .await? + .ok_or(RouteError::LoadFailed)?; + Some(user) + } else { + None + }; - if !token.is_valid(clock.now()) || !session.is_valid() || !user.is_valid() { + // If there is a user for this session, check that it is not locked + let user_valid = user.as_ref().map_or(false, User::is_valid); + + if !token.is_valid(clock.now()) || !session.is_valid() || !user_valid { return Err(RouteError::InvalidToken); } diff --git a/crates/handlers/src/oauth2/introspection.rs b/crates/handlers/src/oauth2/introspection.rs index 3051f984..a627c721 100644 --- a/crates/handlers/src/oauth2/introspection.rs +++ b/crates/handlers/src/oauth2/introspection.rs @@ -184,24 +184,32 @@ pub(crate) async fn post( // XXX: is that the right error to bubble up? .ok_or(RouteError::UnknownToken)?; - let user = repo - .user() - .lookup(session.user_id) - .await? - .filter(User::is_valid) - // XXX: is that the right error to bubble up? - .ok_or(RouteError::UnknownToken)?; + // The session might not have a user on it (for Client Credentials grants for + // example), so we're optionally fetching the user + let (sub, username) = if let Some(user_id) = session.user_id { + let user = repo + .user() + .lookup(user_id) + .await? + // Fail if the user is not valid (e.g. locked) + .filter(User::is_valid) + .ok_or(RouteError::UnknownToken)?; + + (Some(user.sub), Some(user.username)) + } else { + (None, None) + }; IntrospectionResponse { active: true, scope: Some(session.scope), client_id: Some(session.client_id.to_string()), - username: Some(user.username), + username, token_type: Some(OAuthTokenTypeHint::AccessToken), exp: Some(token.expires_at), iat: Some(token.created_at), nbf: Some(token.created_at), - sub: Some(user.sub), + sub, aud: None, iss: None, jti: Some(token.jti()), @@ -224,24 +232,32 @@ pub(crate) async fn post( // XXX: is that the right error to bubble up? .ok_or(RouteError::UnknownToken)?; - let user = repo - .user() - .lookup(session.user_id) - .await? - .filter(User::is_valid) - // XXX: is that the right error to bubble up? - .ok_or(RouteError::UnknownToken)?; + // The session might not have a user on it (for Client Credentials grants for + // example), so we're optionally fetching the user + let (sub, username) = if let Some(user_id) = session.user_id { + let user = repo + .user() + .lookup(user_id) + .await? + // Fail if the user is not valid (e.g. locked) + .filter(User::is_valid) + .ok_or(RouteError::UnknownToken)?; + + (Some(user.sub), Some(user.username)) + } else { + (None, None) + }; IntrospectionResponse { active: true, scope: Some(session.scope), client_id: Some(session.client_id.to_string()), - username: Some(user.username), + username, token_type: Some(OAuthTokenTypeHint::RefreshToken), exp: None, iat: Some(token.created_at), nbf: Some(token.created_at), - sub: Some(user.sub), + sub, aud: None, iss: None, jti: Some(token.jti()), diff --git a/crates/handlers/src/oauth2/revoke.rs b/crates/handlers/src/oauth2/revoke.rs index 28cc76e5..7731cbd4 100644 --- a/crates/handlers/src/oauth2/revoke.rs +++ b/crates/handlers/src/oauth2/revoke.rs @@ -199,25 +199,29 @@ pub(crate) async fn post( return Err(RouteError::UnauthorizedClient); } - // Fetch the user - let user = repo - .user() - .lookup(session.user_id) - .await? - .ok_or(RouteError::UnknownToken)?; + // If the session is associated with a user, make sure we schedule a device + // deletion job for all the devices associated with the session. + if let Some(user_id) = session.user_id { + // Fetch the user + let user = repo + .user() + .lookup(user_id) + .await? + .ok_or(RouteError::UnknownToken)?; - // Scan the scopes of the session to find if there is any device that should be - // deleted from the Matrix server. - // TODO: this should be moved in a higher level "end oauth session" method. - // XXX: this might not be the right semantic, but it's the best we - // can do for now, since we're not explicitly storing devices for OAuth2 - // sessions. - for scope in &*session.scope { - if let Some(device) = Device::from_scope_token(scope) { - // Schedule a job to delete the device. - repo.job() - .schedule_job(DeleteDeviceJob::new(&user, &device)) - .await?; + // Scan the scopes of the session to find if there is any device that should be + // deleted from the Matrix server. + // TODO: this should be moved in a higher level "end oauth session" method. + // XXX: this might not be the right semantic, but it's the best we + // can do for now, since we're not explicitly storing devices for OAuth2 + // sessions. + for scope in &*session.scope { + if let Some(device) = Device::from_scope_token(scope) { + // Schedule a job to delete the device. + repo.job() + .schedule_job(DeleteDeviceJob::new(&user, &device)) + .await?; + } } } diff --git a/crates/handlers/src/oauth2/userinfo.rs b/crates/handlers/src/oauth2/userinfo.rs index d01504ec..dbd8370e 100644 --- a/crates/handlers/src/oauth2/userinfo.rs +++ b/crates/handlers/src/oauth2/userinfo.rs @@ -65,6 +65,9 @@ pub enum RouteError { #[from] AuthorizationVerificationError, ), + #[error("session is not allowed to access the userinfo endpoint")] + Unauthorized, + #[error("no suitable key found for signing")] InvalidSigningKey, @@ -86,7 +89,9 @@ impl IntoResponse for RouteError { Self::Internal(_) | Self::InvalidSigningKey | Self::NoSuchClient | Self::NoSuchUser => { (StatusCode::INTERNAL_SERVER_ERROR, self.to_string()).into_response() } - Self::AuthorizationVerificationError(_e) => StatusCode::UNAUTHORIZED.into_response(), + Self::AuthorizationVerificationError(_) | Self::Unauthorized => { + StatusCode::UNAUTHORIZED.into_response() + } } } } @@ -102,9 +107,19 @@ pub async fn get( ) -> Result { let session = user_authorization.protected(&mut repo, &clock).await?; + // This endpoint requires the `openid` scope. + if !session.scope.contains("openid") { + return Err(RouteError::Unauthorized); + } + + // Fail if the session is not associated with a user. + let Some(user_id) = session.user_id else { + return Err(RouteError::Unauthorized); + }; + let user = repo .user() - .lookup(session.user_id) + .lookup(user_id) .await? .ok_or(RouteError::NoSuchUser)?; diff --git a/crates/storage-pg/src/oauth2/access_token.rs b/crates/storage-pg/src/oauth2/access_token.rs index 96c31f79..758fdf41 100644 --- a/crates/storage-pg/src/oauth2/access_token.rs +++ b/crates/storage-pg/src/oauth2/access_token.rs @@ -135,7 +135,6 @@ impl<'c> OAuth2AccessTokenRepository for PgOAuth2AccessTokenRepository<'c> { fields( db.statement, %session.id, - user.id = %session.user_id, client.id = %session.client_id, access_token.id, ), diff --git a/crates/storage-pg/src/oauth2/authorization_grant.rs b/crates/storage-pg/src/oauth2/authorization_grant.rs index 95562b44..6fe56010 100644 --- a/crates/storage-pg/src/oauth2/authorization_grant.rs +++ b/crates/storage-pg/src/oauth2/authorization_grant.rs @@ -406,7 +406,6 @@ impl<'c> OAuth2AuthorizationGrantRepository for PgOAuth2AuthorizationGrantReposi %grant.id, client.id = %grant.client_id, %session.id, - user.id = %session.user_id, ), err, )] diff --git a/crates/storage-pg/src/oauth2/refresh_token.rs b/crates/storage-pg/src/oauth2/refresh_token.rs index 1cac29ff..c7132f4f 100644 --- a/crates/storage-pg/src/oauth2/refresh_token.rs +++ b/crates/storage-pg/src/oauth2/refresh_token.rs @@ -143,7 +143,6 @@ impl<'c> OAuth2RefreshTokenRepository for PgOAuth2RefreshTokenRepository<'c> { fields( db.statement, %session.id, - user.id = %session.user_id, client.id = %session.client_id, refresh_token.id, ), diff --git a/crates/storage-pg/src/oauth2/session.rs b/crates/storage-pg/src/oauth2/session.rs index 6a7713da..6864a625 100644 --- a/crates/storage-pg/src/oauth2/session.rs +++ b/crates/storage-pg/src/oauth2/session.rs @@ -86,7 +86,7 @@ impl TryFrom for Session { state, created_at: value.created_at, client_id: value.oauth2_client_id.into(), - user_id: value.user_id.into(), + user_id: Some(value.user_id.into()), user_session_id: value.user_session_id.map(Ulid::from), scope, }) @@ -186,7 +186,7 @@ impl<'c> OAuth2SessionRepository for PgOAuth2SessionRepository<'c> { id, state: SessionState::Valid, created_at, - user_id: user_session.user.id, + user_id: Some(user_session.user.id), user_session_id: Some(user_session.id), client_id: client.id, scope, @@ -200,7 +200,6 @@ impl<'c> OAuth2SessionRepository for PgOAuth2SessionRepository<'c> { db.statement, %session.id, %session.scope, - user.id = %session.user_id, client.id = %session.client_id, ), err, diff --git a/frontend/schema.graphql b/frontend/schema.graphql index e27f059d..c0f24bb4 100644 --- a/frontend/schema.graphql +++ b/frontend/schema.graphql @@ -695,7 +695,7 @@ type Oauth2Session implements Node & CreationEvent { """ User authorized for this session. """ - user: User! + user: User } type Oauth2SessionConnection { diff --git a/frontend/src/gql/graphql.ts b/frontend/src/gql/graphql.ts index db90c12c..cafc4ec4 100644 --- a/frontend/src/gql/graphql.ts +++ b/frontend/src/gql/graphql.ts @@ -517,7 +517,7 @@ export type Oauth2Session = CreationEvent & /** The state of the session. */ state: Oauth2SessionState; /** User authorized for this session. */ - user: User; + user?: Maybe; }; export type Oauth2SessionConnection = { diff --git a/frontend/src/gql/schema.ts b/frontend/src/gql/schema.ts index b2b771d7..e35cbefc 100644 --- a/frontend/src/gql/schema.ts +++ b/frontend/src/gql/schema.ts @@ -1390,12 +1390,9 @@ export default { { name: "user", type: { - kind: "NON_NULL", - ofType: { - kind: "OBJECT", - name: "User", - ofType: null, - }, + kind: "OBJECT", + name: "User", + ofType: null, }, args: [], },