Ensuring that an user authenticated in AAD has a corresponding group … #167
Conversation
…assigned before creating the WP_User
There was a problem hiding this comment.
Thanks for the pull request! It looks like you're assuming that Azure AD group membership to WordPress role mapping will always be enabled, which is not necessarily the case. Could you also take into account the scenario where the plugin is not configured to check for group membership?
aad-sso-wordpress.php
Outdated
|
|
||
| // Of the AAD groups defined in the settings, get only those where the user is a member | ||
| $group_ids = array_keys( $this->settings->aad_group_to_wp_role_map ); | ||
| $group_memberships = AADSSO_GraphHelper::user_check_member_groups( $jwt->oid, $group_ids ); |
There was a problem hiding this comment.
We should only be checking for group membership if group->roles is enabled (the enable_aad_group_to_wp_role setting), for a couple reasons:
- Checking for group membership adds a Graph API request to the sign-in, so we definitely don't want to be checking if there's no reason to.
- Checking for group membership requires permission to read all groups. This is a permission which can only be granted by an administrator, which could limit use of the plugin for those who just want it for simple sign-in (and are not admins). Also, if the plugin hasn't been configured for group->role enforcement, it is unlikely it has been given the right permissions for the Azure AD Graph API.
At minimum, this part should only be run if enable_aad_group_to_wp_role is enabled.
aad-sso-wordpress.php
Outdated
| @@ -364,7 +375,8 @@ function get_wp_user_from_aad_user( $jwt ) { | |||
|
|
|||
| // Since the user was authenticated with AAD, but not found in WordPress, | |||
| // need to decide whether to create a new user in WP on-the-fly, or to stop here. | |||
| if( true === $this->settings->enable_auto_provisioning ) { | |||
| // 3. If there are no groups for this user, we should not be creating it. | |||
There was a problem hiding this comment.
This comment is only true if enable_aad_group_to_wp_role is true.
…iting user creation
|
@psignoret thanks for the feedback! I've made some changes to respect the appropriate settings in regards to groups. Please let me know if I have missed anything. |
|
I just realized my logic was incorrect in the new if() statement. Updated now. |
aad-sso-wordpress.php
Outdated
| if ( true === $this->settings->enable_aad_group_to_wp_role && empty( $group_memberships->value ) ) { | ||
| // The user was authenticated, but not found in WP and auto-provisioning is disabled | ||
| return new WP_Error( | ||
| 'user_not_registered', |
There was a problem hiding this comment.
Can you change this to something unique? (E.g. user_not_assigned_to_group.)
aad-sso-wordpress.php
Outdated
|
|
||
| // 3. If we are configured to check, and there are no groups for this user, we should not be creating it. | ||
| if ( true === $this->settings->enable_aad_group_to_wp_role && empty( $group_memberships->value ) ) { | ||
| // The user was authenticated, but not found in WP and auto-provisioning is disabled |
There was a problem hiding this comment.
Can you please update the comment to reflect the error condition? (The user was authenticated, but is not a member of a role-granting group.)
| // Of the AAD groups defined in the settings, get only those where the user is a member | ||
| $group_ids = array_keys( $this->settings->aad_group_to_wp_role_map ); | ||
| $group_memberships = AADSSO_GraphHelper::user_check_member_groups( $aad_user_id, $group_ids ); | ||
| function update_wp_user_roles( $user, $group_memberships ) { |
There was a problem hiding this comment.
Can you please also update the function's DocBlock accordingly?
There was a problem hiding this comment.
This has been updated.
|
Thanks, @ryanwelcher! |
This PR checks the Groups for the user attempting to access before creating the user. This is an attempt to address #126