Skip to content
Open
Show file tree
Hide file tree
Changes from 2 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
211 changes: 211 additions & 0 deletions crates/handlers/src/oauth2/end_session.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,211 @@
// Copyright 2024, 2025 New Vector Ltd.
// Copyright 2023, 2024 The Matrix.org Foundation C.I.C.
//
// 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("unsupported token type")]
// UnsupportedTokenType,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// #[error("unsupported token type")]
// UnsupportedTokenType,

#[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::ClientNotAllowed |
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Self::ClientNotAllowed |

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

// Self::UnsupportedTokenType => (
// StatusCode::BAD_REQUEST,
// Json(ClientError::from(ClientErrorCode::UnsupportedTokenType)),
// )
// .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.

Suggested change
// Self::UnsupportedTokenType => (
// StatusCode::BAD_REQUEST,
// Json(ClientError::from(ClientErrorCode::UnsupportedTokenType)),
// )
// .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.

Loading
Loading