Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions crates/handlers/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,10 @@ where
mas_router::OAuth2AuthorizationEndpoint::route(),
get(self::oauth2::authorization::get),
)
.route(
mas_router::OAuth2EndSession::route(),
get(self::oauth2::end_session::get),
)
.route(
mas_router::Consent::route(),
get(self::oauth2::authorization::consent::get)
Expand Down
2 changes: 2 additions & 0 deletions crates/handlers/src/oauth2/discovery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ pub(crate) async fn get(
let revocation_endpoint = Some(url_builder.oauth_revocation_endpoint());
let userinfo_endpoint = Some(url_builder.oidc_userinfo_endpoint());
let registration_endpoint = Some(url_builder.oauth_registration_endpoint());
let end_session_endpoint = Some(url_builder.oauth_end_session_endpoint());

let scopes_supported = Some(vec![scope::OPENID.to_string(), scope::EMAIL.to_string()]);

Expand Down Expand Up @@ -172,6 +173,7 @@ pub(crate) async fn get(
request_uri_parameter_supported,
prompt_values_supported,
device_authorization_endpoint,
end_session_endpoint,
..ProviderMetadata::default()
};

Expand Down
201 changes: 201 additions & 0 deletions crates/handlers/src/oauth2/end_session.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,201 @@
// Copyright 2025 New Vector Ltd.
//
// SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial
// Please see LICENSE files in the repository root for full details.
use axum::{
Json,
extract::State,
response::{IntoResponse, Redirect, Response},
};
use axum_extra::extract::Query;
use hyper::StatusCode;
use mas_axum_utils::{SessionInfoExt, cookies::CookieJar, record_error};
use mas_data_model::{BoxClock, BoxRng, Clock};
use mas_keystore::Keystore;
use mas_oidc_client::{
error::IdTokenError,
requests::jose::{JwtVerificationData, verify_id_token},
};
use mas_router::UrlBuilder;
use mas_storage::{
BoxRepository, RepositoryAccess,
queue::{QueueJobRepositoryExt as _, SyncDevicesJob},
user::BrowserSessionRepository,
};
use oauth2_types::errors::{ClientError, ClientErrorCode};
use serde::{Deserialize, Serialize};
use thiserror::Error;

use crate::{BoundActivityTracker, impl_from_error_for_route};

#[derive(Debug, Deserialize, Serialize)]
pub(crate) struct EndSessionParam {
id_token_hint: String,
post_logout_redirect_uri: String,
}

#[derive(Debug, Error)]
pub(crate) enum RouteError {
#[error(transparent)]
Internal(Box<dyn std::error::Error + Send + Sync + 'static>),

#[error("bad request")]
BadRequest,

#[error("client not found")]
ClientNotFound,

#[error("client is unauthorized")]
UnauthorizedClient,

#[error("unknown token")]
UnknownToken,
}

impl_from_error_for_route!(mas_storage::RepositoryError);

impl IntoResponse for RouteError {
fn into_response(self) -> Response {
let sentry_event_id = record_error!(self, Self::Internal(_));
let response = match self {
Self::Internal(_) => (
StatusCode::INTERNAL_SERVER_ERROR,
Json(ClientError::from(ClientErrorCode::ServerError)),
)
.into_response(),

Self::BadRequest => (
StatusCode::BAD_REQUEST,
Json(ClientError::from(ClientErrorCode::InvalidRequest)),
)
.into_response(),

Self::ClientNotFound => (
StatusCode::UNAUTHORIZED,
Json(ClientError::from(ClientErrorCode::InvalidClient)),
)
.into_response(),

Self::UnauthorizedClient => (
StatusCode::UNAUTHORIZED,
Json(ClientError::from(ClientErrorCode::UnauthorizedClient)),
)
.into_response(),

// If the token is unknown, we still return a 200 OK response.
Self::UnknownToken => StatusCode::OK.into_response(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have a nice view in some of those cases, in the end that is going to be a redirect, potentially where it doesn't go back to the client

};

(sentry_event_id, response).into_response()
}
}

impl From<IdTokenError> for RouteError {
fn from(_e: IdTokenError) -> Self {
Self::UnknownToken
}
}

#[tracing::instrument(name = "handlers.oauth2.end_session.get", skip_all)]
pub(crate) async fn get(
mut rng: BoxRng,
clock: BoxClock,
State(key_store): State<Keystore>,
State(url_builder): State<UrlBuilder>,
mut repo: BoxRepository,
activity_tracker: BoundActivityTracker,
Query(params): Query<EndSessionParam>,
cookie_jar: CookieJar,
) -> Result<Response, RouteError> {
let (session_info, cookie_jar) = cookie_jar.session_info();

let browser_session_id = session_info
.current_session_id()
.ok_or(RouteError::BadRequest)?;

let browser_session = repo
.browser_session()
.lookup(browser_session_id)
.await?
.ok_or(RouteError::BadRequest)?;

let oauth_session = repo
.oauth2_session()
.find_by_browser_session(browser_session.id)
.await?
.ok_or(RouteError::BadRequest)?;
Comment on lines +122 to +126
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can have many OAuth session started from a single browser session, so this way is not the right approach here.

Instead, best is to decode the JWT (but not necessarily verify it, maybe verify_id_token should be changed a little bit?) to get the claims we need and go from there? I'm unclear what we should be checking exactly on that JWT though, I don't know whether there is something that would let us link it to the current browser session or not 🤔


let client = repo
.oauth2_client()
.lookup(oauth_session.client_id)
.await?
.filter(|client| client.id_token_signed_response_alg.is_some())
.ok_or(RouteError::ClientNotFound)?;

let jwks = key_store.public_jwks();
let issuer: String = url_builder.oidc_issuer().into();

let id_token_verification_data = JwtVerificationData {
issuer: Some(&issuer),
jwks: &jwks,
signing_algorithm: &client.id_token_signed_response_alg.unwrap(),
client_id: &client.client_id,
};

verify_id_token(
&params.id_token_hint,
id_token_verification_data,
None,
clock.now(),
)?;

// Check that the session is still valid.
if !oauth_session.is_valid() {
// If the session is not valid, we redirect to post logout uri
return Ok((cookie_jar, Redirect::to(&params.post_logout_redirect_uri)).into_response());
}

// Check that the client ending the session is the same as the client that
// created it.
if client.id != oauth_session.client_id {
return Err(RouteError::UnauthorizedClient);
}

activity_tracker
.record_oauth2_session(&clock, &oauth_session)
.await;

// 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) = oauth_session.user_id {
// Fetch the user
let user = repo
.user()
.lookup(user_id)
.await?
.ok_or(RouteError::UnknownToken)?;

// Schedule a job to sync the devices of the user with the homeserver
repo.queue_job()
.schedule_job(&mut rng, &clock, SyncDevicesJob::new(&user))
.await?;
}

// Now that we checked everything, we can end the session.
repo.oauth2_session().finish(&clock, oauth_session).await?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about that semantic. I don't know whether we should finish the oauth session at this point or let the client still call the revocation endpoint.

Either way, we need a better way to find the right OAuth session, and I think this should be done by injecting the OAuth session ID in the id_token sid claim when we give out the id_token


activity_tracker
.record_browser_session(&clock, &browser_session)
.await;
repo.browser_session()
.finish(&clock, browser_session)
.await?;

repo.save().await?;

// We always want to clear out the session cookie, even if the session was
// invalid
let cookie_jar = cookie_jar.update_session_info(&session_info.mark_session_ended());

Ok((cookie_jar, Redirect::to(&params.post_logout_redirect_uri)).into_response())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec says we really should be validating the post_logout_redirect_uri, it should be registered by the client first

The value MUST have been previously registered with the OP, either using the post_logout_redirect_uris Registration parameter or via another mechanism.

}
1 change: 1 addition & 0 deletions crates/handlers/src/oauth2/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ use thiserror::Error;
pub mod authorization;
pub mod device;
pub mod discovery;
pub mod end_session;
pub mod introspection;
pub mod keys;
pub mod registration;
Expand Down
8 changes: 8 additions & 0 deletions crates/router/src/endpoints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,14 @@ impl SimpleRoute for OAuth2AuthorizationEndpoint {
const PATH: &'static str = "/authorize";
}

/// `POST /oauth2/end_session`
#[derive(Default, Debug, Clone)]
pub struct OAuth2EndSession;

impl SimpleRoute for OAuth2EndSession {
const PATH: &'static str = "/oauth2/end-session";
}

/// `GET /`
#[derive(Default, Debug, Clone)]
pub struct Index;
Expand Down
6 changes: 6 additions & 0 deletions crates/router/src/url_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,12 @@ impl UrlBuilder {
self.absolute_url_for(&crate::endpoints::OAuth2Revocation)
}

/// OAuth 2.0 revocation endpoint
#[must_use]
pub fn oauth_end_session_endpoint(&self) -> Url {
self.absolute_url_for(&crate::endpoints::OAuth2EndSession)
}

/// OAuth 2.0 client registration endpoint
#[must_use]
pub fn oauth_registration_endpoint(&self) -> Url {
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

39 changes: 39 additions & 0 deletions crates/storage-pg/src/oauth2/session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -592,4 +592,43 @@ impl OAuth2SessionRepository for PgOAuth2SessionRepository<'_> {

Ok(session)
}

#[tracing::instrument(
name = "db.oauth2_session.find_by_browser_session",
skip_all,
fields(
db.query.text,
session.id = %id,
),
err,
)]
async fn find_by_browser_session(&mut self, id: Ulid) -> Result<Option<Session>, Self::Error> {
let res = sqlx::query_as!(
OAuthSessionLookup,
r#"
SELECT oauth2_session_id
, user_id
, user_session_id
, oauth2_client_id
, scope_list
, created_at
, finished_at
, user_agent
, last_active_at
, last_active_ip as "last_active_ip: IpAddr"
, human_name
FROM oauth2_sessions

WHERE user_session_id = $1
"#,
Uuid::from(id),
)
.traced()
.fetch_optional(&mut *self.conn)
.await?;

let Some(session) = res else { return Ok(None) };

Ok(Some(session.try_into()?))
}
}
Loading
Loading