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

feat: extend Microsoft Graph API capabilities #3609

Merged
merged 5 commits into from
Jan 23, 2024

Conversation

moose115
Copy link
Contributor

@moose115 moose115 commented Nov 5, 2023

The change allows to pull more data from Graph API if using Microsoft OIDC. Currently it is only using ID field from the response, and this behaviour is not changed when graph_select field is empty (in fact, the response from Graph is reduced to ID only). By introducing graph_select the Microsoft Provider will request given fields, by appending $select parameter to the request. Claims.ID will be populated as usual, but the entire response will be put into Claims.RawClaims. The list of available properties can be found here.

Related issue(s)

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security vulnerability, I
    confirm that I got the approval (please contact
    [email protected]) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further Comments

Little background on this pull request - I am implementing an internal system in my company and opted for Kratos to take care of sessions and all. I found it a little limiting to use standard fields available under /userinfo endpoint and saw that Graph API could be the solution to my problem. By giving it a little more flexibility, I can pull additional data, i.e. department field so X department's managers cannot access Y departments's files, at the same time limiting user management to Azure only.

I could not set my dev environment properly, so I skipped writing the proper tests; similarly I do not want to touch docs before hearing the feedback from the maintainers. It is a rather small improvement as it is also my first real code contribution to open-source, so I would greatly appreciate any help and comments on it! Thanks!

@CLAassistant
Copy link

CLAassistant commented Nov 5, 2023

CLA assistant check
All committers have signed the CLA.

@moose115 moose115 changed the title feat:Extend Microsoft Graph API capabilities feat: Extend Microsoft Graph API capabilities Nov 9, 2023
Copy link

codecov bot commented Nov 19, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (e4908db) 78.31% compared to head (182d730) 78.29%.
Report is 2 commits behind head on master.

Files Patch % Lines
selfservice/strategy/oidc/provider_microsoft.go 75.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3609      +/-   ##
==========================================
- Coverage   78.31%   78.29%   -0.03%     
==========================================
  Files         346      346              
  Lines       23571    23575       +4     
==========================================
- Hits        18460    18458       -2     
- Misses       3717     3720       +3     
- Partials     1394     1397       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@moose115 moose115 changed the title feat: Extend Microsoft Graph API capabilities feat: extend Microsoft Graph API capabilities Nov 19, 2023
@moose115
Copy link
Contributor Author

Included the change in the docs ory/docs#1603

@moose115 moose115 marked this pull request as ready for review November 19, 2023 05:09
@alnr
Copy link
Contributor

alnr commented Nov 20, 2023

Is there a way to make this possible without having to add configuration? Is the set of user attributes fixed at microsoft? In that case, we could always request all of them in put them into RawClaims. Perhaps we can derive which properties we can query for by looking at the access token scopes?

@moose115
Copy link
Contributor Author

To my understanding, Graph API exceeds beyond standard OIDC fields, the scope is literally entire User.Read permission in Azure cloud. It returns basically every attribute a Microsoft user account can have. This SO answer sums it up - each and every field has to be explicitly given (and by default, without $select it will return a fixed set of a few most common fields).

Microsoft Provider already makes an exception to the config by introducing subject_source in its configuration.

With my fork I managed to successfully get users' departments and use them with Keto for resource access, so that is one niche use achieved.

Answering the questions:

Is there a way to make this possible without having to add configuration?

Yes and no. You can hardcode the URL with every property like so:

"https://graph.microsoft.com/v1.0/me?$select=aboutMe,accountEnabled,ageGroup,assignedLicenses,assignedPlans,birthday,businessPhones,city,companyName,consentProvidedForMinor,country,createdDateTime,creationType,customSecurityAttributes,deletedDateTime,department,displayName,employeeHireDate,employeeLeaveDateTime,employeeId,employeeOrgData,employeeType,externalUserState,externalUserStateChangeDateTime,faxNumber,givenName,hireDate,id,identities,imAddresses,interests,isResourceAccount,jobTitle,lastPasswordChangeDateTime,legalAgeGroupClassification,licenseAssignmentStates,mail,mailboxSettings,mailNickname,mobilePhone,mySite,officeLocation,onPremisesDistinguishedName,onPremisesDomainName,onPremisesExtensionAttributes,onPremisesImmutableId,onPremisesLastSyncDateTime,onPremisesProvisioningErrors,onPremisesSamAccountName,onPremisesSecurityIdentifier,onPremisesSyncEnabled,onPremisesUserPrincipalName,otherMails,passwordPolicies,passwordProfile,pastProjects,postalCode,preferredDataLocation,preferredLanguage,preferredName,provisionedPlans,proxyAddresses,refreshTokensValidFromDateTime,responsibilities,schools,securityIdentifier,showInAddressList,signInActivity,signInSessionsValidFromDateTime,skills,state,streetAddress,surname,usageLocation,userPrincipalName,userType"

