Make WordPress Core

Opened 23 months ago

Closed 18 months ago

Last modified 17 months ago

#53658 closed defect (bug) (fixed)

Display a user-facing message when the application passwords functionality is not available

Reported by: johnbillion's profile johnbillion Owned by: johnbillion's profile johnbillion
Milestone: 5.9 Priority: normal
Severity: normal Version: 5.6
Component: Application Passwords Keywords: good-first-bug has-patch has-unit-tests commit
Focuses: administration Cc:

Description

The Application Passwords feature is not available on sites that don't use HTTPS (via is_ssl()). This applies to all environment types except "local" (via wp_get_environment_type()), which allows the feature to be used on non-HTTPS local environments.

When the feature isn't available there's no explanation shown to the user on the user profile editing screen, it simply doesn't appear. There should be an explanation provided with a link to related documentation about setting up HTTPS for production sites or setting the environment type for local environments.

Attachments (7)

patch_53658.diff (1.4 KB) - added by ashfame 22 months ago.
patch_53658.2.diff (1.4 KB) - added by ashfame 22 months ago.
Correct patch
53658.diff (1.5 KB) - added by johnbillion 21 months ago.
53658.2.diff (1.5 KB) - added by iluy 20 months ago.
Created a quick patch that makes the HelpHub link translatable, based off of @audrasjb latest comment. :)
53658.3.diff (1.2 KB) - added by david.binda 18 months ago.
53658.4.diff (3.8 KB) - added by felipeelia 18 months ago.
test-53658-before-after-pr2069.png (1.0 MB) - added by hellofromTonya 18 months ago.
Test Report: before and after applying PR 2069 and with and without the add_filter. Results: before = can reproduce; after = issue resolved

Download all attachments as: .zip

Change History (26)

#1 @desrosj
23 months ago

  • Milestone changed from Awaiting Review to Future Release

#2 @ashfame
22 months ago

  • Keywords has-patch added; needs-patch removed

Docs doesn't have a page for migrating an install to HTTPS, though we have an issue going on for that now - https://github.com/WordPress/HelpHub/issues/333

Meanwhile, in my patch, I have added the link to search page and this can be updated with an actual page once that page is up and ready.

Last edited 22 months ago by ashfame (previous) (diff)

@ashfame
22 months ago

Correct patch

#3 @johnbillion
22 months ago

  • Milestone changed from Future Release to 5.9
  • Owner set to johnbillion
  • Status changed from new to reviewing

@johnbillion
21 months ago

#4 @johnbillion
21 months ago

53658.diff makes some adjustments to the wording and removes the link for HTTPS while we wait for an appropriate page to be produced.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


20 months ago

#6 @audrasjb
20 months ago

  • Keywords needs-refresh added

Hello, the patch still applies against trunk, but the HelpHub link needs to be translatable:

We should replace
'https://wordpress.org/support/article/editing-wp-config-php/#wp_environment_type'
with
__( 'https://wordpress.org/support/article/editing-wp-config-php/#wp_environment_type' )

@iluy
20 months ago

Created a quick patch that makes the HelpHub link translatable, based off of @audrasjb latest comment. :)

#7 follow-up: @audrasjb
20 months ago

  • Keywords commit added; needs-refresh removed

Thanks @iluy !
Marking this for commit

#8 in reply to: ↑ 7 @iluy
20 months ago

Happy to help. Hopefully in the near future, I'll be able to make more and more meaningful contributions ;)

Replying to audrasjb:

Thanks @iluy !
Marking this for commit

#9 @johnjamesjacoby
19 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 51980:

Application Passwords: display guiding text & link in user-edit.php when unavailable.

This change intends to avoid confusion around the requirements of the Application Passwords feature, specific to it requiring HTTPS and the WP_ENVIRONMENT_TYPE constant.

It does this by conditionally hiding the traditional UI and showing some insightful explanations instead, including a translatable link to the WP_ENVIRONMENT_TYPE documentation on the "Editing wp-config.php" support page.

Props ashfame, audrasjb, iluy, johnbillion.

Fixes #53658.

#10 @SergeyBiryukov
19 months ago

In 51988:

Coding Standards: Fix some WPCS errors and warnings in wp-admin/user-edit.php:

  • Add missing translators comment.
  • Add missing space, correct indentation.
  • Put opening and closing PHP tag on a line by itself.
  • Remove unnecessary escaping for consistency with other strings.

Follow-up to [51980].

See #53658.

#11 follow-up: @david.binda
18 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Testing the 5.9 beta version, I have noticed the new message about missing HTTPS support and development environment setup being displayed on a production site with HTTPS support enabled.

