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.

Sign up for GitHub

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

Jump to bottom

GT-1839, implement facebook auth #1228

Merged
merged 51 commits into from
Mar 30, 2023
Merged

GT-1839, implement facebook auth #1228

merged 51 commits into from
Mar 30, 2023

Conversation

andrewroth
Copy link
Collaborator

No description provided.

@stage-branch-merger
Copy link

I see you added the "On Staging" label, I'll get this merged to the staging branch!

@andrewroth
remove rescue for bad request, it's not an exception but a reponse type
d36f9c5 
(that we handle already by looking at the data)
@frett
Copy link
Contributor

frett commented Mar 9, 2023

I was able to use this to successfully log in to the stage environment

@andrewroth andrewroth force-pushed the GT-1839-support-facebook-logins branch from 509f428 to 9b95d15 Compare March 9, 2023 21:18
@andrewroth andrewroth requested a review from knutsenm March 9, 2023 21:53
frett
frett reviewed Mar 9, 2023
View reviewed changes
app/services/facebook.rb Outdated Show resolved Hide resolved
@frett
Copy link
Contributor

frett commented Mar 13, 2023

it looks like the deletion_request API works correctly for facebook. I was able to initiate the delete from my facebook account and verify the account was deleted from the API.

knutsenm
knutsenm requested changes Mar 15, 2023
View reviewed changes
Copy link
Contributor

@knutsenm knutsenm left a comment

Choose a reason for hiding this comment

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

This looks really good overall. I only have minor comments and questions.

app/services/facebook.rb Outdated Show resolved Hide resolved
app/services/facebook.rb Outdated Show resolved Hide resolved
app/models/deletion_request.rb Outdated Show resolved Hide resolved
app/controllers/deletion_requests_controller.rb Outdated Show resolved Hide resolved
app/models/deletion_request.rb Outdated Show resolved Hide resolved
db/migrate/20230310151243_create_deletion_request.rb Show resolved Hide resolved
app/services/facebook.rb Outdated Show resolved Hide resolved
app/models/deletion_request.rb Outdated Show resolved Hide resolved
app/services/facebook.rb Outdated Show resolved Hide resolved
@frett
Copy link
Contributor

frett commented Mar 27, 2023

The latest version of this PR is failing to deploy to the stage environment, I think it has to do with some of the changing DB migrations. It might take manually fixing the stage database to get this to deploy

frett
frett requested changes Mar 27, 2023
View reviewed changes
Copy link
Contributor

@frett frett left a comment

Choose a reason for hiding this comment

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

Facebook auth isn't working currently, getting a rollbar error when validating an access_token

knutsenm
knutsenm requested changes Mar 28, 2023
View reviewed changes
db/schema.rb Outdated Show resolved Hide resolved
@frett
Copy link
Contributor

frett commented Mar 28, 2023

@levieggertcru confirmed that the Apple login functionality is working on stage

@frett
Copy link
Contributor

frett commented Mar 28, 2023

Current error when logging in with Facebook:

{"errors":[{"source":{"pointer":"/data/attributes/id"},"detail":"Validation failed: Email has already been taken"}]}

We need to remove the unique constraint on user.email because accounts from different login providers could share the same email address.

Also merging users based on email address is problemsome due to: #1228 (comment)

@frett
Copy link
Contributor

frett commented Mar 28, 2023

facebook login is working again. so that should be all 3 of them tested.

From a functional perspective this PR looks good

frett
frett previously approved these changes Mar 28, 2023
View reviewed changes
@frett frett dismissed their stale review March 28, 2023 20:01

I'm good with this PR, I'll let @knutsenm give the final approval

This was referenced Mar 28, 2023
Merged
Merged
@frett
Copy link
Contributor

frett commented Mar 29, 2023

I'm hoping to get this merged by the end of the week

@andrewroth andrewroth mentioned this pull request Mar 29, 2023
Open
knutsenm
knutsenm requested changes Mar 29, 2023
View reviewed changes
app/services/auth_service_base.rb Outdated Show resolved Hide resolved
app/services/auth_service_base.rb Outdated Show resolved Hide resolved
app/services/google_auth_service.rb Outdated Show resolved Hide resolved
app/services/okta_auth_service.rb Outdated Show resolved Hide resolved
app/services/okta_auth_service.rb Outdated Show resolved Hide resolved
knutsenm
knutsenm approved these changes Mar 30, 2023
View reviewed changes
Copy link
Contributor

@knutsenm knutsenm left a comment

Choose a reason for hiding this comment

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

🎉

@frett
Copy link
Contributor

frett commented Mar 30, 2023

@andrewroth this is good to merge, we just need to set the environment variables for production first

@andrewroth
Copy link
Collaborator Author

@frett ok, I'll need a GOOGLE_APP_ID. Typically something like id.apps.googleusercontent.com.

@andrewroth
Copy link
Collaborator Author

I have APPLE_CLIENT_ID=org.cru.godtools also, I believe that will be valid for production..

@andrewroth andrewroth merged commit 72d92c2 into master Mar 30, 2023
@andrewroth andrewroth deleted the GT-1839-support-facebook-logins branch March 30, 2023 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knutsenm knutsenm knutsenm approved these changes

@frett frett frett left review comments

Assignees
No one assigned
Labels
On Staging
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@andrewroth @frett @knutsenm