Implement Application Passwords for API-based authentication. by georgestephanis · Pull Request #540 · WordPress/wordpress-develop

georgestephanis · 2020-09-18T18:59:31Z

This is an alternative to Basic Auth in #169

Based off existing work in https://github.com/WordPress/application-passwords

PR in progress, collaborating with @TimothyBJacobs for the moment. Trying to make sure this is workable to provide an alternative to Basic Auth.

Trac ticket: https://core.trac.wordpress.org/ticket/42790

Also, worth acknowledging all the folks who have contributed to the Application Passwords plugin thus far, especially @kasparsd who has put in a ton of effort.:

https://github.com/WordPress/application-passwords/graphs/contributors

make/core proposal link: https://make.wordpress.org/core/2020/09/23/proposal-rest-api-authentication-application-passwords/

Not 100% sure it works, still need to build and test, but this is a starting point for future commits to get it shipshape. There's also a number of core tweaks in here that need to get spread out to better places within the codebase.

src/wp-includes/class-wp-application-passwords.php

If there is no current user authenticated via other means, clear the cached lack of user, so that an authenticate check can set it properly. This is done because for authentications such as Application Passwords, we don't want it to be accepted unless the current HTTP request is an API request, which can't always be identified early enough in evaluation.

georgestephanis · 2020-09-18T23:08:36Z

Added some changes since the last comment phase -- spread the code out across the admin in more relevant places.

Also moves the list table back outside of the .form-table to fix styling conflicts with the list table.

Use wp.apiRequest instead of raw jQuery.ajax. Use wp.i18n for JS translation instead of translating in PHP. Remove localization for application-passwords.js. Also removes some phpcs comments that core doesn't use.

TimothyBJacobs · 2020-09-19T05:40:23Z

Instead of suggesting a user to edit their .htaccess, we should probably add it to the default config in WP_Rewrite::mod_rewrite_rules as described in https://core.trac.wordpress.org/ticket/39224. Related https://core.trac.wordpress.org/ticket/47077

src/wp-includes/class-wp-application-passwords.php

TimothyBJacobs · 2020-09-19T05:47:43Z

We should also populate rest_authentication_errors with an error message if the app auth fails.

src/wp-includes/rest-api.php

TimothyBJacobs · 2020-09-19T05:52:57Z

https://core.trac.wordpress.org/ticket/43605 will need to land as well.

TimothyBJacobs · 2020-09-19T05:56:26Z

Let's also move test-basic-authorization-header to a Site Health test instead.

src/wp-includes/class-wp-application-passwords.php

This should rarely be an issue now since we include the rewrite in the default htaccess. Once the Site Health REST API endpoints land, the check can be moved there. See #48105.

src/wp-admin/authorize-application.php

TimothyBJacobs · 2020-09-20T04:57:47Z

I think we should require the WordPress site be on SSL. And also verify that both the success and reject URLs are https urls as well.

Ideally, we'd also mandate a state parameter be included. Since the URLs are completely arbitrary, client's could include it themselves, but I think it'd be a good idea to force developers to include it to encourage them to implement CSRF protections on the client side.

georgestephanis · 2020-09-21T07:59:51Z

And also verify that both the success and reject URLs are https urls as well.

Possibly to manually upgrade or display a warning for http urls, but please no for mandating https --

It was explicitly written to account for custom protocols such as WordPress:// so that apps could open a browser tab for the user to authenticate, with a return url that would pass the token directly back into the app upon completion.

georgestephanis · 2020-09-23T20:43:36Z

The wp-admin/authorize-application.php page would look a lot better if it had a look/aesthetic similar to that of the login page... There really is no need to have it in the dashboard itself.

I do kinda wanna make it obvious that the user can just walk away from this process if they choose to and not have it feel like a "squeeze page" that forces a choice. I also hesitate on taking over the entire admin ui unless it's well discussed a conscious decision.

This introduces a new function wp_is_authorize_application_password_request_valid that contains an action to allow for removing this error if desired. This also allows for developers to add additional constraints to the application request such as mandating that the application has been approved by an administrator.

This fixes an issue where the details of a newly added password weren't viewable on mobile. Props dinhtungdu. From WordPress/application-passwords#120.

As a follow-up to WordPress#540 (comment) Co-Authored-By: Ayesh Karunaratne <[email protected]>

Add: confirmation before delete passwords

JS hooks are introduced to customize the data sent to the REST API and perform actions after an app is approved/rejected. Display hooks are added to the authorize-application.php page to add additional form fields or customize the output.

This fixes JS linting issues.

Previously it'd only fire when used in wp_validate_application_password(). This meant the hook wouldn't fire during XML-RPC requests.

Core doesn't apply this level of sanitization.