It was due to __return_false hooked to wp_is_application_passwords_available filter (but can be reproduced even by hooking wp_is_application_passwords_available_for_user filter).

Seeing a message suggesting that HTTPS is not enabled while accessing wp-admin via https, and suggestions on how to setup my development environment while checking production site, might feel confusing, at least.

In my humble opinion, the new message on missing support for the application passwords should be more finely targeted. Possibly by performing the same checks as wp_is_application_passwords_available is doing ( $available = is_ssl() || 'local' === wp_get_environment_type(); ), just without additional filters.

#12 in reply to: ↑ 11 ; follow-up: @SergeyBiryukov
18 months ago

Replying to david.binda:

Seeing a message suggesting that HTTPS is not enabled while accessing wp-admin via https, and suggestions on how to setup my development environment while checking production site, might feel confusing, at least.

In my humble opinion, the new message on missing support for the application passwords should be more finely targeted. Possibly by performing the same checks as wp_is_application_passwords_available is doing ( $available = is_ssl() || 'local' === wp_get_environment_type(); ), just without additional filters.

Good catch! Seems like we can just use wp_is_application_passwords_available() though, instead of duplicating the checks?

#13 in reply to: ↑ 12 @ocean90
18 months ago

  • Keywords commit removed

Replying to SergeyBiryukov:

Good catch! Seems like we can just use wp_is_application_passwords_available() though, instead of duplicating the checks?

wp_is_application_passwords_available() is not just about HTTPS, that's only the default behaviour. Since it has a filter it can be disabled for whatever reason. So it can't be used to display a message which is only about HTTPS.

Though, we could extract the part for the is_ssl() and environment check into a wp_is_application_passwords_supported() function which has no filter so it can be used in wp_is_application_passwords_available() and to display the HTTPS message.

@felipeelia
18 months ago

#14 @felipeelia
18 months ago

The diff above addresses @ocean90's feedback and adds a PHP unit test.

This ticket was mentioned in PR #2069 on WordPress/wordpress-develop by costdev.


18 months ago
#15

  • Keywords has-unit-tests added

@hellofromTonya
18 months ago

Test Report: before and after applying PR 2069 and with and without the add_filter. Results: before = can reproduce; after = issue resolved

#16 @hellofromTonya
18 months ago

  • Keywords commit added

Test Report

Env:

  • OS: macOS Big Sur
  • Browser: Chrome and Edge
  • Localhost: Local and wp-env
  • Plugins: none
  • Theme: TT2

Steps

  1. Pull latest trunk.
  2. Add a must-use file by creating a folder under wp-content/mu-plugins and then adding wp-content/mu-plugins/test53658.php file.
  3. Add the following code to the test file:
    <?php
    
    add_filter( 'wp_is_application_passwords_available', '__return_false' );
    
  4. Enable HTTPS on your test site.
  5. Navigate to your user profile in Users > Profile.
  6. Scroll down to "Application Passwords" section.

Notice the error message that HTTPS is needed:

The application password feature requires HTTPS, which is not enabled on this site.
  1. Applying PR 2069 patch.
  2. Refresh the page.

Expected behavior: The "Application Passwords" section is gone.

  1. Comment out the add_filter code in the test file.

Expected behavior: The "Application Passwords" section displays and there's no message about needing HTTPS.

Results

  • At Step 6: Able to reproduce the reported issue ✅
  • At Step 7-8: The application passwords section is gone and no error ✅
  • At Step 9: The application passwords section displays properly and no error ✅

PR 2069 resolves the reported issue in comment 11 above.

Marking it for commit.

#17 @hellofromTonya
18 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 52398:

Application Passwords: Show HTTPS required message without filtering when not enabled or not in local environment.

When add_filter( 'wp_is_application_passwords_available', '__return_false' ) exists, HTTPS requirement message is shown even if HTTPS is enabled on the site. This happens because wp_is_application_passwords_available_for_user() first invokes wp_is_application_passwords_available() which is filterable. The situation could happen if the 'wp_is_application_passwords_available_for_user' filter returns false.

To fix this, the check for HTTPS (or if in a 'local' environment) is moved to a new function called wp_is_application_passwords_supported(). Then the return from this function is used as an OR condition for the Application Passwords section and for displaying the HTTPS required message.

Tests are included for both wp_is_application_passwords_supported() and wp_is_application_passwords_available().

Follow-up to [51980], [51988].

Props davidbinda, SergeyBiryukov, ocean90, felipeelia, costdev, hellofromTonya.
Fixes #53658.

Note: See TracTickets for help on using tickets.