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

🪟 🐛 New OAuth connectors should never say "re-authenticate" #18487

Merged
merged 9 commits into from
Nov 4, 2022

Conversation

teallarson
Copy link
Contributor

@teallarson teallarson commented Oct 26, 2022

What

closes #16599

Previously, some OAuth connectors would render a button that said "Re-Authenticate" when a user was creating a new connection. Some of this was resolved with #17965 when the new useAuthentication hook was introduced. This PR resolves the rest of it.

How

Previously, we were looking to see if there was an empty credentials object to determine whether a user had OAuthed yet or not. However, in some cases the keys are present already, but all values are undefined. This PR changes that to check for either no credentials object, or an object with no defined values.

Recommended reading order

The one file

Testing

Previously, you could see this bug by OAuthing a Google connector (we have Google Sheets on frontend-dev) and then trying to create a Google Ads source. This was the only connector I was able to find with the bug remaining after #17965.

Now, if you create a Google Sheets OAuth source, then go to create a Google Ads source, you should see Sign in with Google and not Re-authenticate on the OAuth button.

Choosing an existing connector and going to settings should display "Re-authenticate"

@teallarson teallarson marked this pull request as ready for review October 26, 2022 13:56
@teallarson teallarson requested a review from a team as a code owner October 26, 2022 13:56
@timroes
Copy link
Collaborator

timroes commented Oct 26, 2022

Not introduced by this PR, but I feel I asked this already on the original PR that introduced this: credentials is just one arbitrary path inside values that could hold the information, but they could as well also just sit on the root level? Or any other path defined in the spec. How does the current code handle that? E.g. can we verify this works e.g. with the Instagram or Salesforce connector which have the button on the root of the form?

@teallarson
Copy link
Contributor Author

teallarson commented Oct 26, 2022

E.g. can we verify this works e.g. with the Instagram or Salesforce connector which have the button on the root of the form?

Salesforce never says "re-authenticate" as it stands, due to what you mentioned.

I do wonder if I could pull the auth fields to hide from the new service and use that instead. I'll try it out.

@timroes
Copy link
Collaborator

timroes commented Oct 26, 2022

I think that would be exactly the right solution, @tealjulia. Maybe let's in general move this as a flag into the useAuhentication hook as something like hasAuthFieldValues or such

@github-actions github-actions bot added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Oct 26, 2022
@teallarson
Copy link
Contributor Author

teallarson commented Oct 26, 2022

@timroes I pinged you for review since you had shared some opinions about wanting a more generalizable solution here! This should work for any connector configuration now (and does per my testing it out with salesforce, google ads, google sheets, github, and instagram).

I do wonder if there are branding requirements for the "Re-authneticate" button for Google? Are we required to include the words "google account" there?

@teallarson teallarson changed the title 🪟 🐛 New OAuth connectors never say "re-authenticate" 🪟 🐛 New OAuth connectors should never say "re-authenticate" Oct 27, 2022
Copy link
Contributor

@lmossman lmossman left a comment

Choose a reason for hiding this comment

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

The solution looks good to me!

I tested this out on frontend-dev and was able to reproduce the re-authenticate issue you described in the PR description, and then I spun up this branch on npm and that solved the problem. I still got a 404 when clicking the oauth button on Google Ads, but I think that is expected based on that source not being listed here

Not super sure about your Google branding question. @timroes maybe that is something we can check with the Google team? Not sure what that process looks like

@timroes
Copy link
Collaborator

timroes commented Oct 27, 2022

Regarding the Sign-In branding question. Their branding guidelines are documented here: https://developers.google.com/identity/branding-guidelines

There is no specific rules around the "reauthentication" purpose, so I think we'l be fine with this. We can always change it, if they'd would reach out and see this as a problem (which they tend to do).

…on.tsx

Co-authored-by: Tim Roes <tim@airbyte.io>
Copy link
Collaborator

@timroes timroes left a comment

Choose a reason for hiding this comment

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

Code LGTM, have not tested locally since the suggested refactoring

@teallarson teallarson merged commit 6e94249 into master Nov 4, 2022
@teallarson teallarson deleted the teal/dont-say-reauthenticate-for-new-sources branch November 4, 2022 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend Related to the Airbyte webapp area/platform issues related to the platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Service Form should not say "Re-Authenticate" for new connectors
3 participants