Skip to content

Handle deletion of app password when authenticated with WPCom #16046

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
Merged
merged 10 commits into from
Aug 29, 2025
Merged

Handle deletion of app password when authenticated with WPCom #16046

merged 10 commits into from
Aug 29, 2025

Conversation

@itsmeichigo
Copy link
Contributor

@itsmeichigo itsmeichigo commented Aug 27, 2025 •
edited
Loading

Close WOOMOB-1177

Description

This PR adds the support for cleaning up application password when user is authenticated with WPCom. The idea to delete passwords in two places:

  • Delete password created for the last site upon switching to a different site. In this case, only delete password remotely to avoid deleting the password created for the new site by mistake (race condition).
  • Delete password when logging out. In this case, delete both locally and remotely.

Apologies for the large changes, most of them come from unit tests. Specifically:

  • Updates ApplicationPasswordUseCase to support deleting password remotely only.
  • Moves the trigger to delete password from SessionManager to DefaultStoresManager for separation of concern and easier testing.
  • Updates SessionManager to support delete password when authenticated with WPCom.
  • Updates DefaultStoresManager to trigger delete password remotely only for switching sites (in removeDefaultStore) and locally upon logout (in deauthenticate).
  • Updates app password use cases for testability.
  • Updates ApplicationPasswordNameAndUUIDMapper to support parsing app password from Jetpack-proxied response with data envelope.
  • Adds tests.

Testing steps

WPCom authentication

  • Using at least 2 test stores, revoke all app passwords for your user in wp-admin > Users > Profile.
  • Log in to Store A on the app as the same user using WPCom credentials.
  • Confirm in wp-admin that an application password is created for your user.
  • Switch to Store B. Confirm that:
    • The password created in Store A is deleted.
    • The app works correctly with direct requests to store B (confirmed with Proxyman).
    • A password is created for your user in store B.
  • Repeat the steps with another test store if you have more, or switch back and forth between the stores.
  • Log out of the app. Confirm that the password created for the last store is removed.

Site credentials / app password login

  • Using a self-hosted store, revoke all app passwords for your user in wp-admin > Users > Profile.
  • Log in to the store on the app using site credentials.
  • Check on wp-admin and confirm that a password is created for your user.
  • Log out of the app.
  • Check on wp-admin and confirm that the created password has been removed.

Testing information

  • Confirmed the above steps with 3 test stores.
  • Added unit tests for app password use cases and default stores manager.

Screenshots

No UI changes.

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@itsmeichigo itsmeichigo added this to the 23.2 milestone Aug 27, 2025
@itsmeichigo itsmeichigo added the type: task An internally driven task. label Aug 27, 2025
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Aug 27, 2025 •
edited
Loading

App Icon📲 You can test the changes from this Pull Request in WooCommerce iOS Prototype by scanning the QR code below to install the corresponding build.

App NameWooCommerce iOS Prototype
Build Numberpr16046-f302200
Version23.1
Bundle IDcom.automattic.alpha.woocommerce
Commitf302200
Installation URL55n8r8v7bc460
 Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

Base automatically changed from task/woomob-1123-update-alamofirenetwork to trunk August 28, 2025 02:16
itsmeichigo
itsmeichigo commented Aug 28, 2025
View reviewed changes
WooCommerce/WooCommerceTests/System/SessionManagerTests.swift

/// Verifies that application password is deleted upon calling `deleteApplicationPassword`
///
func test_deleteApplicationPassword_deletes_password_from_keychain() {
Copy link
Contributor Author

@itsmeichigo itsmeichigo Aug 28, 2025

Choose a reason for hiding this comment

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

These tests are removed as they are incorrect. Unit tests should only test immediate functionalities.

@itsmeichigo itsmeichigo marked this pull request as ready for review August 28, 2025 04:04
RafaelKayumov
RafaelKayumov approved these changes Aug 28, 2025
View reviewed changes
Copy link
Contributor

@RafaelKayumov RafaelKayumov left a comment

Choose a reason for hiding this comment

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

LGTM.

WP.com authorized accounts switching produced a new password after login on site B and deleted the password on site A. Store credentials login produced a new application password. Logging out caused the password deletion.

Just left a couple of non-blocking comments.

WooCommerce/Classes/System/SessionManager.swift

/// Keeps strong reference of the use case to keep the password deletion request alive
/// periphery: ignore
var applicationPasswordUseCase: ApplicationPasswordUseCase?
Copy link
Contributor

@RafaelKayumov RafaelKayumov Aug 28, 2025

Choose a reason for hiding this comment

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

NIT: Will the applicationPasswordUseCase instance live until the next deleteApplicationPassword(using creds: Credentials?, locally: Bool) is called? Should we clean that up after async task is completed?

Copy link
Contributor Author

@itsmeichigo itsmeichigo Aug 29, 2025

Choose a reason for hiding this comment

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

Cleanup added in f302200.

WooCommerce/Classes/System/SessionManager.swift
/// Nukes all of the known Session's properties.
///
func reset() {
deleteApplicationPassword()
Copy link
Contributor

@RafaelKayumov RafaelKayumov Aug 28, 2025

Choose a reason for hiding this comment

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

I'm still not fluent with the application passwords states and logic. So the question: what's the reason for removing the password deletion on reset call?

Copy link
Contributor

@RafaelKayumov RafaelKayumov Aug 28, 2025

Choose a reason for hiding this comment

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

Was the deleteApplicationPassword(locally: true) just moved outside of the reset?

Copy link
Contributor Author

@itsmeichigo itsmeichigo Aug 29, 2025

Choose a reason for hiding this comment

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

Yes just moved outside for separation of concern. The method is not called anywhere else other than DefaultStoresManager.

@itsmeichigo itsmeichigo enabled auto-merge August 29, 2025 02:49
@itsmeichigo itsmeichigo merged commit 09438f3 into trunk Aug 29, 2025
14 checks passed
@itsmeichigo itsmeichigo deleted the woomob-1177-handle-removing-application-passwords-for-all-sites-upon branch August 29, 2025 03:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RafaelKayumov RafaelKayumov RafaelKayumov approved these changes

Assignees

No one assigned

Labels

type: task An internally driven task.

Projects

None yet

Milestone

23.2

Development

Successfully merging this pull request may close these issues.

4 participants

@itsmeichigo @wpmobilebot @RafaelKayumov