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: facebook connect bug #1689

Merged
merged 1 commit into from
Aug 27, 2021
Merged

Conversation

abador
Copy link
Contributor

@abador abador commented Aug 26, 2021

After updating to version 0.7 Facebook Connect stopped working and we can find errors in the logs:

Unable to initialize OpenID Connect Provider: oidc: issuer did not match the issuer returned by provider, expected \"https://facebook.com\" got \"https://www.facebook.com\"

It's a minimal change that adds www to the issuer url

Related issue(s)

#1687

#1686

Checklist

Further Comments

@CLAassistant
Copy link

CLAassistant commented Aug 26, 2021

CLA assistant check
All committers have signed the CLA.

@abador abador changed the title Fix Facebook connect bug, issue #1687 Fix Facebook connect bug Aug 26, 2021
@abador abador changed the title Fix Facebook connect bug fix: facebook connect bug Aug 26, 2021
@abador
Copy link
Contributor Author

abador commented Aug 26, 2021

@aeneasr what's your opinion on adding tests to all provider implementations? It seems that none of them have any tests, except for the generic one. Was that intentional? Not sure if adding tests only to the Facebook provider is the way to go. Maybe this should be done separately?

@aeneasr
Copy link
Member

aeneasr commented Aug 26, 2021

I would love to have tests for those - the problem is that most social sign in providers detect CIs and automation and require solving CAPTCHAs for login. So unless we mock the payloads (which would not have caught this particular bug) I don't see a lot of possibilities here. Or is there some framework or something we could use?

@aeneasr
Copy link
Member

aeneasr commented Aug 26, 2021

Thank you, this looks great! It looks like the CLA bot is not properly detecting your signature. To fix this, try the following:

$ git commit  --amend --author="Author Name <email@address.com>"

Ensure that Author Name is replaced with your GitHub username (e.g. aeneasr) and that the email address is replaced with the email address you have set up in GitHub (e.g. 3372410+aeneasr@users.noreply.github.com):

$ git commit  --amend --author="aeneasr <3372410+aeneasr@users.noreply.github.com>"

Once that is done, you can force-push your changes (make sure to push to the correct remote and branch!):

$ git push --force <remote> HEAD:<branch>

@Amarantine84
Copy link

Guys,
This one seems a bit critical.
Looking forward to get the issue fixed.

@abador abador force-pushed the ISSUE-1687_facebook_connect branch from 6c08858 to 92a5ef7 Compare August 27, 2021 06:16
@abador abador force-pushed the ISSUE-1687_facebook_connect branch from 92a5ef7 to 4f88ae1 Compare August 27, 2021 06:19
@abador
Copy link
Contributor Author

abador commented Aug 27, 2021

Sorry for that @aeneasr it seems that after reinstalling the system I set the wrong git config :)

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.

I'll look into the e2e tests now

@aeneasr
Copy link
Member

aeneasr commented Aug 27, 2021

It should be resolved now, will check again once CI passed

@codecov
Copy link

codecov bot commented Aug 27, 2021

Codecov Report

Merging #1689 (4f88ae1) into master (1ba6c4a) will decrease coverage by 0.11%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1689      +/-   ##
==========================================
- Coverage   74.25%   74.14%   -0.12%     
==========================================
  Files         259      259              
  Lines       12618    12618              
==========================================
- Hits         9370     9356      -14     
- Misses       2627     2638      +11     
- Partials      621      624       +3     
Impacted Files Coverage Δ
selfservice/strategy/oidc/provider_facebook.go 0.00% <0.00%> (ø)
driver/config/config.go 82.00% <100.00%> (ø)
courier/courier.go 57.35% <0.00%> (-10.30%) ⬇️

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 b863a82...4f88ae1. Read the comment docs.

@aeneasr aeneasr merged commit 85337bf into ory:master Aug 27, 2021
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

4 participants