Merge pull request #136 from himmelblau-idm/dmulder/group_lookup_failure

Fix group lookup failure

Cargo.toml

@@ -38,7 +38,7 @@ tracing-subscriber = "^0.3.17"
 tracing = "^0.1.37"
 himmelblau_unix_common = { path = "src/common" }
 kanidm_unix_common = { path = "src/glue" }
-msal = { version = "0.1.24" }
+msal = { version = "0.2.0" }
 graph = { path = "src/graph" }
 clap = { version = "^4.5", features = ["derive", "env"] }
 clap_complete = "^4.4.1"

src/common/src/idprovider/himmelblau.rs

@@ -19,9 +19,7 @@ use msal::auth::{
     DeviceAuthorizationResponse as msal_DeviceAuthorizationResponse, EnrollAttrs, IdToken,
     MFAAuthContinue, UserToken as UnixUserToken,
 };
-use msal::error::{
-    MsalError, AUTH_PENDING, DEVICE_AUTH_FAIL, NO_CONSENT, NO_GROUP_CONSENT, REQUIRES_MFA,
-};
+use msal::error::{MsalError, AUTH_PENDING, DEVICE_AUTH_FAIL, REQUIRES_MFA};
 use reqwest;
 use std::collections::HashMap;
 use std::sync::Arc;
@@ -575,44 +573,20 @@ impl IdProvider for HimmelblauProvider {
             Ok(prt) => prt,
             Err(_) => fake_user!(),
         };
-        let scopes = vec!["GroupMember.Read.All"];
         let token = match self
             .client
             .write()
             .await
-            .exchange_prt_for_access_token(&prt, scopes, tpm, machine_key, None)
+            .exchange_prt_for_access_token(
+                &prt,
+                vec!["User.Read"],
+                Some("https://graph.microsoft.com".to_string()),
+                tpm,
+                machine_key,
+            )
             .await
         {
             Ok(token) => token,
-            Err(MsalError::AcquireTokenFailed(resp)) => {
-                // We may have been denied GroupMember.Read.All, try again without it
-                if resp.error_codes.contains(&NO_GROUP_CONSENT)
-                    || resp.error_codes.contains(&NO_CONSENT)
-                {
-                    debug!("Failed auth with GroupMember.Read.All permissions.");
-                    debug!("Group memberships will be missing display names.");
-                    debug!("{}: {}", resp.error, resp.error_description);
-                    match self
-                        .client
-                        .write()
-                        .await
-                        .exchange_prt_for_access_token(&prt, vec![], tpm, machine_key, None)
-                        .await
-                    {
-                        Ok(token) => token,
-                        Err(e) => {
-                            error!("{:?}", e);
-                            // Never return IdpError::NotFound. This deletes
-                            // the existing user from the cache.
-                            fake_user!()
-                        }
-                    }
-                } else {
-                    // Never return IdpError::NotFound. This deletes the
-                    // existing user from the cache.
-                    fake_user!()
-                }
-            }
             Err(e) => {
                 error!("{:?}", e);
                 // Never return IdpError::NotFound. This deletes the existing
@@ -688,8 +662,8 @@ impl IdProvider for HimmelblauProvider {
                     .await
                     .acquire_token_by_refresh_token(
                         &$token.refresh_token,
-                        vec![],
-                        None,
+                        vec!["User.Read"],
+                        Some("https://graph.microsoft.com".to_string()),
                         tpm,
                         machine_key,
                     )
@@ -712,8 +686,8 @@ impl IdProvider for HimmelblauProvider {
                                         .await
                                         .acquire_token_by_refresh_token(
                                             &$token.refresh_token,
-                                            vec![],
-                                            None,
+                                            vec!["User.Read"],
+                                            Some("https://graph.microsoft.com".to_string()),
                                             tpm,
                                             machine_key,
                                         )
@@ -741,7 +715,8 @@ impl IdProvider for HimmelblauProvider {
                     .acquire_token_by_hello_for_business_key(
                         account_id,
                         &$hello_key,
-                        vec![],
+                        vec!["User.Read"],
+                        Some("https://graph.microsoft.com".to_string()),
                         tpm,
                         machine_key,
                         &$cred,
@@ -1458,7 +1433,7 @@ impl HimmelblauProvider {
     ) -> Result<(), MsalError> {
         /* If not already joined, join the domain now. */
         let attrs = EnrollAttrs::new(self.domain.clone(), None, None, None, None)?;
-        return match self
+        match self
             .client
             .write()
             .await
@@ -1506,19 +1481,6 @@ impl HimmelblauProvider {
                     "Setting domain {} config authority_host to {}",
                     self.domain, &self.authority_host
                 );
-                let mut allow_groups = match config.get(&self.domain, "pam_allow_groups") {
-                    Some(allowed) => allowed.split(',').map(|g| g.to_string()).collect(),
-                    None => vec![],
-                };
-                allow_groups.push(token.spn()?.clone());
-                /* Remove duplicates from the allow_groups */
-                allow_groups.sort();
-                allow_groups.dedup();
-                config.set(&self.domain, "pam_allow_groups", &allow_groups.join(","));
-                debug!(
-                    "Setting global pam_allow_groups to {}",
-                    &allow_groups.join(",")
-                );
                 if let Err(e) = config.write_server_config() {
                     return Err(MsalError::GeneralFailure(format!(
                         "Failed to write domain join configuration: {:?}",
@@ -1528,7 +1490,7 @@ impl HimmelblauProvider {
                 Ok(())
             }
             Err(e) => Err(e),
-        };
+        }
     }
 
     async fn is_domain_joined<D: KeyStoreTxn + Send>(&self, keystore: &mut D) -> bool {

src/config/himmelblau.conf.example

@@ -5,10 +5,11 @@
 # this would be the primary user of the device.
 # domains =
 #
-# pam_allow_groups MUST be defined or all users will be rejected by pam account.
-# The option should be set to a comma seperated list of Users and Groups which
-# are allowed access to the system. The first user to logon (the device owner)
-# to a configured domain will be added to this list automatically.
+# REQUIRED: pam_allow_groups MUST be defined or all users will be rejected by
+# pam account. The option should be set to a comma seperated list of Users and
+# Groups which are allowed access to the system. Groups MUST be specified by
+# Object ID, not by UPN. This is because Azure does not permit regular users
+# the right to read group names, only the Object IDs which they belong to.
 # pam_allow_groups =
 #
 ### Optional global values

src/graph/src/user.rs

@@ -2,7 +2,7 @@ use anyhow::{anyhow, Result};
 use reqwest::header;
 use serde::Deserialize;
 use serde_json::{json, to_string_pretty};
-use tracing::debug;
+use tracing::{debug, error};
 
 #[derive(Debug, Deserialize)]
 pub struct DirectoryObject {
@@ -59,7 +59,12 @@ pub async fn request_user_groups(
         }
         Ok(res)
     } else {
-        Err(anyhow!(resp.status()))
+        let status = resp.status();
+        error!(
+            "Error encountered while fetching user groups: {}",
+            resp.text().await.map_err(|_| { anyhow!(status) })?
+        );
+        Err(anyhow!(status))
     }
 }