Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensuring that an user authenticated in AAD has a corresponding group … #167

Merged
merged 4 commits into from
Feb 14, 2018
Merged

Ensuring that an user authenticated in AAD has a corresponding group … #167

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a29a777
Ensuring that an user authenticated in AAD has a corresponding group …
Feb 7, 2018
a095d52
Ensuring that the plugin is configured to check for groups before lim…
Feb 11, 2018
308348b
Fixing logic
Feb 12, 2018
9bec7f1
Comestic updates as per review
Feb 14, 2018
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
32 changes: 18 additions & 14 deletions aad-sso-wordpress.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,19 +291,30 @@ function authenticate( $user, $username, $password ) {
);
}

// 1. Retrieve the Groups for this user once here so we can pass them around as needed.
// Pass the settings to GraphHelper
AADSSO_GraphHelper::$settings = $this->settings;
AADSSO_GraphHelper::$tenant_id = $jwt->tid;

// 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 );
Copy link
Owner

Choose a reason for hiding this comment

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

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.



// Invoke any configured matching and auto-provisioning strategy and get the user.
$user = $this->get_wp_user_from_aad_user( $jwt );
// 2. Pass the Group Membership to allow us to control when a user is created if auto-provisioning is enabled.
$user = $this->get_wp_user_from_aad_user( $jwt, $group_memberships );

if ( is_a( $user, 'WP_User' ) ) {

// At this point, we have an authorization code, an access token and the user
// exists in WordPress (either because it already existed, or we created it
// on-the-fly). All that's left is to set the roles based on group membership.
// 4. If a user was created or found above, we can pass the groups here to have them assigned normally
if ( true === $this->settings->enable_aad_group_to_wp_role ) {
$user = $this->update_wp_user_roles( $user, $jwt->oid, $jwt->tid );
$user = $this->update_wp_user_roles( $user, $group_memberships );
}
}

} elseif ( isset( $token->error ) ) {

// Unable to get an access token ( although we did get an authorization code )
Expand Down Expand Up @@ -335,7 +346,7 @@ function authenticate( $user, $username, $password ) {
return $user;
}

function get_wp_user_from_aad_user( $jwt ) {
function get_wp_user_from_aad_user( $jwt, $group_memberships ) {

// Try to find an existing user in WP where the upn or unique_name of the current AAD user is
// (depending on config) the 'login' or 'email' field in WordPress
Expand Down Expand Up @@ -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.
Copy link
Owner

Choose a reason for hiding this comment

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

This comment is only true if enable_aad_group_to_wp_role is true.

if ( true === $this->settings->enable_auto_provisioning && ! empty( $group_memberships->value ) ) {

// Setup the minimum required user data
// TODO: Is null better than a random password?
Expand Down Expand Up @@ -420,15 +432,7 @@ function get_wp_user_from_aad_user( $jwt ) {
*
* @return WP_User|WP_Error Return the WP_User with updated roles, or WP_Error if failed.
*/
function update_wp_user_roles( $user, $aad_user_id, $aad_tenant_id ) {

// TODO: Cleaner (but still lazy) initialization of GraphHelper
AADSSO_GraphHelper::$settings = $this->settings;
AADSSO_GraphHelper::$tenant_id = $aad_tenant_id;

// 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 ) {
Copy link
Owner

Choose a reason for hiding this comment

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

Can you please also update the function's DocBlock accordingly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been updated.


// Check for errors in the group membership check response
if ( isset( $group_memberships->value ) ) {
Expand Down