Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 2 years 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 3 years ago.
patch_53658.2.diff (1.4 KB) - added by ashfame 3 years ago.
Correct patch
53658.diff (1.5 KB) - added by johnbillion 3 years ago.
53658.2.diff (1.5 KB) - added by iluy 3 years 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 3 years ago.
53658.4.diff (3.8 KB) - added by felipeelia 3 years ago.
test-53658-before-after-pr2069.png (1.0 MB) - added by hellofromTonya 3 years 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
3 years ago

  • Milestone changed from Awaiting Review to Future Release

@ashfame
3 years ago

#2 @ashfame
3 years ago

  • Keywords has-patch added; needs-patch removed

Docs doesn't have a page for transitioning 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.

Version 0, edited 3 years ago by ashfame (next)

@ashfame
3 years ago

Correct patch

#3 @johnbillion
3 years ago

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

@johnbillion
3 years ago

#4 @johnbillion
3 years 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.


3 years ago

#6 @audrasjb
3 years 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
3 years ago

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

#7 follow-up: @audrasjb
3 years ago

  • Keywords commit added; needs-refresh removed

Thanks @iluy !
Marking this for commit

#8 in reply to: ↑ 7 @iluy
3 years 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
3 years 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
3 years 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
3 years 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.

@david.binda
3 years ago

#12 in reply to: ↑ 11 ; follow-up: @SergeyBiryukov
3 years 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
3 years 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
3 years ago

#14 @felipeelia
3 years 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.


3 years ago
#15

  • Keywords has-unit-tests added

@hellofromTonya
3 years 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
3 years 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
3 years 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.