Skip to content
Merged
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: 2 additions & 2 deletions crates/defguard_core/src/handlers/openid_flow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -260,11 +260,11 @@ impl AuthenticationRequest {
&self,
oauth2client: &OAuth2Client<Id>,
) -> Result<(), CoreAuthErrorResponseType> {
// check scope: it is valid if any requested scope exists in the `oauth2client`
// check scope: it is valid only if all requested scopes exist in the `oauth2client`
if self
.scope
.split(' ')
.all(|scope| !oauth2client.scope.iter().any(|s| s == scope))
.any(|scope| !oauth2client.scope.iter().any(|s| s == scope))
{
error!(
"Invalid scope for client {}: {}",
Expand Down
127 changes: 127 additions & 0 deletions crates/defguard_core/tests/integration/api/oauth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -428,3 +428,130 @@ async fn test_token_client_credentials(_: PgPoolOptions, options: PgConnectOptio
.await;
assert_eq!(response.status(), StatusCode::BAD_REQUEST);
}

/// Regression test for DG26-6: scope parameter must be validated per individual element, not as a
/// whole. Sending scope=openid%20email when the client only allows ["openid"] must be rejected
/// with invalid_scope.
#[sqlx::test]
async fn dg26_6_test_authorize_scope_validation(_: PgPoolOptions, options: PgConnectOptions) {
let pool = setup_pool(options).await;
let (client, pool) = make_client_with_db(pool).await;

// authenticate
let auth = Auth::new("admin", "pass123");
let response = client.post("/api/v1/auth").json(&auth).send().await;
assert_eq!(response.status(), StatusCode::OK);

// create client with only "openid" scope
let oauth2client = NewOpenIDClient {
name: "Scope test client".into(),
redirect_uri: vec!["http://test.server.tnt:12345/".into()],
scope: vec!["openid".into()],
enabled: true,
};
let response = client
.post("/api/v1/oauth")
.json(&oauth2client)
.send()
.await;
assert_eq!(response.status(), StatusCode::CREATED);
let oauth2client: OAuth2Client<Id> = response.json().await;

// authorize client for the test user so session-based redirect returns a code, not consent
OAuth2AuthorizedApp::new(1, oauth2client.id)
.save(&pool)
.await
.unwrap();

// valid request: scope=openid (the only allowed scope) - must succeed
let response = client
.get(format!(
"/api/v1/oauth/authorize?\
response_type=code&\
client_id={}&\
redirect_uri=http%3A%2F%2Ftest.server.tnt%3A12345%2F&\
scope=openid&\
state=valid",
oauth2client.client_id
))
.send()
.await;
assert_eq!(response.status(), StatusCode::FOUND);
let location = Url::parse(
response
.headers()
.get("Location")
.unwrap()
.to_str()
.unwrap(),
)
.unwrap();
assert_eq!(location.domain().unwrap(), "test.server.tnt");
assert!(location.query().unwrap().contains("code="));

// forbidden single scope: scope=email - must be rejected
let response = client
.get(format!(
"/api/v1/oauth/authorize?\
response_type=code&\
client_id={}&\
redirect_uri=http%3A%2F%2Ftest.server.tnt%3A12345%2F&\
scope=email&\
state=forbidden_single",
oauth2client.client_id
))
.send()
.await;
assert_eq!(response.status(), StatusCode::FOUND);
let location = Url::parse(
response
.headers()
.get("Location")
.unwrap()
.to_str()
.unwrap(),
)
.unwrap();
let mut pairs = location.query_pairs();
assert_eq!(
pairs.next(),
Some((Cow::Borrowed("error"), Cow::Borrowed("invalid_scope")))
);
assert_eq!(
pairs.next(),
Some((Cow::Borrowed("state"), Cow::Borrowed("forbidden_single")))
);

// mixed scope=openid%20email - second token must not be accepted
let response = client
.get(format!(
"/api/v1/oauth/authorize?\
response_type=code&\
client_id={}&\
redirect_uri=http%3A%2F%2Ftest.server.tnt%3A12345%2F&\
scope=openid%20email&\
state=mixed",
oauth2client.client_id
))
.send()
.await;
assert_eq!(response.status(), StatusCode::FOUND);
let location = Url::parse(
response
.headers()
.get("Location")
.unwrap()
.to_str()
.unwrap(),
)
.unwrap();
let mut pairs = location.query_pairs();
assert_eq!(
pairs.next(),
Some((Cow::Borrowed("error"), Cow::Borrowed("invalid_scope")))
);
assert_eq!(
pairs.next(),
Some((Cow::Borrowed("state"), Cow::Borrowed("mixed")))
);
}
17 changes: 3 additions & 14 deletions crates/defguard_core/tests/integration/api/openid.rs
Original file line number Diff line number Diff line change
Expand Up @@ -505,7 +505,7 @@ async fn test_openid_authorization_code(_: PgPoolOptions, options: PgConnectOpti
let oauth2client = NewOpenIDClient {
name: "My test client".into(),
redirect_uri: vec![FAKE_REDIRECT_URI.into()],
scope: vec!["openid".into()],
scope: vec!["openid".into(), "email".into(), "profile".into()],
enabled: true,
};
let response = client
Expand Down Expand Up @@ -646,7 +646,7 @@ async fn dg25_20_test_openid_disabled_client_doesnt_generate_code(
let oauth2client = NewOpenIDClient {
name: "My test client".into(),
redirect_uri: vec![FAKE_REDIRECT_URI.into()],
scope: vec!["openid".into()],
scope: vec!["openid".into(), "email".into(), "profile".into()],
enabled: true,
};
let response = client
Expand Down Expand Up @@ -878,7 +878,7 @@ async fn test_openid_authorization_code_with_pkce(_: PgPoolOptions, options: PgC
let oauth2client = NewOpenIDClient {
name: "My test client".into(),
redirect_uri: vec![FAKE_REDIRECT_URI.into()],
scope: vec!["openid".into()],
scope: vec!["openid".into(), "email".into(), "profile".into()],
enabled: true,
};
let response = client
Expand Down Expand Up @@ -1463,17 +1463,6 @@ async fn dg25_22_test_respect_openid_scope_in_userinfo(
// Verify claims include only email, not phone
assert!(claims.email().is_some());
assert!(claims.phone_number().is_none());

// Client has only email scope, request phone
let claims = get_user_claims(
vec!["openid".to_string(), "email".to_string()],
vec!["email".to_string(), "phone".to_string()],
)
.await;

// Verify claims include only email since client doesn't have phone scope
assert!(claims.email().is_some());
assert!(claims.phone_number().is_none());
}

#[sqlx::test]
Expand Down
Loading