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

fix: check return code of ms graphapi /me request. #2647

Merged
merged 2 commits into from
Sep 12, 2022

Conversation

floriankramer
Copy link
Contributor

@floriankramer floriankramer commented Aug 5, 2022

When using the microsoft oicd provider and tyring to link an account with insufficient scope (e.g. missing User.Read) for the authorization code the request to https://graph.microsoft.com/v1.0/me used to determine the user id fails silently. This request is used when building the Context to get the user id, which is used as a unique identifier for the credentials.

The result is that the identifier in the identity_crendential_identifiers table is microsoft:. As soon as a second user tries to link their account ory attempts to create a secondcredential identifier with the same identifier of microsoft:. The linking then fails, and the user logs in as the first user who linked with microsoft if they use microsoft oicd.

This pr adds a check to make sure the status code returned from the graph api is 200. There will be a separate pr from a colleague that updates the documentation to make it more clear which scopes are needed for microsoft oicd.

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 green light (please contact security@ory.sh) 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

@CLAassistant
Copy link

CLAassistant commented Aug 5, 2022

CLA assistant check
All committers have signed the CLA.

@floriankramer floriankramer changed the title fix: Check return code of ms graphapi /me request. fix: check return code of ms graphapi /me request. Aug 8, 2022
@floriankramer
Copy link
Contributor Author

I haven't touched anything that should trigger the linter errors (and am getting very different errors locally in code that I've also not changed), so it seems like they are caused by upstream code.

@jonas-jonas
Copy link
Member

The linter errors came from a premature update of the gofmt binary and our use of Open API. I updated the PR, with our state in master. Should be fixed now. :)

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.

Awesome, thank you! 🎉 Your contribution makes Ory better :)

@aeneasr aeneasr merged commit 3f490a3 into ory:master Sep 12, 2022
@floriankramer floriankramer deleted the fix_ms_graph_403 branch September 12, 2022 09:55
@vinckr
Copy link
Member

vinckr commented Sep 14, 2022

Hello @floriankramer
Congrats on merging your first PR in Ory 🎉 !
Your contribution will soon be helping secure millions of identities around the globe 🌏.
As a small token of appreciation we send all our first time contributors a gift package to welcome them to the community.
Please drop me an email and I will forward you the form to claim your Ory swag!

peturgeorgievv pushed a commit to senteca/kratos-fork that referenced this pull request Jun 30, 2023
Co-authored-by: Jonas Hungershausen <jonas.hungershausen@ory.sh>
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.

None yet

5 participants