Improve OAuth2 authorisation ux (#3158)

- Resolve an issue where oauth2 could trigger the login page to
  incorrectly redirect to an oauth2 application instead of apps
- Add indication of what client application we are accessing
  if the session is not yet authenticated

server/core/src/actors/v1_read.rs

@@ -1422,13 +1422,13 @@ impl QueryServerReadV1 {
             .map_err(Oauth2Error::ServerError)?;
         let ident = idms_prox_read
             .validate_client_auth_info_to_ident(client_auth_info, ct)
-            .map_err(|e| {
+            .inspect_err(|e| {
                 error!("Invalid identity: {:?}", e);
-                Oauth2Error::AuthenticationRequired
+            })
-            })?;
+            .ok();
 
         // Now we can send to the idm server for authorisation checking.
-        idms_prox_read.check_oauth2_authorisation(&ident, &auth_req, ct)
+        idms_prox_read.check_oauth2_authorisation(ident.as_ref(), &auth_req, ct)
     }
 
     #[instrument(

server/core/src/https/oauth2.rs

@@ -289,7 +289,8 @@ async fn oauth2_authorise(
                 .body(body)
                 .unwrap()
         }
-        Err(Oauth2Error::AuthenticationRequired) => {
+        Ok(AuthoriseResponse::AuthenticationRequired { .. })
+        | Err(Oauth2Error::AuthenticationRequired) => {
             // This will trigger our ui to auth and retry.
             #[allow(clippy::unwrap_used)]
             Response::builder()

server/core/src/https/views/constants.rs

@@ -28,6 +28,7 @@ pub(crate) enum Urls {
     UpdateCredentials,
     Oauth2Resume,
     Login,
+    Ui,
 }
 
 impl AsRef<str> for Urls {
@@ -39,6 +40,7 @@ impl AsRef<str> for Urls {
             Self::UpdateCredentials => "/ui/update_credentials",
             Self::Oauth2Resume => "/ui/oauth2/resume",
             Self::Login => "/ui/login",
+            Self::Ui => "/ui",
         }
     }
 }

server/core/src/https/views/login.rs

@@ -76,10 +76,15 @@ pub struct Reauth {
     pub purpose: ReauthPurpose,
 }
 
+pub struct Oauth2Ctx {
+    pub client_name: String,
+}
+
 pub struct LoginDisplayCtx {
     pub domain_info: DomainInfoRead,
     // We only need this on the first re-auth screen to indicate what we are doing
     pub reauth: Option<Reauth>,
+    pub oauth2: Option<Oauth2Ctx>,
     pub error: Option<LoginError>,
 }
 
@@ -182,6 +187,17 @@ pub async fn view_reauth_get(
     return_location: &str,
     display_ctx: LoginDisplayCtx,
 ) -> Response {
+    // No matter what, we always clear the stored oauth2 cookie to prevent
+    // ui loops
+    let jar = if let Some(authreq_cookie) = jar.get(COOKIE_OAUTH2_REQ) {
+        let mut authreq_cookie = authreq_cookie.clone();
+        authreq_cookie.make_removal();
+        authreq_cookie.set_path(Urls::Ui.as_ref());
+        jar.add(authreq_cookie)
+    } else {
+        jar
+    };
+
     let session_valid_result = state
         .qe_r_ref
         .handle_auth_valid(client_auth_info.clone(), kopid.eventid)
@@ -247,11 +263,14 @@ pub async fn view_reauth_get(
 
             let remember_me = !username.is_empty();
 
+            (
+                jar,
                 LoginView {
                     display_ctx,
                     username,
                     remember_me,
-            }
+                },
+            )
                 .into_response()
         }
         Err(err_code) => UnrecoverableErrorView {
@@ -262,6 +281,33 @@ pub async fn view_reauth_get(
     }
 }
 
+pub fn view_oauth2_get(
+    jar: CookieJar,
+    display_ctx: LoginDisplayCtx,
+    login_hint: Option<String>,
+) -> Response {
+    let (username, remember_me) = if let Some(login_hint) = login_hint {
+        (login_hint, false)
+    } else if let Some(cookie_username) =
+        // cookie jar with remember me.
+        jar.get(COOKIE_USERNAME).map(|c| c.value().to_string())
+    {
+        (cookie_username, true)
+    } else {
+        (String::default(), false)
+    };
+
+    (
+        jar,
+        LoginView {
+            display_ctx,
+            username,
+            remember_me,
+        },
+    )
+        .into_response()
+}
+
 pub async fn view_index_get(
     State(state): State<ServerState>,
     VerifiedClientInformation(client_auth_info): VerifiedClientInformation,
@@ -275,10 +321,21 @@ pub async fn view_index_get(
         .handle_auth_valid(client_auth_info, kopid.eventid)
         .await;
 
+    // No matter what, we always clear the stored oauth2 cookie to prevent
+    // ui loops
+    let jar = if let Some(authreq_cookie) = jar.get(COOKIE_OAUTH2_REQ) {
+        let mut authreq_cookie = authreq_cookie.clone();
+        authreq_cookie.make_removal();
+        authreq_cookie.set_path(Urls::Ui.as_ref());
+        jar.add(authreq_cookie)
+    } else {
+        jar
+    };
+
     match session_valid_result {
         Ok(()) => {
             // Send the user to the landing.
-            Redirect::to(Urls::Apps.as_ref()).into_response()
+            (jar, Redirect::to(Urls::Apps.as_ref())).into_response()
         }
         Err(OperationError::NotAuthenticated) | Err(OperationError::SessionExpired) => {
             // cookie jar with remember me.
@@ -291,15 +348,19 @@ pub async fn view_index_get(
 
             let display_ctx = LoginDisplayCtx {
                 domain_info,
+                oauth2: None,
                 reauth: None,
                 error: None,
             };
 
+            (
+                jar,
                 LoginView {
                     display_ctx,
                     username,
                     remember_me,
-            }
+                },
+            )
                 .into_response()
         }
         Err(err_code) => UnrecoverableErrorView {
@@ -368,6 +429,7 @@ pub async fn view_login_begin_post(
 
     let mut display_ctx = LoginDisplayCtx {
         domain_info,
+        oauth2: None,
         reauth: None,
         error: None,
     };
@@ -450,6 +512,7 @@ pub async fn view_login_mech_choose_post(
 
     let display_ctx = LoginDisplayCtx {
         domain_info,
+        oauth2: None,
         reauth: None,
         error: None,
     };
@@ -505,6 +568,7 @@ pub async fn view_login_totp_post(
         Err(_) => {
             let display_ctx = LoginDisplayCtx {
                 domain_info,
+                oauth2: None,
                 reauth: None,
                 error: None,
             };
@@ -610,6 +674,7 @@ async fn credential_step(
 
     let display_ctx = LoginDisplayCtx {
         domain_info,
+        oauth2: None,
         reauth: None,
         error: None,
     };

server/core/src/https/views/oauth2.rs

@@ -1,4 +1,8 @@
-use crate::https::{extractors::VerifiedClientInformation, middleware::KOpId, ServerState};
+use crate::https::{
+    extractors::{DomainInfo, DomainInfoRead, VerifiedClientInformation},
+    middleware::KOpId,
+    ServerState,
+};
 use kanidmd_lib::idm::oauth2::{
     AuthorisationRequest, AuthorisePermitSuccess, AuthoriseResponse, Oauth2Error,
 };
@@ -23,6 +27,7 @@ use axum_htmx::HX_REDIRECT;
 use serde::Deserialize;
 
 use super::constants::Urls;
+use super::login::{LoginDisplayCtx, Oauth2Ctx};
 use super::{cookies, UnrecoverableErrorView};
 
 #[derive(Template)]
@@ -45,23 +50,25 @@ pub async fn view_index_get(
     State(state): State<ServerState>,
     Extension(kopid): Extension<KOpId>,
     VerifiedClientInformation(client_auth_info): VerifiedClientInformation,
+    DomainInfo(domain_info): DomainInfo,
     jar: CookieJar,
     Query(auth_req): Query<AuthorisationRequest>,
 ) -> Response {
-    oauth2_auth_req(state, kopid, client_auth_info, jar, auth_req).await
+    oauth2_auth_req(state, kopid, client_auth_info, domain_info, jar, auth_req).await
 }
 
 pub async fn view_resume_get(
     State(state): State<ServerState>,
     Extension(kopid): Extension<KOpId>,
     VerifiedClientInformation(client_auth_info): VerifiedClientInformation,
+    DomainInfo(domain_info): DomainInfo,
     jar: CookieJar,
 ) -> Result<Response, UnrecoverableErrorView> {
     let maybe_auth_req =
         cookies::get_signed::<AuthorisationRequest>(&state, &jar, COOKIE_OAUTH2_REQ);
 
     if let Some(auth_req) = maybe_auth_req {
-        Ok(oauth2_auth_req(state, kopid, client_auth_info, jar, auth_req).await)
+        Ok(oauth2_auth_req(state, kopid, client_auth_info, domain_info, jar, auth_req).await)
     } else {
         error!("unable to resume session, no auth_req was found in the cookie");
         Err(UnrecoverableErrorView {
@@ -75,6 +82,7 @@ async fn oauth2_auth_req(
     state: ServerState,
     kopid: KOpId,
     client_auth_info: ClientAuthInfo,
+    domain_info: DomainInfoRead,
     jar: CookieJar,
     auth_req: AuthorisationRequest,
 ) -> Response {
@@ -83,21 +91,23 @@ async fn oauth2_auth_req(
         .handle_oauth2_authorise(client_auth_info, auth_req.clone(), kopid.eventid)
         .await;
 
+    // No matter what, we always clear the stored oauth2 cookie to prevent
+    // ui loops
+    let jar = if let Some(authreq_cookie) = jar.get(COOKIE_OAUTH2_REQ) {
+        let mut authreq_cookie = authreq_cookie.clone();
+        authreq_cookie.make_removal();
+        authreq_cookie.set_path(Urls::Ui.as_ref());
+        jar.add(authreq_cookie)
+    } else {
+        jar
+    };
+
     match res {
         Ok(AuthoriseResponse::Permitted(AuthorisePermitSuccess {
             mut redirect_uri,
             state,
             code,
         })) => {
-            let jar = if let Some(authreq_cookie) = jar.get(COOKIE_OAUTH2_REQ) {
-                let mut authreq_cookie = authreq_cookie.clone();
-                authreq_cookie.make_removal();
-                authreq_cookie.set_path("/ui");
-                jar.add(authreq_cookie)
-            } else {
-                jar
-            };
-
             redirect_uri
                 .query_pairs_mut()
                 .clear()
@@ -133,9 +143,15 @@ async fn oauth2_auth_req(
             }
             .into_response()
         }
-        Err(Oauth2Error::AuthenticationRequired) => {
+
-            // Sign the auth req and hide it in our cookie.
+        Ok(AuthoriseResponse::AuthenticationRequired {
-            let maybe_jar = cookies::make_signed(&state, COOKIE_OAUTH2_REQ, &auth_req, "/ui")
+            client_name,
+            login_hint,
+        }) => {
+            // Sign the auth req and hide it in our cookie - we'll come back for
+            // you later.
+            let maybe_jar =
+                cookies::make_signed(&state, COOKIE_OAUTH2_REQ, &auth_req, Urls::Ui.as_ref())
                     .map(|mut cookie| {
                         cookie.set_same_site(SameSite::Strict);
                         jar.add(cookie)
@@ -143,7 +159,16 @@ async fn oauth2_auth_req(
                     .ok_or(OperationError::InvalidSessionState);
 
             match maybe_jar {
-                Ok(jar) => (jar, Redirect::to(Urls::Login.as_ref())).into_response(),
+                Ok(jar) => {
+                    let display_ctx = LoginDisplayCtx {
+                        domain_info,
+                        oauth2: Some(Oauth2Ctx { client_name }),
+                        reauth: None,
+                        error: None,
+                    };
+
+                    super::login::view_oauth2_get(jar, display_ctx, login_hint)
+                }
                 Err(err_code) => UnrecoverableErrorView {
                     err_code,
                     operation_id: kopid.eventid,

server/core/src/https/views/profile.rs

@@ -73,6 +73,7 @@ pub(crate) async fn view_profile_unlock_get(
 
     let display_ctx = LoginDisplayCtx {
         domain_info,
+        oauth2: None,
         reauth: Some(Reauth {
             username: uat.spn,
             purpose: ReauthPurpose::ProfileSettings,

server/core/src/https/views/reset.rs

@@ -653,6 +653,7 @@ pub(crate) async fn view_self_reset_get(
     } else {
         let display_ctx = LoginDisplayCtx {
             domain_info,
+            oauth2: None,
             reauth: Some(Reauth {
                 username: uat.spn,
                 purpose: ReauthPurpose::ProfileSettings,

server/core/templates/login_base.html

@@ -20,6 +20,10 @@
 	<div class="alert alert-info" role="alert">
 		Reauthenticating as (( reauth.username )) to access (( reauth.purpose ))
 	</div>
+	(% else if let Some(oauth2) = display_ctx.oauth2 %)
+	<div class="alert alert-info" role="alert">
+		Authenticate to access (( oauth2.client_name ))
+	</div>
 	(% endif %)
 	<div>
 		(% block logincontainer %)

server/lib/src/idm/oauth2.rs

@@ -208,6 +208,12 @@ impl fmt::Display for Oauth2TokenType {
 
 #[derive(Debug)]
 pub enum AuthoriseResponse {
+    AuthenticationRequired {
+        // A pretty-name of the client
+        client_name: String,
+        // A username hint, if any
+        login_hint: Option<String>,
+    },
     ConsentRequested {
         // A pretty-name of the client
         client_name: String,
@@ -1775,7 +1781,7 @@ impl<'a> IdmServerProxyReadTransaction<'a> {
     #[instrument(level = "debug", skip_all)]
     pub fn check_oauth2_authorisation(
         &self,
-        ident: &Identity,
+        maybe_ident: Option<&Identity>,
         auth_req: &AuthorisationRequest,
         ct: Duration,
     ) -> Result<AuthoriseResponse, Oauth2Error> {
@@ -1889,6 +1895,11 @@ impl<'a> IdmServerProxyReadTransaction<'a> {
             None
         };
 
+        // =============================================================================
+        // By this point, we have validated the majority of the security related
+        // parameters of the request. We can now inspect the identity and decide
+        // if we should ask the user to re-authenticate and proceed.
+
         // TODO: https://openid.net/specs/openid-connect-basic-1_0.html#RequestParameters
         // Are we going to provide the functions for these? Most of these can be "later".
         // IF CHANGED: Update OidcDiscoveryResponse!!!
@@ -1908,7 +1919,13 @@ impl<'a> IdmServerProxyReadTransaction<'a> {
 
         // TODO: id_token_hint - a past token which can be used as a hint.
 
-        // NOTE: login_hint is handled in the UI code, not here.
+        let Some(ident) = maybe_ident else {
+            debug!("No identity available, assume authentication required");
+            return Ok(AuthoriseResponse::AuthenticationRequired {
+                client_name: o2rs.displayname.clone(),
+                login_hint: auth_req.oidc_ext.login_hint.clone(),
+            });
+        };
 
         let Some(account_uuid) = ident.get_uuid() else {
             error!("consent request ident does not have a valid uuid, unable to proceed");
@@ -2939,7 +2956,7 @@ mod tests {
             };
 
             $idms_prox_read
-                .check_oauth2_authorisation($ident, &auth_req, $ct)
+                .check_oauth2_authorisation(Some($ident), &auth_req, $ct)
                 .expect("OAuth2 authorisation failed")
         }};
     }
@@ -3431,7 +3448,7 @@ mod tests {
 
         assert!(
             idms_prox_read
-                .check_oauth2_authorisation(&ident, &auth_req, ct)
+                .check_oauth2_authorisation(Some(&ident), &auth_req, ct)
                 .unwrap_err()
                 == Oauth2Error::UnsupportedResponseType
         );
@@ -3451,7 +3468,7 @@ mod tests {
 
         assert!(
             idms_prox_read
-                .check_oauth2_authorisation(&ident, &auth_req, ct)
+                .check_oauth2_authorisation(Some(&ident), &auth_req, ct)
                 .unwrap_err()
                 == Oauth2Error::InvalidRequest
         );
@@ -3471,7 +3488,7 @@ mod tests {
 
         assert!(
             idms_prox_read
-                .check_oauth2_authorisation(&ident, &auth_req, ct)
+                .check_oauth2_authorisation(Some(&ident), &auth_req, ct)
                 .unwrap_err()
                 == Oauth2Error::InvalidClientId
         );
@@ -3491,7 +3508,7 @@ mod tests {
 
         assert!(
             idms_prox_read
-                .check_oauth2_authorisation(&ident, &auth_req, ct)
+                .check_oauth2_authorisation(Some(&ident), &auth_req, ct)
                 .unwrap_err()
                 == Oauth2Error::InvalidOrigin
         );
@@ -3511,11 +3528,33 @@ mod tests {
 
         assert!(
             idms_prox_read
-                .check_oauth2_authorisation(&ident, &auth_req, ct)
+                .check_oauth2_authorisation(Some(&ident), &auth_req, ct)
                 .unwrap_err()
                 == Oauth2Error::InvalidOrigin
         );
 
+        // Not Authenticated
+        let auth_req = AuthorisationRequest {
+            response_type: "code".to_string(),
+            client_id: "test_resource_server".to_string(),
+            state: "123".to_string(),
+            pkce_request: pkce_request.clone(),
+            redirect_uri: Url::parse("https://demo.example.com/oauth2/result").unwrap(),
+            scope: OAUTH2_SCOPE_OPENID.to_string(),
+            nonce: None,
+            oidc_ext: Default::default(),
+            unknown_keys: Default::default(),
+        };
+
+        let req = idms_prox_read
+            .check_oauth2_authorisation(None, &auth_req, ct)
+            .unwrap();
+
+        assert!(matches!(
+            req,
+            AuthoriseResponse::AuthenticationRequired { .. }
+        ));
+
         // Requested scope is not available
         let auth_req = AuthorisationRequest {
             response_type: "code".to_string(),
@@ -3531,7 +3570,7 @@ mod tests {
 
         assert!(
             idms_prox_read
-                .check_oauth2_authorisation(&ident, &auth_req, ct)
+                .check_oauth2_authorisation(Some(&ident), &auth_req, ct)
                 .unwrap_err()
                 == Oauth2Error::AccessDenied
         );
@@ -3551,7 +3590,7 @@ mod tests {
 
         assert!(
             idms_prox_read
-                .check_oauth2_authorisation(&idm_admin_ident, &auth_req, ct)
+                .check_oauth2_authorisation(Some(&idm_admin_ident), &auth_req, ct)
                 .unwrap_err()
                 == Oauth2Error::AccessDenied
         );
@@ -3571,7 +3610,7 @@ mod tests {
 
         assert!(
             idms_prox_read
-                .check_oauth2_authorisation(&anon_ident, &auth_req, ct)
+                .check_oauth2_authorisation(Some(&anon_ident), &auth_req, ct)
                 .unwrap_err()
                 == Oauth2Error::AccessDenied
         );
@@ -3863,7 +3902,7 @@ mod tests {
         };
 
         let consent_request = idms_prox_read
-            .check_oauth2_authorisation(&ident, &auth_req, ct)
+            .check_oauth2_authorisation(Some(&ident), &auth_req, ct)
             .expect("OAuth2 authorisation failed");
 
         trace!(?consent_request);
@@ -3932,7 +3971,7 @@ mod tests {
         };
 
         let consent_request = idms_prox_read
-            .check_oauth2_authorisation(&ident, &auth_req, ct)
+            .check_oauth2_authorisation(Some(&ident), &auth_req, ct)
             .expect("OAuth2 authorisation failed");
 
         trace!(?consent_request);
@@ -5134,7 +5173,7 @@ mod tests {
         };
 
         idms_prox_read
-            .check_oauth2_authorisation(&ident, &auth_req, ct)
+            .check_oauth2_authorisation(Some(&ident), &auth_req, ct)
             .expect("Oauth2 authorisation failed");
     }
 
@@ -5347,7 +5386,7 @@ mod tests {
         };
 
         let consent_request = idms_prox_read
-            .check_oauth2_authorisation(&ident, &auth_req, ct)
+            .check_oauth2_authorisation(Some(&ident), &auth_req, ct)
             .expect("Oauth2 authorisation failed");
 
         // Should be in the consent phase;
@@ -5405,7 +5444,7 @@ mod tests {
         };
 
         let consent_request = idms_prox_read
-            .check_oauth2_authorisation(&ident, &auth_req, ct)
+            .check_oauth2_authorisation(Some(&ident), &auth_req, ct)
             .expect("Oauth2 authorisation failed");
 
         // Should be present in the consent phase however!
@@ -5542,7 +5581,7 @@ mod tests {
         };
 
         let consent_request = idms_prox_read
-            .check_oauth2_authorisation(&ident, &auth_req, ct)
+            .check_oauth2_authorisation(Some(&ident), &auth_req, ct)
             .expect("Failed to perform OAuth2 authorisation request.");
 
         // Should be in the consent phase;
@@ -5620,7 +5659,7 @@ mod tests {
 
         assert!(
             idms_prox_read
-                .check_oauth2_authorisation(&ident, &auth_req, ct)
+                .check_oauth2_authorisation(Some(&ident), &auth_req, ct)
                 .unwrap_err()
                 == Oauth2Error::InvalidOrigin
         );
@@ -6575,7 +6614,7 @@ mod tests {
         };
 
         let consent_request = idms_prox_read
-            .check_oauth2_authorisation(&ident, &auth_req, ct)
+            .check_oauth2_authorisation(Some(&ident), &auth_req, ct)
             .expect("OAuth2 authorisation failed");
 
         // Should be in the consent phase;