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: add support for verified Graph API calls for facebook oidc provider #2547

Merged
merged 4 commits into from
Jul 5, 2022

Conversation

sayoun
Copy link
Contributor

@sayoun sayoun commented Jun 27, 2022

Add support to Facebook OIDC provider for verified Graph API calls.

Facebook allow Apps to enable a security flag that requires proof on all API calls as explained in their documentation

Related issue(s)

The current Facebook provider code does not support this flag and the claims retrieval will silently fail with no claims available. When we check the logs returned by the API call made to the Graph API it returns an error of this type

{
  "error": {
    "message": "API calls from the server require an appsecret_proof argument",
    "type": "GraphMethodException",
    "code": 100,
    "fbtrace_id": "AJWp8p6GrKrNml7iA4eIadU"
  }
}

This PR add code to compute the appsecret_proof and use it on the Graph API call made to retrieve claims.

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.

@codecov
Copy link

codecov bot commented Jun 27, 2022

Codecov Report

Merging #2547 (52bffee) into master (2aeb0a2) will increase coverage by 0.28%.
The diff coverage is 100.00%.

❗ Current head 52bffee differs from pull request most recent head 5ba72a4. Consider uploading reports for the commit 5ba72a4 to get more accurate results

@@            Coverage Diff             @@
##           master    #2547      +/-   ##
==========================================
+ Coverage   76.31%   76.59%   +0.28%     
==========================================
  Files         321      321              
  Lines       17954    17960       +6     
==========================================
+ Hits        13701    13756      +55     
+ Misses       3308     3252      -56     
- Partials      945      952       +7     
Impacted Files Coverage Δ
selfservice/strategy/oidc/provider_facebook.go 83.09% <100.00%> (+83.09%) ⬆️
session/test/persistence.go 98.61% <0.00%> (-1.39%) ⬇️
selfservice/strategy/oidc/provider_apple.go 21.12% <0.00%> (+0.29%) ⬆️

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 659cf57...5ba72a4. Read the comment docs.

@sayoun sayoun marked this pull request as ready for review June 27, 2022 21:38
@sayoun
Copy link
Contributor Author

sayoun commented Jun 28, 2022

I'll try to add some tests for facebook provider

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 1ba7c66 into ory:master Jul 5, 2022
@vinckr
Copy link
Member

vinckr commented Jul 7, 2022

Hello @sayoun
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
…der (ory#2547)

Co-authored-by: aeneasr <3372410+aeneasr@users.noreply.github.com>
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

3 participants