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

Add Google OAuth2 authentication flow #935

Merged
merged 8 commits into from
Apr 23, 2021
Merged

Conversation

adekbadek
Copy link
Member

All Submissions:

Changes proposed in this Pull Request:

Adds an OAuth2 authentication flow, as a preparation for the upcoming GAM integration.
Currently, Site Kit's authorisation is reused to access user's GA data. Site Kit does not support the scopes necessary to access GAM data, so Newspack has to roll its own authorisation for this purpose.

UI: I have made it very lightweight purposefully, since this PR is quite large as it is. The UI should probably live in some kind of settings view. cc @thomasguillot

How to test the changes in this Pull Request:

  1. Nothing should change unless the necessary environment variables are present, so have a look at the Dashboard, Campaigns Analytics, and Analytics wizard - everything should work as before
  2. Create a Google OAuth2 app. Set <test-site->/wp-admin/admin.php?page=newspack (e.g. https://newspack.test/wp-admin/admin.php?page=newspack) as an Authorized redirect URI.
  3. Set client ID and secret of the app as NEWSPACK_GOOGLE_OAUTH_CLIENT_ID and NEWSPACK_GOOGLE_OAUTH_CLIENT_SECRET environment variables in wp-config.php
  4. Visit the Dashboard, observe a new section above the wizard links - with an "Authorise" button
  5. Click the button and complete the authorisation flow - observe a success message and information "Authorized Google as " in the end
  6. This was the happy path, now let's test how it can break. Remove the _newspack_google_oauth user meta from the DB
  7. Reload the Dasboard, observe initial (pre-authorisation) state. Complete the authorisation from again.
  8. Visit your Google account's permissions screen and revoke permissions for the app
  9. Again, observe the pre-authorisation state and complete the flow again.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@thomasguillot
Copy link
Contributor

@adekbadek I moved the OAuth2 section to the bottom of the dashboard.

Screenshot 2021-04-16 at 16 31 44

Not quite sure but goToAuthPage didn't seem to work... not quite sure why 🤔

UI: I have made it very lightweight purposefully, since this PR is quite large as it is. The UI should probably live in some kind of settings view

Do you mean the actual connection, or the client ID/secret?

@adekbadek
Copy link
Member Author

UI: I have made it very lightweight purposefully, since this PR is quite large as it is. The UI should probably live in some kind of settings view

Do you mean the actual connection, or the client ID/secret?

The connection - it's better after your changes, but still it's an odd item among links to wizards.

@adekbadek
Copy link
Member Author

Not quite sure but goToAuthPage didn't seem to work... not quite sure why 🤔

What happens after you click on the authorisation dashboard item?

@thomasguillot
Copy link
Contributor

What happens after you click on the authorisation dashboard item?

Absolutely nothing

@thomasguillot
Copy link
Contributor

thomasguillot commented Apr 20, 2021

The connection - it's better after your changes, but still it's an odd item among links to wizards.

Oh ok I see.
What if we replicate something like the integrations step from the onboarding? Where a user could see Jetpack, Site Kit, Mailchimp, and Google OAuth statuses

@adekbadek
Copy link
Member Author

What if we replicate something like the integrations step from the onboarding?

Definitely, but that's on top of the dashboard/post-onboarding UI.

Copy link
Contributor

@dkoo dkoo left a comment

Choose a reason for hiding this comment

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

Really like the consolidation of the Google Services authorizations into a single class, and the DRY-ability(?) it enables. I have a few nits below but nothing blocking, so 👍

@github-actions github-actions bot added [Status] Approved The pull request has been reviewed and is ready to merge and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Apr 22, 2021
Co-authored-by: Derrick Koo <[email protected]>
@adekbadek adekbadek merged commit 98ee47b into master Apr 23, 2021
@adekbadek adekbadek deleted the try/google-oauth2-flow branch April 23, 2021 10:52
matticbot pushed a commit that referenced this pull request Apr 27, 2021
# [1.37.0-alpha.1](v1.36.0...v1.37.0-alpha.1) (2021-04-27)

### Bug Fixes

* **google-oauth:** credentials refreshment ([92c4fce](92c4fce))

### Features

* reorganize segment UI ([#862](#862)) ([bbf2d35](bbf2d35))
* **google:** set up OAuth2 authentication ([#935](#935)) ([98ee47b](98ee47b))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.37.0-alpha.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Apr 28, 2021
# [1.37.0](v1.36.0...v1.37.0) (2021-04-28)

### Bug Fixes

* **google-oauth:** credentials refreshment ([92c4fce](92c4fce))

### Features

* reorganize segment UI ([#862](#862)) ([bbf2d35](bbf2d35))
* **google:** set up OAuth2 authentication ([#935](#935)) ([98ee47b](98ee47b))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.37.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OAuth released on @alpha released [Status] Approved The pull request has been reviewed and is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants