aad-sso-wordpress
aad-sso-wordpress copied to clipboard
psignoret
Use email properties for email when auto-provisioning
Can you explain this one? I assume it's related to the line:
'user_email' => $jwt->upn, // Hopefully this stays the email!
But I don't see any other fields in that jwt response which would be email-specific.
The AAD user's UserPrincipalName ($jwt->upn) is not a real email address. It has the format <user>@<domain>, but there is no guarantee that there is a mailbox behind it. And if there is, there is no guarantee that the mailbox belongs to the same user (though there is a guarantee that the mailbox belongs to the same organization).
A better alternative is to make a call to the AAD Graph API to get more details on the user, and choose a better attribute for the user_email property of the new WP user. (That's why there's AADSSO_GraphHelper::getMe() and AADSSO_GraphHelper::getUser($id).)
The next challenge is choosing the right attribute, and there isn't a magic answer since AAD has multiple attributes that might hold an email address: otherMails, mail, proxyAddresses, and userPrincipalName. Which one to use depends on how the organization is set up. For instance, for an organization using Exchange Online, the best strategy would be to use the primary proxy address out of proxyAddresses. For an organization that doesn't have Office 365 but uses AAD Sync to populate the directory, they may prefer the mail property. Or for an organization that manages their directory purely in AAD, they might use otherMails.
I think a good implementation would be a configurable, ordered list. E.g. ['primary_proxy_address', 'mail', 'userPrincipalName'], would first try the primary proxy address, and if that's not present, fall back to mail attribute, and it that's not present, fall back to the UPN.
Ok, this is good to go with https://github.com/WebDevStudios/aad-sso-wordpress/commit/145825a2555d82deac269b9ca1281421c0221aa2 and https://github.com/WebDevStudios/aad-sso-wordpress/commit/a5ca14b0b22375883519f1403de9377a913b9378. The ordered list is a filter so it can be overridden if necessary.
That's pretty close, but does not work for proxyAddress values. I added some comments to the code: https://github.com/WebDevStudios/aad-sso-wordpress/commit/a5ca14b0b22375883519f1403de9377a913b9378#diff-a0ada58cb496292a73dad99a8465f94fR275
Any updates here? An enhancement here would be great as external users added to a AAD can have an invalid email address (e.g., #live.com#[email protected]). This makes it necessary to add all users to the WordPress site manually rather than doing auto provisioning, or fixing the email addresses after the auto provisioning.
For my purposes, I stripped out the '#' from the variable set to get_wp_user_from_aad_user(). It looks like jwt->upn is blank for external users, so the code uses jwt->unique_name instead. This is where '#' comes in. Once I stripped that out, the email address and username look like real email addresses and it seems to work well.
The call to wp_insert_user() had failed and there was no error handling for it. I added that too.