diff --git a/crates/handlers/src/oauth2/token.rs b/crates/handlers/src/oauth2/token.rs index 0d9f4b30..a9abbe5c 100644 --- a/crates/handlers/src/oauth2/token.rs +++ b/crates/handlers/src/oauth2/token.rs @@ -30,6 +30,7 @@ use mas_storage::{ oauth2::{ access_token::{add_access_token, revoke_access_token}, authorization_grant::{exchange_grant, lookup_grant_by_code}, + end_oauth_session, refresh_token::{add_refresh_token, lookup_active_refresh_token, replace_refresh_token}, }, DatabaseInconsistencyError, @@ -184,6 +185,9 @@ async fn authorization_code_grant( .await .wrap_error()?; + // TODO: that's not a timestamp from the DB. Let's assume they are in sync + let now = Utc::now(); + let session = match authz_grant.stage { AuthorizationGrantStage::Cancelled { cancelled_at } => { debug!(%cancelled_at, "Authorization grant was cancelled"); @@ -192,12 +196,17 @@ async fn authorization_code_grant( AuthorizationGrantStage::Exchanged { exchanged_at, fulfilled_at, - session: _, + session, } => { - // TODO: we should invalidate the existing session if a code is used twice after - // some period of time. See the `oidcc-codereuse-30seconds` test from the - // conformance suite debug!(%exchanged_at, %fulfilled_at, "Authorization code was already exchanged"); + + // Ending the session if the token was already exchanged more than 20s ago + if now - exchanged_at > Duration::seconds(20) { + debug!("Ending potentially compromised session"); + end_oauth_session(&mut txn, session).await.wrap_error()?; + txn.commit().await.wrap_error()?; + } + return error(InvalidGrant); } AuthorizationGrantStage::Pending => { @@ -206,10 +215,13 @@ async fn authorization_code_grant( } AuthorizationGrantStage::Fulfilled { ref session, - fulfilled_at: _, + fulfilled_at, } => { - // TODO: we should check that the session was not fullfilled too long ago - // (30s to 1min?). The main problem is getting a timestamp from the database + if now - fulfilled_at > Duration::minutes(10) { + debug!("Code exchange took more than 10 minutes"); + return error(InvalidGrant); + } + session } }; diff --git a/crates/storage/migrations/20220114112012_oauth_session_end.down.sql b/crates/storage/migrations/20220114112012_oauth_session_end.down.sql new file mode 100644 index 00000000..d209e665 --- /dev/null +++ b/crates/storage/migrations/20220114112012_oauth_session_end.down.sql @@ -0,0 +1,16 @@ +-- Copyright 2021 The Matrix.org Foundation C.I.C. +-- +-- Licensed under the Apache License, Version 2.0 (the "License"); +-- you may not use this file except in compliance with the License. +-- You may obtain a copy of the License at +-- +-- http://www.apache.org/licenses/LICENSE-2.0 +-- +-- Unless required by applicable law or agreed to in writing, software +-- distributed under the License is distributed on an "AS IS" BASIS, +-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +-- See the License for the specific language governing permissions and +-- limitations under the License. + +ALTER TABLE oauth2_sessions + DROP COLUMN "ended_at"; diff --git a/crates/storage/migrations/20220114112012_oauth_session_end.up.sql b/crates/storage/migrations/20220114112012_oauth_session_end.up.sql new file mode 100644 index 00000000..87f800d6 --- /dev/null +++ b/crates/storage/migrations/20220114112012_oauth_session_end.up.sql @@ -0,0 +1,16 @@ +-- Copyright 2021 The Matrix.org Foundation C.I.C. +-- +-- Licensed under the Apache License, Version 2.0 (the "License"); +-- you may not use this file except in compliance with the License. +-- You may obtain a copy of the License at +-- +-- http://www.apache.org/licenses/LICENSE-2.0 +-- +-- Unless required by applicable law or agreed to in writing, software +-- distributed under the License is distributed on an "AS IS" BASIS, +-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +-- See the License for the specific language governing permissions and +-- limitations under the License. + +ALTER TABLE oauth2_sessions + ADD COLUMN "ended_at" TIMESTAMP WITH TIME ZONE DEFAULT NULL; diff --git a/crates/storage/src/oauth2/access_token.rs b/crates/storage/src/oauth2/access_token.rs index 1b4f730b..cfde16e9 100644 --- a/crates/storage/src/oauth2/access_token.rs +++ b/crates/storage/src/oauth2/access_token.rs @@ -125,6 +125,7 @@ pub async fn lookup_active_access_token( WHERE at.token = $1 AND at.created_at + (at.expires_after * INTERVAL '1 second') >= now() AND us.active + AND os.ended_at IS NULL ORDER BY usa.created_at DESC LIMIT 1 diff --git a/crates/storage/src/oauth2/mod.rs b/crates/storage/src/oauth2/mod.rs index 46d9d516..a81e9873 100644 --- a/crates/storage/src/oauth2/mod.rs +++ b/crates/storage/src/oauth2/mod.rs @@ -12,6 +12,31 @@ // See the License for the specific language governing permissions and // limitations under the License. +use mas_data_model::Session; +use sqlx::PgExecutor; + +use crate::PostgresqlBackend; + pub mod access_token; pub mod authorization_grant; pub mod refresh_token; + +pub async fn end_oauth_session( + executor: impl PgExecutor<'_>, + session: Session, +) -> anyhow::Result<()> { + let res = sqlx::query!( + r#" + UPDATE oauth2_sessions + SET ended_at = NOW() + WHERE id = $1 + "#, + session.data, + ) + .execute(executor) + .await?; + + anyhow::ensure!(res.rows_affected() == 1); + + Ok(()) +} diff --git a/crates/storage/src/oauth2/refresh_token.rs b/crates/storage/src/oauth2/refresh_token.rs index 6339c164..e5f91c6b 100644 --- a/crates/storage/src/oauth2/refresh_token.rs +++ b/crates/storage/src/oauth2/refresh_token.rs @@ -112,6 +112,7 @@ pub async fn lookup_active_refresh_token( WHERE rt.token = $1 AND rt.next_token_id IS NULL AND us.active + AND os.ended_at IS NULL ORDER BY usa.created_at DESC LIMIT 1