From fbd478af1749425906dbe76e59c4b2a0943f62f2 Mon Sep 17 00:00:00 2001 From: Aleksander <170264518+t-aleksander@users.noreply.github.com> Date: Tue, 9 Sep 2025 13:01:39 +0200 Subject: [PATCH 1/4] clear authorized apps after changing scope --- .../src/db/models/oauth2client.rs | 26 ++- .../src/handlers/openid_clients.rs | 10 +- .../tests/integration/api/openid.rs | 153 ++++++++++++++++++ 3 files changed, 182 insertions(+), 7 deletions(-) diff --git a/crates/defguard_core/src/db/models/oauth2client.rs b/crates/defguard_core/src/db/models/oauth2client.rs index 535bc8d628..ce3561e601 100644 --- a/crates/defguard_core/src/db/models/oauth2client.rs +++ b/crates/defguard_core/src/db/models/oauth2client.rs @@ -1,5 +1,5 @@ use model_derive::Model; -use sqlx::{Error as SqlxError, PgPool, query_as}; +use sqlx::{Error as SqlxError, PgExecutor, PgPool, query_as}; use super::NewOpenIDClient; use crate::{ @@ -55,20 +55,36 @@ impl OAuth2Client { impl OAuth2Client { /// Find client by 'client_id`. - pub(crate) async fn find_by_client_id( - pool: &PgPool, + pub(crate) async fn find_by_client_id<'e, E>( + executor: E, client_id: &str, - ) -> Result, SqlxError> { + ) -> Result, SqlxError> + where + E: PgExecutor<'e>, + { query_as!( Self, "SELECT id, client_id, client_secret, redirect_uri, scope, name, enabled \ FROM oauth2client WHERE client_id = $1", client_id ) - .fetch_optional(pool) + .fetch_optional(executor) .await } + pub(crate) async fn clear_authorizations<'e, E>(&self, executor: E) -> Result<(), SqlxError> + where + E: PgExecutor<'e>, + { + sqlx::query!( + "DELETE FROM oauth2authorizedapp WHERE oauth2client_id = $1", + self.id + ) + .execute(executor) + .await?; + Ok(()) + } + /// Find using `client_id` and `client_secret`; must be `enabled`. pub(crate) async fn find_by_auth( pool: &PgPool, diff --git a/crates/defguard_core/src/handlers/openid_clients.rs b/crates/defguard_core/src/handlers/openid_clients.rs index 0421b60996..341b05adc6 100644 --- a/crates/defguard_core/src/handlers/openid_clients.rs +++ b/crates/defguard_core/src/handlers/openid_clients.rs @@ -89,15 +89,21 @@ pub async fn change_openid_client( "User {} updating OpenID client {client_id}...", session.user.username ); - let status = match OAuth2Client::find_by_client_id(&appstate.pool, &client_id).await? { + let mut transaction = appstate.pool.begin().await?; + let status = match OAuth2Client::find_by_client_id(&mut *transaction, &client_id).await? { Some(mut client) => { // store client before mods let before = client.clone(); client.name = data.name; client.redirect_uri = data.redirect_uri; client.enabled = data.enabled; + let scope_differs = client.scope != data.scope; client.scope = data.scope; - client.save(&appstate.pool).await?; + client.save(&mut *transaction).await?; + if scope_differs { + client.clear_authorizations(&mut *transaction).await?; + } + transaction.commit().await?; info!( "User {} updated OpenID client {client_id} ({})", session.user.username, client.name diff --git a/crates/defguard_core/tests/integration/api/openid.rs b/crates/defguard_core/tests/integration/api/openid.rs index 4017100d92..7b701bba14 100644 --- a/crates/defguard_core/tests/integration/api/openid.rs +++ b/crates/defguard_core/tests/integration/api/openid.rs @@ -610,6 +610,159 @@ async fn test_openid_authorization_code_with_pkce(_: PgPoolOptions, options: PgC .unwrap(); } +#[sqlx::test] +async fn dg25_23_test_openid_client_scope_change_clears_authorizations( + _: PgPoolOptions, + options: PgConnectOptions, +) { + let pool = setup_pool(options).await; + let (client, state) = make_client_with_state(pool).await; + let admin = User::find_by_username(&state.pool, "admin") + .await + .unwrap() + .unwrap(); + + // Authenticate admin + let auth = Auth::new("admin", "pass123"); + let response = client.post("/api/v1/auth").json(&auth).send().await; + assert_eq!(response.status(), StatusCode::OK); + + // Create OAuth2 client with initial scopes + let oauth2client = NewOpenIDClient { + name: "Test Client".into(), + redirect_uri: vec!["http://localhost:3000/".into()], + scope: vec!["openid".into(), "email".into()], + enabled: true, + }; + + let response = client + .post("/api/v1/oauth") + .json(&oauth2client) + .send() + .await; + assert_eq!(response.status(), StatusCode::CREATED); + let oauth2client: OAuth2Client = response.json().await; + + // Authorize the client - simulate user authorization + let response = client + .post(format!( + "/api/v1/oauth/authorize?\ + response_type=code&\ + client_id={}&\ + redirect_uri=http%3A%2F%2Flocalhost%3A3000&\ + scope=openid email&\ + state=ABCDEF&\ + allow=true&\ + nonce=blabla", + oauth2client.client_id + )) + .send() + .await; + assert_eq!(response.status(), StatusCode::FOUND); + + // Verify that the authorization was created + use defguard_core::db::OAuth2AuthorizedApp; + let authorized_app = OAuth2AuthorizedApp::find_by_user_and_oauth2client_id( + &state.pool, + admin.id, + oauth2client.id, + ) + .await + .unwrap(); + assert!( + authorized_app.is_some(), + "Authorization should exist before scope change" + ); + + // Update the client with different scopes + let updated_client = NewOpenIDClient { + name: "Test Client".into(), + redirect_uri: vec!["http://localhost:3000/".into()], + scope: vec!["openid".into(), "profile".into()], // Changed from email to profile + enabled: true, + }; + + let response = client + .put(format!("/api/v1/oauth/{}", oauth2client.client_id)) + .json(&updated_client) + .send() + .await; + assert_eq!(response.status(), StatusCode::OK); + + // Verify that the authorization was cleared after scope change + let authorized_app_after = OAuth2AuthorizedApp::find_by_user_and_oauth2client_id( + &state.pool, + admin.id, + oauth2client.id, + ) + .await + .unwrap(); + assert!( + authorized_app_after.is_none(), + "Authorization should be cleared after scope change" + ); + + // Test that updating without scope changes does NOT clear authorizations + + // Re-authorize the client + let response = client + .post(format!( + "/api/v1/oauth/authorize?\ + response_type=code&\ + client_id={}&\ + redirect_uri=http%3A%2F%2Flocalhost%3A3000&\ + scope=openid profile&\ + state=ABCDEF2&\ + allow=true&\ + nonce=blabla2", + oauth2client.client_id + )) + .send() + .await; + assert_eq!(response.status(), StatusCode::FOUND); + + // Verify authorization exists again + let authorized_app = OAuth2AuthorizedApp::find_by_user_and_oauth2client_id( + &state.pool, + admin.id, + oauth2client.id, + ) + .await + .unwrap(); + assert!( + authorized_app.is_some(), + "Authorization should exist after re-authorization" + ); + + // Update the client without changing scopes (only name) + let same_scope_update = NewOpenIDClient { + name: "Test Client Updated Name".into(), + redirect_uri: vec!["http://localhost:3000/".into()], + scope: vec!["openid".into(), "profile".into()], // Same scopes + enabled: true, + }; + + let response = client + .put(format!("/api/v1/oauth/{}", oauth2client.client_id)) + .json(&same_scope_update) + .send() + .await; + assert_eq!(response.status(), StatusCode::OK); + + // Verify that the authorization still exists when scopes haven't changed + let authorized_app_preserved = OAuth2AuthorizedApp::find_by_user_and_oauth2client_id( + &state.pool, + admin.id, + oauth2client.id, + ) + .await + .unwrap(); + assert!( + authorized_app_preserved.is_some(), + "Authorization should be preserved when scopes don't change" + ); +} + #[sqlx::test] async fn dg25_22_test_respect_openid_scope_in_userinfo( _: PgPoolOptions, From e7a73467db592efdaa9582fa81fd04f0fa8ee1b3 Mon Sep 17 00:00:00 2001 From: Aleksander <170264518+t-aleksander@users.noreply.github.com> Date: Tue, 9 Sep 2025 13:03:01 +0200 Subject: [PATCH 2/4] fix --- crates/defguard_core/src/handlers/openid_clients.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/defguard_core/src/handlers/openid_clients.rs b/crates/defguard_core/src/handlers/openid_clients.rs index 341b05adc6..cc6a0bd52e 100644 --- a/crates/defguard_core/src/handlers/openid_clients.rs +++ b/crates/defguard_core/src/handlers/openid_clients.rs @@ -97,10 +97,9 @@ pub async fn change_openid_client( client.name = data.name; client.redirect_uri = data.redirect_uri; client.enabled = data.enabled; - let scope_differs = client.scope != data.scope; client.scope = data.scope; client.save(&mut *transaction).await?; - if scope_differs { + if before.scope != client.scope { client.clear_authorizations(&mut *transaction).await?; } transaction.commit().await?; From 8afcc7ff0fa49e45a14136d687e3d08e1f3e9e12 Mon Sep 17 00:00:00 2001 From: Aleksander <170264518+t-aleksander@users.noreply.github.com> Date: Tue, 9 Sep 2025 13:07:21 +0200 Subject: [PATCH 3/4] remove redundant header --- crates/defguard_core/src/handlers/mod.rs | 7 +------ crates/defguard_core/src/handlers/pagination.rs | 12 ++---------- 2 files changed, 3 insertions(+), 16 deletions(-) diff --git a/crates/defguard_core/src/handlers/mod.rs b/crates/defguard_core/src/handlers/mod.rs index 67b71a41fb..b088716a37 100644 --- a/crates/defguard_core/src/handlers/mod.rs +++ b/crates/defguard_core/src/handlers/mod.rs @@ -1,7 +1,7 @@ use axum::{ Json, extract::{FromRef, FromRequestParts}, - http::{HeaderName, HeaderValue, StatusCode, request::Parts}, + http::{StatusCode, request::Parts}, response::{IntoResponse, Response}, }; use axum_client_ip::InsecureClientIp; @@ -14,7 +14,6 @@ use webauthn_rs::prelude::RegisterPublicKeyCredential; #[cfg(feature = "wireguard")] use crate::db::Device; use crate::{ - VERSION, appstate::AppState, auth::SessionInfo, db::{Id, NoId, User, UserInfo, WebHook}, @@ -219,10 +218,6 @@ impl IntoResponse for WebError { impl IntoResponse for ApiResponse { fn into_response(self) -> Response { let mut response = Json(self.json).into_response(); - response.headers_mut().insert( - HeaderName::from_static("x-defguard-version"), - HeaderValue::from_static(VERSION), - ); *response.status_mut() = self.status; response } diff --git a/crates/defguard_core/src/handlers/pagination.rs b/crates/defguard_core/src/handlers/pagination.rs index c0c2bdd86e..6a4327a17c 100644 --- a/crates/defguard_core/src/handlers/pagination.rs +++ b/crates/defguard_core/src/handlers/pagination.rs @@ -1,12 +1,11 @@ use axum::{ body::Body, - http::{HeaderName, HeaderValue}, response::{IntoResponse, Response}, }; use reqwest::StatusCode; use serde::Serialize; -use crate::{VERSION, error::WebError}; +use crate::error::WebError; /// Query params for paginated endpoints #[derive(Debug, Deserialize, Default)] @@ -51,13 +50,6 @@ where } }; - let mut response = Response::new(Body::from(json)); - - response.headers_mut().insert( - HeaderName::from_static("x-defguard-version"), - HeaderValue::from_static(VERSION), - ); - - response + Response::new(Body::from(json)) } } From 7adb64b6215f3e3508235fd775e768caaa764100 Mon Sep 17 00:00:00 2001 From: Aleksander <170264518+t-aleksander@users.noreply.github.com> Date: Tue, 9 Sep 2025 13:23:51 +0200 Subject: [PATCH 4/4] prepare --- ...666a95924214dc1b175ba520bf417b63cb598478d9.json | 14 ++++++++++++++ crates/defguard_core/src/handlers/app_info.rs | 3 ++- 2 files changed, 16 insertions(+), 1 deletion(-) create mode 100644 .sqlx/query-7874a2d0b3a5a6b78bdb22666a95924214dc1b175ba520bf417b63cb598478d9.json diff --git a/.sqlx/query-7874a2d0b3a5a6b78bdb22666a95924214dc1b175ba520bf417b63cb598478d9.json b/.sqlx/query-7874a2d0b3a5a6b78bdb22666a95924214dc1b175ba520bf417b63cb598478d9.json new file mode 100644 index 0000000000..d0c08947b8 --- /dev/null +++ b/.sqlx/query-7874a2d0b3a5a6b78bdb22666a95924214dc1b175ba520bf417b63cb598478d9.json @@ -0,0 +1,14 @@ +{ + "db_name": "PostgreSQL", + "query": "DELETE FROM oauth2authorizedapp WHERE oauth2client_id = $1", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Int8" + ] + }, + "nullable": [] + }, + "hash": "7874a2d0b3a5a6b78bdb22666a95924214dc1b175ba520bf417b63cb598478d9" +} diff --git a/crates/defguard_core/src/handlers/app_info.rs b/crates/defguard_core/src/handlers/app_info.rs index e8126ecdbe..bc67e8c17f 100644 --- a/crates/defguard_core/src/handlers/app_info.rs +++ b/crates/defguard_core/src/handlers/app_info.rs @@ -1,8 +1,9 @@ use axum::{extract::State, http::StatusCode}; use serde_json::json; -use super::{ApiResponse, ApiResult, VERSION}; +use super::{ApiResponse, ApiResult}; use crate::{ + VERSION, appstate::AppState, auth::SessionInfo, db::{Settings, WireguardNetwork},