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: Use id from https://graph.microsoft.com/v1.0/me as oidc subject. #2153

Closed
wants to merge 3 commits into from

Conversation

splaunov
Copy link
Contributor

Use id from https://graph.microsoft.com/v1.0/me as a subject identifier instead of userinfo sub field.

Related issue(s)

#2150

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 green light (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

@splaunov
Copy link
Contributor Author

splaunov commented Jan 17, 2022

Please give your feedback.
Not tested these changes yet.

Copy link
Member

@aeneasr aeneasr 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! I've got a question

return nil, errors.WithStack(herodot.ErrInternalServerError.WithReasonf("%s", err))
}

claims.Subject = user.Id
Copy link
Member

Choose a reason for hiding this comment

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

Is the value from the me endpoint different than the one from the userinfo endpoint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, they are different. Have checked this with my own account.

Copy link
Member

Choose a reason for hiding this comment

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

Could you share what their formats look like? And is there no way to get this other ID from the userinfo endpoint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using an access token with scope [User.Read openid profile email] I receive

  • from /userinfo

{
"sub": "AAAAAAAAAAAAAAAAAAAAAENMusLqo_f5vKwPzixWdNS",
"email": "[email protected]",
"picture": "https://graph.microsoft.com/v1.0/me/photo/$value"
}

  • from /me

{
"@odata.context": "https://graph.microsoft.com/v1.0/$metadata#users/$entity",
"displayName": "",
"surname": null,
"givenName": null,
"id": "7wt4391vs271486n",
"userPrincipalName": "[email protected]",
"businessPhones": [],
"jobTitle": null,
"mail": null,
"mobilePhone": null,
"officeLocation": null,
"preferredLanguage": null
}

Copy link
Member

Choose a reason for hiding this comment

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

Thank you, this helps!

Comment on lines +87 to +89
client := o.Client(ctx, exchange)

u, err := url.Parse("https://graph.microsoft.com/v1.0/me")
Copy link
Member

Choose a reason for hiding this comment

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

Note to self: this is not using the resilient client

selfservice/strategy/oidc/provider_microsoft.go Outdated Show resolved Hide resolved
selfservice/strategy/oidc/provider_microsoft.go Outdated Show resolved Hide resolved
@aeneasr
Copy link
Member

aeneasr commented Jan 31, 2022

I tried pushing some changes required for merging the PR to your fork & branch, but it appears that I am not allowed to do so 😕

% git push ...
ERROR: Permission to push denied to aeneasr.
fatal: could not read from the remote repository.

Please make sure that you have the correct access rights
and the repository exists.

But the good news is, giving access is easy! ☺️ All you need to do is enable write access for maintainers. Thank you! 😄

If the repository belongs to an organization, please add me for the project as a collaborator!

@splaunov
Copy link
Contributor Author

splaunov commented Feb 2, 2022

I have invited you as a collaborator to the forked repo.

@aeneasr
Copy link
Member

aeneasr commented Feb 3, 2022

