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

Desktop app: Beta release for OAuth login #99742

Merged
merged 13 commits into from
Feb 28, 2025
Merged

Desktop app: Beta release for OAuth login #99742

merged 13 commits into from
Feb 28, 2025

Conversation

psrpinto
Copy link
Member

@psrpinto psrpinto commented Feb 13, 2025

I would recommend reviewing this PR commit-by-commit, each commit should be self-explanatory.

Related to #89736 #72754 #55611

Proposed Changes

In #98345 we introduced a new OAuth login flow for the desktop app, but kept it disabled. This PR enables that flow, and issues a new beta release so that these changes can be tested before releasing to customers. Please see the description of #98345 for details on the changes that are being introduced.

In addition to enabling the OAuth flow, this PR also fixes issues that were found during testing, namely preventing that the user ever sees the regular login page, and instead always sees the desktop login page.

Why are these changes being made?

Please see the description of #98345.

Testing Instructions

  1. Download and install the beta app for the system you're testing on (Mac, Linux, Windows)
    • On macOS, you need to run the following command after installing: xattr -c /Applications/WordPress.com.app
  2. Set your default browser to the one you'd like to test (Please consider testing with a browser that is not yet marked as checked in the list below)
  3. Open the app
  4. If you're logged-in, log out
  5. You should see the new Desktop login page that only displays a "Log in with WordPress.com" button
  6. Click "Log in with WordPress.com"
  7. The browser that you set as default should open
  8. Go through the authentication flow in the browser
  9. At the end, when you click "Authorize" the app should re-open automatically, and should log you in
  10. Close the app and reopen, you should still be logged in.
  11. If successful, check the corresponding entry in the list below
  12. Leave a comment in this PR stating what system/browser you tested, and feel free to mention any feedback you might have
  • Mac
    • Firefox
    • Chrome
    • Safari
  • Windows
    • Edge
    • Firefox
    • Chrome
  • Linux
    • Firefox
    • Chrome/Chromium

Pre-merge Checklist

  • Has the general commit checklist been followed? (PCYsg-hS-p2)
  • Have you written new tests for your changes?
  • Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
  • Have you checked for TypeScript, React or other console errors?
  • Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
  • Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
    • For UI changes, have we tested the change in various languages (for example, ES, PT, FR, or DE)? The length of text and words vary significantly between languages.
  • For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

@psrpinto psrpinto changed the base branch from trunk to fix/always-register-desktop-login-routes February 13, 2025 16:21
@psrpinto psrpinto self-assigned this Feb 13, 2025
Copy link
Collaborator

@wp-desktop wp-desktop left a comment

Choose a reason for hiding this comment

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

WordPress Desktop CI Failure for job "wp-desktop-mac".

@psrpinto please inspect this job's build steps for breaking changes at this link. For temporal failures, you may try to "Rerun Workflow from Failed".

Please also ensure this branch is rebased off latest Calypso.

@matticbot
Copy link
Contributor

matticbot commented Feb 13, 2025

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@matticbot
Copy link
Contributor

matticbot commented Feb 13, 2025

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

  • notifications
  • wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug desktop/oauth-login on your sandbox.

Base automatically changed from fix/always-register-desktop-login-routes to trunk February 14, 2025 10:03
@psrpinto psrpinto force-pushed the desktop/oauth-login branch from 3bbab0b to 95e3287 Compare February 14, 2025 10:18
@psrpinto psrpinto changed the title Desktop app: Enable the new oauth login flow Desktop app: Beta release for OAuth login Feb 14, 2025
@psrpinto psrpinto force-pushed the desktop/oauth-login branch from 95e3287 to 724bc07 Compare February 14, 2025 11:49
@wp-desktop wp-desktop dismissed their stale review February 14, 2025 11:55

wp-desktop ci passing, closing review

@psrpinto psrpinto force-pushed the desktop/oauth-login branch from 5452390 to 2210ab0 Compare February 19, 2025 15:32
@psrpinto psrpinto marked this pull request as ready for review February 19, 2025 16:00
@psrpinto psrpinto force-pushed the desktop/oauth-login branch from 2210ab0 to 62eb38f Compare February 19, 2025 16:52
@psrpinto psrpinto marked this pull request as draft February 19, 2025 17:01
@psrpinto psrpinto force-pushed the desktop/oauth-login branch 4 times, most recently from d26bd2c to 7a7ac3b Compare February 23, 2025 16:54
@psrpinto psrpinto changed the base branch from trunk to fix/desktop-oauth-remember-me February 24, 2025 16:08
@psrpinto psrpinto force-pushed the desktop/oauth-login branch from 7a7ac3b to 0585d89 Compare February 24, 2025 16:08
Base automatically changed from fix/desktop-oauth-remember-me to trunk February 25, 2025 14:29
@psrpinto psrpinto force-pushed the desktop/oauth-login branch from 0585d89 to 3b3fce0 Compare February 25, 2025 14:35
@psrpinto psrpinto marked this pull request as ready for review February 26, 2025 14:33
@psrpinto psrpinto requested a review from tyxla February 26, 2025 14:33
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 26, 2025
@psrpinto
Copy link
Member Author

I've tested this on Mac and Windows, all good. Currently testing on Linux.

@psrpinto
Copy link
Member Author

psrpinto commented Feb 26, 2025

Sadly, this doesn't seem to be working on Linux (debian 12.9). After installing the app from the .deb, I had to create a wpdesktop.desktop file under ~/.local/share/applications with the following content:

[Desktop Entry]
Name=WordPress.com Desktop
Exec=/opt/WordPress.com/wpcom %u
Type=Application
Terminal=false
MimeType=x-scheme-handler/wpdesktop;

Then run:

update-desktop-database
xdg-mime default wpdesktop.desktop x-scheme-handler/wpdesktop

This makes the app open after the user clicks Authorize on their browser, but the app just hangs apparently indefinitely, until the system asks me if I want to force quit it.

For now I will disable the OAuth flow on Linux, so that it uses the regular login flow, so that we can ship to Mac and Windows.

@psrpinto psrpinto requested a review from nsakaimbo February 27, 2025 12:02
@psrpinto
Copy link
Member Author

psrpinto commented Feb 27, 2025

@nsakaimbo Added you as reviewer since PCYsg-RR0-p2 mentions doing so.

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

Changes make sense to me 👍

I can't help too much with testing it, I wonder if we can borrow some help or post on the cft p2?

@psrpinto
Copy link
Member Author

psrpinto commented Feb 27, 2025

I can't help too much with testing it, I wonder if we can borrow some help or post on the cft p2?

No worries @tyxla, thanks for reviewing!

I tested on Mac and Windows in multiple browsers and everything appears operational. The idea would be to merge this PR so that a beta version of the app can be released.

Once the beta is released, we can do further testing of this and some dependency updates we have done in other PRs that require using a signed version of the app, and only released versions are signed.

Once we have the beta released, I will request help from cft P2 (thanks for the suggestion!)

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

👍 🚀

@psrpinto psrpinto merged commit b285e3b into trunk Feb 28, 2025
17 checks passed
@psrpinto psrpinto deleted the desktop/oauth-login branch February 28, 2025 10:48
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 28, 2025
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.

4 participants