The Passwords list table can now be extended by plugins. This requires the creation of a "fake" screen id of `application-passwords-user`. Otherwise, the list table uses the user screen id which isn't unique to the app passwords instance. The JS templating is now also moved to the table so plugins can now add their own templates for their columns. I've also moved the data formatting into the underscore template so we can pass the raw response to the template.

This allows for plugins to support things like read-only support, scoped capabilities, expiration, etc...

This can be used for implementing read-only passwords for instance.

The determine_current_user hook doesn't support a WP_Error return type, instead the input should be passed through.

Rearranges the App Passwords class to follow CRUD. Use static:: to allow for subclassing.

…flow. This should make it more clear what site you are logging into and that you need to login to continue the process. https://make.wordpress.org/core/2020/09/23/proposal-rest-api-authentication-application-passwords/#comment-40010

Switches the user-edit an inline role="alert" notice. The modal behavior we were using was not accessible and can be a jarring experience. Implements speak calls after passwords are revoked. Switches auth-app to also use an inline notice.

spacedmonkey · 2020-10-08T20:58:01Z

src/wp-includes/class-wp-application-passwords.php

+	 *
+	 * @type string
+	 */
+	const USERMETA_KEY_APPLICATION_PASSWORDS = '_application_passwords';


Shouldn't an application password be lock to a single site on a multisite? Shouldn't this be prefixed this site id?

Nope. If someone wants to use the WordPress mobile apps, for example, it would suck for them to log into each individual site in their multisite install individually.

Just as interactive login sessions aren't locked to a single site when in a multisite environment, api authentication is also supported across the install.

TimothyBJacobs · 2020-10-08T22:23:52Z

Merged in 1856d0f.

soatok · 2020-10-10T06:56:28Z

src/wp-includes/user.php

+	$hashed_passwords = WP_Application_Passwords::get_user_application_passwords( $user->ID );
+
+	foreach ( $hashed_passwords as $key => $item ) {
+		if ( ! wp_check_password( $password, $item['password'], $user->ID ) ) {


There might be a more efficient way to do this, but it'd require you to adopt a split tokens methodology.

Con: Your generated passwords will necessarily be longer as a result.

Pro: You don't need to check multiple password hashes on API authentication.

The way the code is written, it could be a little slow to handle requests for users with a lot of application passwords.

How to make the change:

Make the generated passwords longer. (e.g. 24 -> 32 chars)

The first 8 characters are now used as a separate identifier for this database lookup, and are included in the password hash calculation but are stored alongside it as plaintext. (This is provably as secure as 24 character passwords, but you get to side-step this performance pain.)

soatok · 2020-10-10T07:06:54Z

src/wp-login.php

 				$errors->add( 'updated', __( '<strong>You have successfully updated WordPress!</strong> Please log back in to see what&#8217;s new.' ), 'message' );
 			} elseif ( WP_Recovery_Mode_Link_Service::LOGIN_ACTION_ENTERED === $action ) {
 				$errors->add( 'enter_recovery_mode', __( 'Recovery Mode Initialized. Please log in to continue.' ), 'message' );
+			} elseif ( isset( $_GET['redirect_to'] ) && false !== strpos( $_GET['redirect_to'], 'wp-admin/authorize-application.php' ) ) {


Accessing superglobals makes me feel less than fuzzy, but none of the subsequent uses are dangerous, so I think this is fine.

soatok · 2020-10-10T07:11:54Z

src/wp-includes/rest-api/endpoints/class-wp-rest-application-passwords-controller.php

+				__( 'Sorry, you are not allowed to manage application passwords for this user.' ),
+				array( 'status' => rest_authorization_required_code() )
+			);
+		}


Is this a satisfactory permission check? Does it make sense that edit_user implies manage application passwords, or should that be a separate permission bit?

georgestephanis commented Sep 18, 2020

View reviewed changes

georgestephanis added 3 commits September 18, 2020 18:09

Populate user application passwords fieldswhere they go.

33102a0

Abstract out the "Authorize Application Password" page.

b5ee86c

TimothyBJacobs added 5 commits September 18, 2020 21:26

Coding standards fixes, add JS to build tools.

2268d73

Also moves the list table back outside of the .form-table to fix styling conflicts with the list table.

Refactor REST API routes to follow core patterns.

46a6724

Use wp.apiRequest instead of raw jQuery.ajax. Use wp.i18n for JS translation instead of translating in PHP. Remove localization for application-passwords.js. Also removes some phpcs comments that core doesn't use.

Remove escaping from translation functions when not necessary.

2965d1e

Add app passwords to the expected routes list.

b815537

Fix linting issues

38c2ba3

TimothyBJacobs reviewed Sep 19, 2020

View reviewed changes

src/wp-includes/class-wp-application-passwords.php Outdated Show resolved Hide resolved

TimothyBJacobs reviewed Sep 19, 2020

View reviewed changes

src/wp-includes/class-wp-application-passwords.php Outdated Show resolved Hide resolved

TimothyBJacobs reviewed Sep 19, 2020