Thank you! Could you please also update the docs section ( https://www.ory.sh/kratos/docs/next/guides/sign-in-with-github-google-facebook-linkedin#microsoft ) and include how and when to use this feature?

@splaunov splaunov force-pushed the microsoft-me-login branch 2 times, most recently from 88456e0 to 0fbd346 Compare March 5, 2022 06:52
@aeneasr
Copy link
Member

aeneasr commented Mar 23, 2022

Ping @splaunov could you please add a section in https://www.ory.sh/docs/kratos/guides/sign-in-with-github-google-facebook-linkedin#microsoft documenting the use of this feature? :)

@splaunov
Copy link
Contributor Author

Yes, sorry for delay. Will do it end of week.

@aeneasr
Copy link
Member

aeneasr commented Mar 23, 2022

great, thank you for the update!

@splaunov
Copy link
Contributor Author

Docs PR: ory/docs#706

@codecov
Copy link

codecov bot commented Mar 25, 2022

Codecov Report

Merging #2153 (a059b6e) into master (4348b86) will decrease coverage by 0.11%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #2153      +/-   ##
==========================================
- Coverage   76.65%   76.54%   -0.12%     
==========================================
  Files         318      318              
  Lines       17272    17297      +25     
==========================================
  Hits        13240    13240              
- Misses       3098     3123      +25     
  Partials      934      934              
Impacted Files Coverage Δ
selfservice/strategy/oidc/provider_config.go 37.20% <ø> (ø)
selfservice/strategy/oidc/provider_microsoft.go 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 794c2fd...a059b6e. Read the comment docs.

@splaunov splaunov requested a review from aeneasr March 28, 2022 07:50
@aeneasr aeneasr force-pushed the microsoft-me-login branch from b1bd18f to a059b6e Compare March 28, 2022 12:33
@aeneasr aeneasr self-assigned this Mar 28, 2022
aeneasr added a commit that referenced this pull request Mar 28, 2022
…crosoft (#2347)

Adds the ability to read the OIDC subject ID from the `https://graph.microsoft.com/v1.0/me` endpoint. This introduces a new field `subject_source` to the OIDC configuration.

Closes #2153

Co-authored-by: splaunov <[email protected]>
harnash pushed a commit to Wikia/kratos that referenced this pull request Mar 28, 2022
…crosoft (ory#2347)

Adds the ability to read the OIDC subject ID from the `https://graph.microsoft.com/v1.0/me` endpoint. This introduces a new field `subject_source` to the OIDC configuration.

Closes ory#2153

Co-authored-by: splaunov <[email protected]>
@mooijtech
Copy link

mooijtech commented Apr 19, 2022

Hello,

I am getting the following error when adding subject_source to my OIDC configuration:

FATA[2022-04-19T12:07:45+02:00] Unable to instantiate configuration.          
audience=application error=map[message:I[#/selfservice/methods/oidc/config/providers/0] S[#/properties/selfservice/properties/methods/properties/oidc/properties/config/properties/providers/items/$ref] doesn't validate with "#/definitions/selfServiceOIDCProvider"
  I[#/selfservice/methods/oidc/config/providers/0] S[#/definitions/selfServiceOIDCProvider/additionalProperties] additionalProperties "subject_source" not allowed] service_name=Ory Kratos service_version=v0.9.0-alpha.3

Maybe the definitions have not been updated correctly?
I will try look into this.

My configuration looks like this:

selfservice:
  default_browser_return_url: http://localhost:3000/
  allowed_return_urls:
    - http://localhost:3000/

  methods:
    password:
      enabled: true
    oidc:
      enabled: true
      config:
        providers:
          - id: outlook
            provider: microsoft
            mapper_url: MAPPER_URL
            client_id: CLIENT_ID
            client_secret: CLIENT_SECRET
            scope:
              - User.Read
              - https://graph.microsoft.com/User.Read
            microsoft_tenant: common
            subject_source: me
            issuer_url: https://login.microsoftonline.com/common
            auth_url: https://login.microsoftonline.com/common/oauth2/v2.0/authorize
            token_url: https://login.microsoftonline.com/common/oauth2/v2.0/token
            requested_claims:
              me:
                userPrincipalName:
                  essential: true

EDIT: Looks like this pull request isn't in v0.9.0-alpha.3

@splaunov
Copy link
Contributor Author

EDIT: Looks like this pull request isn't in v0.9.0-alpha.3

Correct. To test it right now you should build from head of master.

peturgeorgievv pushed a commit to senteca/kratos-fork that referenced this pull request Jun 30, 2023
…crosoft (ory#2347)

Adds the ability to read the OIDC subject ID from the `https://graph.microsoft.com/v1.0/me` endpoint. This introduces a new field `subject_source` to the OIDC configuration.

Closes ory#2153

Co-authored-by: splaunov <[email protected]>
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