Major caveat - some fields require other permissions like User-LifeCycleInfo.Read.All which would require each user to have redundant permissions just to call this many fields.

Is the set of user attributes fixed at microsoft?

When you do not specify $select you get only a few fields and a note saying:

{
  "@microsoft.graph.tips": "This request only returns a subset of the resource's properties. Your app will need to use $select to return non-default properties. To find out what other properties are available for this resource see https://learn.microsoft.com/graph/api/resources/user"
}

@alnr
Copy link
Contributor

alnr commented Nov 24, 2023

I'd like to get to a solution where we don't have to have the new graph_select configuration option.
Instead, we'd query all the fields Microsoft documents here for which we don't need additional permissions.
Hardcoding a long list is fine.
Mapping them to Kratos traits would then happen as usual in jsonnet. If you don't need the fields, you just ignore them. That would also be backwards compatible.

This would require verification:

  1. Which fields we can query via $select without additional permissions.
  2. If we query for a parameter which is not set for the user, that is handled gracefully by Microsoft. Meaning we don't get an error back.

Finally, what I meant by "Is the set of user attributes fixed at microsoft?" is this: is it possible to configure additional attributes queryable via $select in the Azure tenant configuration? Because that would then potentially be a reason to actually have the graph_select Kratos configuration.

@moose115
Copy link
Contributor Author

I can push a commit with a query for all fields available under User.Read permission only.

As for the fields being fixed, then yes it is fixed for what I found. The set of fields from that docs page is fixed and I have not found any way to extend or otherwise customize it.

For error handling, /me endpoint most often returns 200 with queried data (null if empty), ignoring redundant fields and parameters. The only issue I found is 403 when requested fields are the outside permitted scope.

@alnr
Copy link
Contributor

alnr commented Nov 26, 2023

I can push a commit with a query for all fields available under User.Read permission only.

As for the fields being fixed, then yes it is fixed for what I found. The set of fields from that docs page is fixed and I have not found any way to extend or otherwise customize it.

For error handling, /me endpoint most often returns 200 with queried data (null if empty), ignoring redundant fields and parameters. The only issue I found is 403 when requested fields are the outside permitted scope.

Sounds perfect! Does this also solve the problem you initially encoutered?

@moose115
Copy link
Contributor Author

Yep! That's still many more identity fields than with standard traits alone, and like I said with Keto I can change someone's permissions by simply editing their role/department in Azure itself, no need for me to care about building UI and server to toggle people's roles.

@alnr
Copy link
Contributor

alnr commented Nov 27, 2023

Nice. Please add an explanatory comment and a link to https://learn.microsoft.com/en-us/previous-versions/azure/ad/graph/api/entity-and-complex-type-reference#user-entity when you get around to updating the PR.

Grazie!

Copy link
Contributor

@alnr alnr left a comment

Choose a reason for hiding this comment

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

Thanks! You'll have to rebase your changes please.

selfservice/strategy/oidc/provider_microsoft.go Outdated Show resolved Hide resolved
@moose115
Copy link
Contributor Author

moose115 commented Dec 7, 2023

I think I may have messed this PR with rebasing

@moose115 moose115 force-pushed the extend-graph-microsoft branch from 0ba8560 to b605170 Compare December 8, 2023 02:13
@moose115 moose115 force-pushed the extend-graph-microsoft branch from 874d9c5 to 182d730 Compare January 22, 2024 18:48
@moose115
Copy link
Contributor Author

Finally got around to tidying up the mess I made while rebasing. Hope it is ready to merge now

@moose115 moose115 requested a review from alnr January 22, 2024 19:02
Copy link
Contributor

@alnr alnr left a comment

Choose a reason for hiding this comment

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

Thank you!

@alnr alnr merged commit 4a7bcc9 into ory:master Jan 23, 2024
27 of 28 checks passed
alnr added a commit that referenced this pull request Jan 25, 2024
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.

3 participants