View reviewed changes

src/wp-includes/rest-api.php Outdated Show resolved Hide resolved

TimothyBJacobs reviewed Sep 19, 2020

View reviewed changes

src/wp-includes/class-wp-application-passwords.php Outdated Show resolved Hide resolved

TimothyBJacobs added 5 commits September 19, 2020 14:30

Add rewrite rule to default htaccess generation.

d45d946

Remove basic auth test endpoint.

42693f0

This should rarely be an issue now since we include the rewrite in the default htaccess. Once the Site Health REST API endpoints land, the check can be moved there. See #48105.

Move auth methods to standalone functions.

283dd69

Provide an error message when App Password auth fails.

96e685c

Move entity logic into WP_Application_Passwords.

ac315a9

TimothyBJacobs reviewed Sep 20, 2020

View reviewed changes

src/wp-admin/authorize-application.php Outdated Show resolved Hide resolved

TimothyBJacobs added 3 commits September 20, 2020 01:02

Fix the New Password dialog box being semi-transparent.

f8f0221

Misc clean up.

2a12a61

Add additional fields to the prepared response data.

37235d3

TimothyBJacobs added 2 commits September 26, 2020 16:59

Add expand row button to the JS template.

8717102

This fixes an issue where the details of a newly added password weren't viewable on mobile. Props dinhtungdu. From WordPress/application-passwords#120.

TimothyBJacobs marked this pull request as ready for review September 26, 2020 21:06

TimothyBJacobs and others added 21 commits September 26, 2020 17:23

Fix JS replacement of the password.

3c5843f

Revise the language to be a bit more clear.

9a9ff73

As a follow-up to WordPress#540 (comment) Co-Authored-By: Ayesh Karunaratne <[email protected]>

Add: confirmation before delete passwords

8a49158

refactor: use wp.i18n instead of localize_script

6d038c5

Merge pull request #1 from dinhtungdu/test/application-passwords-119

c4406ac

Add: confirmation before delete passwords

Explicitly use window.confirm for showing the confirmation message.

fc55518

This fixes JS linting issues.

Make sure the failed authentication hook consistently fires.

8ccc6cf

Previously it'd only fire when used in wp_validate_application_password(). This meant the hook wouldn't fire during XML-RPC requests.

Remove strip tags from column headers.

498349c

Core doesn't apply this level of sanitization.

Add hooks to app password creation process.

ad92081

This allows for plugins to support things like read-only support, scoped capabilities, expiration, etc...

Add hook to allow for plugins to constraint password usage.

4173b80

This can be used for implementing read-only passwords for instance.

Don't return an error from wp_validate_application_password.

39edb92

The determine_current_user hook doesn't support a WP_Error return type, instead the input should be passed through.

Add tests covering wp_authenticate_application_password()

c018b6d

Add ticket annotations for REST API tests.

1a1fee1

Some more tests

23e097b

Misc cleanup.

b64d88b

Revert unintended whitespace change

a300a08

More docblock and misc cleanup.

3ab2012

Rearranges the App Passwords class to follow CRUD. Use static:: to allow for subclassing.

Accessibility improvements.

798b684

Switches the user-edit an inline role="alert" notice. The modal behavior we were using was not accessible and can be a jarring experience. Implements speak calls after passwords are revoked. Switches auth-app to also use an inline notice.

spacedmonkey reviewed Oct 8, 2020

View reviewed changes

TimothyBJacobs closed this Oct 8, 2020

soatok reviewed Oct 10, 2020

View reviewed changes

JJJ mentioned this pull request Nov 12, 2020

Add "Application Passwords" section for WordPress 5.6 support stuttter/wp-user-profiles#48

Closed

Conversation

georgestephanis commented Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

georgestephanis commented Sep 18, 2020

Uh oh!

TimothyBJacobs commented Sep 19, 2020

Uh oh!

Uh oh!

Uh oh!

TimothyBJacobs commented Sep 19, 2020

Uh oh!

Uh oh!

TimothyBJacobs commented Sep 19, 2020

Uh oh!

TimothyBJacobs commented Sep 19, 2020

Uh oh!

Uh oh!

Uh oh!

TimothyBJacobs commented Sep 20, 2020

Uh oh!

georgestephanis commented Sep 21, 2020

Uh oh!

georgestephanis commented Sep 23, 2020

Uh oh!

spacedmonkey Oct 8, 2020

Choose a reason for hiding this comment

Uh oh!

georgestephanis Oct 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimothyBJacobs commented Oct 8, 2020

Uh oh!

soatok Oct 10, 2020

Choose a reason for hiding this comment

Uh oh!

soatok Oct 10, 2020

Choose a reason for hiding this comment

Uh oh!

soatok Oct 10, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

georgestephanis commented Sep 18, 2020 •

edited

Loading

georgestephanis Oct 8, 2020 •

edited

Loading