Skip to content

Commit

Permalink
Comestic updates as per review
Browse files Browse the repository at this point in the history
  • Loading branch information
Ryan Welcher committed Feb 14, 2018
1 parent 308348b commit 9bec7f1
Showing 1 changed file with 4 additions and 5 deletions.
9 changes: 4 additions & 5 deletions aad-sso-wordpress.php
Original file line number Diff line number Diff line change
Expand Up @@ -384,9 +384,9 @@ function get_wp_user_from_aad_user( $jwt, $group_memberships ) {

// 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
// The user was authenticated, but is not a member a role-granting group.

This comment has been minimized.

Copy link
@bradkovach

bradkovach Feb 14, 2018

Contributor

nit: not a member **of** a role-granting group

return new WP_Error(
'user_not_registered',
'user_not_assigned_to_group',
sprintf(
__( 'ERROR: The authenticated user \'%s\' does not have a group assignment for this site.',
'aad-sso-wordpress' ),
Expand Down Expand Up @@ -441,8 +441,7 @@ function get_wp_user_from_aad_user( $jwt, $group_memberships ) {
* Sets a WordPress user's role based on their AAD group memberships
*
* @param WP_User $user
* @param string $aad_user_id The AAD object id of the user
* @param string $aad_tenant_id The AAD directory tenant ID
* @param mixed $group_memberships The response to the checkMemberGroups request.
*
* @return WP_User|WP_Error Return the WP_User with updated roles, or WP_Error if failed.
*/
Expand All @@ -451,7 +450,7 @@ function update_wp_user_roles( $user, $group_memberships ) {
// Check for errors in the group membership check response
if ( isset( $group_memberships->value ) ) {
AADSSO::debug_log( sprintf(
'Out of [%s], user \'%s\' is a member of [%s]',
'Out of [%s], user \'%s\' is a member of [%s]',
implode( ',', $group_ids ), $aad_user_id, implode( ',', $group_memberships->value ) ), 20
);
} elseif ( isset ( $group_memberships->{'odata.error'} ) ) {
Expand Down

4 comments on commit 9bec7f1

@bradkovach
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@psignoret since hooks and WP_Error instances are relatively new to our project, should these be namespaced, ie aadsso_user_not_assigned_to_group?

@psignoret
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They should, but they haven't been up to now (WP_Error was already used for quite some time), so I don't think it's worth keeping this PR pending over that. It can (and should) be addressed separately, for all errors.

@ryanwelcher
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@psignoret I will have another PR incoming that will add some logging to all of the instances where a WP_Error is fired to allow us to leverage the hook adding in #168.

I'd be happy to address the namespacing with that.

@psignoret
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would en great!

Please sign in to comment.