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.

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

Improve the Azure provider #3

Merged
merged 3 commits into from
May 18, 2020
Merged

Conversation

o1oo11oo
Copy link
Contributor

@o1oo11oo o1oo11oo commented May 17, 2020

This adds or changes the following features:

  • Allow configuring a tenant for controlling which kinds of users can connect and to generate better login urls.
  • Add the default OpenID Connect scopes for Azure for more information in the tokens, like the actual email address.
  • Get the profile picture from Azure, max size can be configured.
  • Optionally get the groups from Azure.

Other than other identity providers, Azure does not provide public urls to the profile pictures, because in many cases they will be private pictures of organization members. Grav only accepts urls to the picture from oauth2 identity providers, therefore this creates and returns a data url with a base64 encoded image. I'm not sure if there's a better way for this, I couldn't find one to properly create a profile image from the provider in an easy way. There is also a problem with this when using the admin plugin, unless a url already includes a question mark, which these do not, it adds a size parameter for the sidebar image, which breaks the base64 data. See getgrav/grav-plugin-admin#1889.

The groups include transitive memberships. The first request to the Microsoft Graph API returns only the guids of the groups, therefore one additional request has to be made for each group to get the name. To make sure this doesn't slow down the login process for users with many group memberships, the requests are run in parallel by using Guzzle promises, because that's the library that the oauth2-client library by league uses. oauth2-client does not provide parallel requests though. It would be better if this logic would be implemented in the lower level libraries, but it works for now.

Important notes about the group syncronization:

  • Since the groups get returned under the group key in the data_user array, which gets merged into the existing user, groups will only be added to a user, but not deleted. Just like the profile pictures I didn't find a good way to also delete them when they're not present anymore, without also changing the login-oauth2 plugin. As a workaround saving users can be turned off.
  • The groups will only be mapped to the user, to give them access rights and make sure they're displayed in the admin plugin they need to be manually created once.

Setting the tenant makes sure the correct login url for single tenant
app registrations gets used. Adding the OpenID Connect scopes allows
access to more claims in the token, like the email claim, which is
better for the email property of the user compared to the upn.
Other than other identity providers, Azure does not provide public urls
to the profile pictures, because in many cases they will be private
pictures of organization members. Grav only accepts urls to the picture
from oauth2 identity providers, therefore this creates and returns a
data url with a base64 encoded image.
The groups include transitive memberships. The first request to the
Microsoft Graph API returns only the guids of the groups, therefore one
additional request has to be made for each group to get the name. To
make sure this doesn't slow down the login process for users with many
group memberships, the requests are run in parallel by using Guzzle
promises, because that's the library that the oauth2-client library by
league uses. oauth2-client does not provide parallel requests though. It
would be better if this logic would be implemented in the lower level
libraries, but it works for now.

Although it's already included as an indirect dependency, this adds
guzzlehttp/promises as a direct dependency, because it's directly used.
@rhukster
Copy link
Member

Cheers

@rhukster rhukster merged commit 1e5240e into trilbymedia:develop May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants