[WIP] Version 0.1 by valendesigns · Pull Request #1 · WP-API/jwt-auth

valendesigns · 2018-11-18T22:18:13Z

kadamwhite

This plugin's a WIP prototype, so I have no formal objection to merging as-is if it'll keep us moving; there's lots to be done and this does provide a decent foundation.

However, there's a couple wishlist things I'd like to see here before pushing the green button:

A little more rigor around documentation, commenting, and style; on the theory that if we start strong, it'll be easier to end strong
A comment thoroughly walking through the flow of requests necessary to generate a token
Some skeleton documentation around the overall structure; as evidenced by the conversation in Slack today several of us are not confidant we can suss out the flows from the code alone, so a talk-through would be very valuable.

kadamwhite · 2019-03-14T19:18:27Z

.eslintrc

+{
+	"env": {
+		"browser": true
+	},


We'll probably want to clean this up in a similar way to WordPress/grunt-patch-wordpress#72 -- consume the core eslint plugin, etc -- but that's a follow-up, not necessary to resolve for this PR.

Pulled this out into #3 so we can table it for now and revisit once the important stuff is done 👌

kadamwhite · 2019-03-14T19:22:18Z

.travis.yml

+
+# Newer versions like trusty don't have PHP 5.2 or 5.3
+# https://blog.travis-ci.com/2017-07-11-trusty-as-default-linux-is-coming
+dist: precise


With the work we're doing to bump minimum version, let's code towards a 7.x future and only test down to 5.6 at minimum.

We can test down to 5.6, which is fine, but we still would support 5.3 through static analysis.

I slightly disagree, 5.3 is long past EOL and the timeline we have for this would put us into a zone where we shouldn't have to worry about it with regard to core development. I don't think it should hold this up but I don't think we need to go all the way back to precise.

kadamwhite · 2019-03-14T19:26:34Z

jwt-auth.php

+
+	if ( version_compare( PHP_VERSION, '5.3', '<' ) ) {
+		deactivate_plugins( plugin_basename( __FILE__ ) );
+		wp_die( esc_html__( 'The JWT Auth plugin requires PHP Version 5.3 or above.', 'jwt-auth' ) );


See above note about 5.6

@kadamwhite Changing this to 5.6 is not needed. The minimum PHP version required for the code to work is 5.3 and Core is adopting 5.3, so what would the benefit of making this 5.6 be?

Per this post: https://wordpress.org/news/2019/04/minimum-php-version-update/ the PHP minimum version for core is, "5.6 now and soon 7+"

jwt-auth.php

kadamwhite · 2019-03-14T19:29:01Z

tests/autoload.php

+$config = getenv( 'WP_TESTS_CONFIG' );
+
+/**
+ * Supports loading the wp-tests-config.php from a non VVV custom directory.


Nitpick: let's remove the VVV-specific language.

At this point the only environment the unit tests work in is VVV, or have been tested in would be more accurate.

@valendesigns Do you mean it can only run inside VVV? Because I was able to run it via Laravel Valet.

kadamwhite · 2019-03-14T19:46:04Z

wp-includes/rest-api/auth/class-wp-rest-key-pair.php

+ *
+ * This class only allows REST authentication to the `{api_prefix}/wp/v2/token`
+ * endpoint with an API key:secret. This allows a user to be identified via the
+ * REST API without using their login credentials to generate a JSON Web Token.


I see what's intended here, but as discussed in Slack I think this flow is confusing and that we'll need a request flow example (Either in js, php, whichever, saying, POST to /wp-json/tokens/generate, then take X from that response, etc…) before we make any progress on that.

kadamwhite · 2019-03-14T19:47:56Z

wp-includes/rest-api/auth/class-wp-rest-key-pair.php

+	 */
+	public function register_routes() {
+		$args = array(
+			array(


I'm not clear why this is an array of arrays. Shouldn't these all just be one level? None of the core endpoints use nested arrays in this way, they pass $args = array( 'args' => array(), 'methods' => ... ) etc.

I second that!

kadamwhite · 2019-03-14T19:49:56Z

wp-includes/rest-api/auth/class-wp-rest-key-pair.php

+				'callback' => array(
+					$this,
+					'delete_key_pair',
+				),


Absolute style nit but we try to keep these callback arrays to one line in the core controller code, to make it more obvious it's a class method lookup.

Second that!

kadamwhite · 2019-03-14T19:52:20Z

wp-includes/rest-api/auth/class-wp-rest-key-pair.php

+		if ( isset( $_POST['pass1'] ) && ! empty( $_POST['pass1'] ) ) { // phpcs:ignore
+			$keypairs = $this->get_user_key_pairs( $user_id );
+			if ( ! empty( $keypairs ) ) {
+				$this->set_user_key_pairs( $user_id, array() );


Could use a clarifying comment, here or in the function docbloc, clearly explaining what's being cleared.

kadamwhite · 2019-03-14T19:57:07Z

wp-includes/rest-api/auth/jwt/JWT.php

@@ -0,0 +1,379 @@
+<?php
+
+namespace Firebase\JWT;


Anybody know the policy on external dependencies like this, in terms of where they should go in the codebase? Most of them go into /wp-includes/[ project ], looking at SimplePie, Requests, etc. I'd expect this should be followed here

I've moved it 👍

jasonbahl · 2019-03-15T18:10:05Z

I see in the checklist: Add support for WPGraphQL

I think right now, as long as there are appropriate filters/hooks/public methods for any API to hook into, then at the moment it should be WPGraphQL's responsibility to provide some "adapter" to work with this.

There's no official plan (that I know of) to move WPGraphQL into core, so a plugin like this that is targeting core should probably not be concerned with explicit WPGraphQL support, but rather general access for any API layer to make use of.

REST and GraphQL aren't the only API's that will exist for WordPress. Whatever cool new API tech 5 years from now should also be able to make use of whatever foundation is there.

I'll try and review the code shortly and provide thoughts on how I would want to hook in and if any changes are necessary to be able to accomplish what I would need to do

valendesigns · 2019-03-17T02:58:10Z

@kadamwhite I'll work on updates from the review on Monday. Thanks for reviewing!

@jasonbahl Yes the intent is to ensure the current hooks/filters and workflow support WPGraphQL not to write specific code only used for it.

renatonascalves

Thoughts on my initial review!

renatonascalves · 2019-03-19T12:15:31Z

tests/autoload.php

+$config = getenv( 'WP_TESTS_CONFIG' );
+
+/**
+ * Supports loading the wp-tests-config.php from a non VVV custom directory.


@valendesigns Do you mean it can only run inside VVV? Because I was able to run it via Laravel Valet.

renatonascalves · 2019-03-19T12:17:41Z

tests/autoload.php

+$config = getenv( 'WP_TESTS_CONFIG' );
+
+/**
+ * Supports loading the wp-tests-config.php from a non VVV custom directory.


renatonascalves · 2019-03-19T12:22:42Z

wp-includes/rest-api/auth/class-wp-rest-key-pair.php

+	 */
+	public function register_routes() {
+		$args = array(
+			array(


I second that!

renatonascalves · 2019-03-19T12:24:12Z

wp-includes/rest-api/auth/class-wp-rest-key-pair.php

+		$args = array(
+			array(
+				'methods'  => WP_REST_Server::CREATABLE,
+				'callback' => array(


Suggestion: leave in one line: 'callback' => array( $this, 'generate_key_pair' ) This and others below.

renatonascalves · 2019-03-19T12:26:01Z

wp-includes/rest-api/auth/class-wp-rest-key-pair.php

+				'callback' => array(
+					$this,
+					'delete_key_pair',
+				),


Second that!

renatonascalves · 2019-03-19T12:28:15Z

wp-includes/rest-api/auth/class-wp-rest-key-pair.php

+	public function get_item_schema() {
+		$schema = array(
+			'$schema'    => 'http://json-schema.org/draft-04/schema#',
+			'title'      => esc_html__( 'Key-pair', 'jwt-auth' ),


Shouldn't this be lower case and with the underscore? key_pair?

renatonascalves · 2019-03-19T12:34:31Z

wp-includes/rest-api/auth/class-wp-rest-key-pair.php

+	 * @param mixed           $user    The user that's being authenticated.
+	 * @param WP_REST_Request $request The REST request object.
+	 *
+	 * @return bool|object|mixed


Suggestion: Add the objects returned: WP_User and WP_Error so that I don't have to read the code/method to know which objects are returned.

Technically if someone has filtered rest_authentication_user it could be any kind of object. If $user is not equal to false the method returns the value so yes the intention is that it will return a WP_User but that's not a guarantee and will work perfectly with any type of object with the correct properties. If someone wanted to build their own decoupled user system that's possible by filtering rest_authentication_user. I'll update the docblock to call this out.

K! Thank you! Makes sense!

renatonascalves · 2019-03-19T12:35:35Z

wp-includes/rest-api/auth/class-wp-rest-key-pair.php

+	 *
+	 * @param object $jwt The JSON Web Token.
+	 *
+	 * @return object|WP_Error


Same here! Which object is that?

Uses the built-in `get_rest_url` API to handle all situations. Return only the path from `get_rest_uri`

kadamwhite · 2019-04-03T20:33:27Z

The key thing we're waiting on here, if you have bandwidth @valendesigns , is this:

A comment thoroughly walking through the flow of requests necessary to generate a token

If we can get a human-language walkthrough of the full auth process, that will help move everything else along.

composer.json

ocean90 · 2019-04-11T12:13:41Z

I'm wondering what the reason was for invalidating a token when an email changes in 7ccfc84?
This now means when a user changes their email via the API you'd have to request them to re-login after the change to get a valid token again. This sounds like a bad UX and would be different from the existing cookie validation since only a user_login change invalidates the cookie.

koke · 2019-04-12T11:32:45Z

I’d extend the same question to user_login. I don’t see why a username change would invalidate a token.

I even have doubts about whether a password change should, not sure if there has been a discussion about that, or just going with what cookie authentication does.

ocean90 · 2019-04-12T11:48:52Z

A password change doesn't invalidate the token currently, which I think is fine.

Since one of the principles of JWT is stateless I'm suggesting to just drop validate_user() altogether. A valid signed and not expired token needs to be trusted. If it's valid let wp_set_current_user( $user_id ); do the rest.

If we follow this we could remove all the user data/type handling to keep it simple. Maybe even use the sub claim for the user ID?

jasonbahl · 2019-04-12T13:44:18Z

I need to get caught up on the codebase and conversations, but I would submit that Username and password changes should for sure invalidate tokens.

If we go the "stateless" approach, we need to issue very short lived tokens (few minute expiration max) and an additional revokeable longer-lived refresh token.

A token that's valid when it's issued should only be trusted for a very short time.

If a users account is compromised - let's say a malicious co-worker or relative used their username and password - the compromised user should be able to take action, like resetting their password, and the compromised tokens should no longer be valid.

Even think of Netflix, etc which give you a "Sign out of all accounts" option. That's invalidating all previously issued tokens.

The way we've approached this on WPGraphQL JWT Authentication is issuing 2 tokens. A short-lived access/auth token (5 min expiration) and a long-lived (1-year) refresh token.

The authToken can be used to do things like read private data, execute mutations (writes), etc.

The refreshToken is only good for getting a new authToken. If an authToken is compromised, it's limited to 5 minutes, which may even be considered long, especially if you're the one who's token was compromised.

Anyway, our flow looks like this:

User logs in and asks for a jwtAuthToken and jwtRefreshToken in repsonse.
for future requests, the jwtAuthToken is checked to see if it's expired or not.
- If not expired, it's added to the headers of the request and used.
- If it is expired, a pre-flight request is sent using the jwtRefreshToken to ask for a new jwtAuthToken
  - If the request succeeds, a new jwtAuthToken is provided
  - if the request fails (the jwtRefreshToken is invalid. . .manually revoked by an admin, pw reset, etc) then a new jwtAuthToken is not issued and the user must login again to get new tokens.

If we're using a single token (haven't really dug into the code yet) then it absolutely needs to be stateful.

Review fixes

Refresh Tokens

…ermalinks

Create readme.md

…hub.com/jkmassel/jwt-auth into fix-get-requests

Allow for both plain and pretty permalink structures

kadamwhite · 2019-08-15T21:48:32Z

After discussion with @TimothyBJacobs , the size of this PR is prohibitive for a thorough review. We've all had a lot of eyes on it over the past few months; let's land this and iterate rather than letting this linger further. Thank you @valendesigns !

renatonascalves · 2019-08-15T22:02:40Z

@kadamwhite agree! Cool! :)

valendesigns added 7 commits November 18, 2018 13:47

Development version 0.1

0b17f1e

Additional configs

337239d

Update build matrix

738046f

Remove whitespace

8e3f267

Add php-coveralls

67ee609

Clean up

1f20422

Fix build

f9f1cef

valendesigns self-assigned this Nov 18, 2018

valendesigns requested a review from kadamwhite November 18, 2018 22:21

valendesigns added 5 commits March 7, 2019 17:26

Clean up tools & add tests

82a263c

Ensure email|login changes invalidate token

7ccfc84

Ensure password change revokes all tokens

16493c1

Only support REST for now

2ef940e

Fix spacing

e3fda4d

kadamwhite requested changes Mar 14, 2019

View reviewed changes

kadamwhite mentioned this pull request Mar 14, 2019

Adopt modern WP frontend standards #3

Open

renatonascalves suggested changes Mar 19, 2019

View reviewed changes

jkmassel and others added 5 commits March 28, 2019 16:27

Allow GET requests to use tokens, if provided

b25d117

Fix manual token creation with plain permalinks

0036f10

Uses the built-in `get_rest_url` API to handle all situations. Return only the path from `get_rest_uri`

Enforce the default permalink style in testing

68a2989

Test both permalink types

0373219

Add missing vendor name in package name

d233067

ocean90 reviewed Apr 5, 2019

View reviewed changes

composer.json Outdated Show resolved Hide resolved

valendesigns and others added 26 commits May 9, 2019 01:34

Merge pull request #10 from WP-API/review-fixes

4a0d1ea

Review fixes

Update plugin header

055d9b6

Add refresh tokens

b9e788a

Merge branch 'develop' into refresh-token

a822124

Update UI to handle new refresh token

a84b9a1

Revert test name change

7abbe76

Merge pull request #11 from WP-API/refresh-token

e632e2d

Refresh Tokens

Fix test failures

2e28eb5

Merge branch 'develop' into fix/privileged-get-requests-not-working

9259df0

Merge branch 'develop' into add-readme

bb69088

Fix Multi-site

21edd99

Merge branch 'develop' into fix/manual-tokens-not-working-for-plain-p…

96b85c0

…ermalinks

Fix token checking

a06359a

Merge changes & rework solution

f0735f7

Rework docs & ensure token is generated with key-pair only

e4c66f9

Merge pull request #9 from WP-API/add-readme

191ac93

Create readme.md

Add token validate endpoint

b7ea5a0

Fix spacing

9f68e99

Merge branch 'fix/privileged-get-requests-not-working' of https://git…

654dd6b

…hub.com/jkmassel/jwt-auth into fix-get-requests

Fix typo

91fce0d

Update GET request comment

85094e5

Merge branch 'fix-get-requests' into develop

1dcc3e9

Merge branch 'develop' into auth-fix

1e49c96

Ensure fields autoselect. closes #2

989f2f8

Merge branch 'develop' into auth-fix

e1e7e88

Merge pull request #12 from WP-API/auth-fix

855cfab

Allow for both plain and pretty permalink structures

jeffpaul mentioned this pull request Jul 5, 2019

Create CONTRIBUTORS.md #16

Merged

kadamwhite merged commit a68125c into master Aug 15, 2019

Conversation

valendesigns commented Nov 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kadamwhite left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonbahl commented Mar 15, 2019

Uh oh!

valendesigns commented Mar 17, 2019

Uh oh!

renatonascalves left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kadamwhite commented Apr 3, 2019

Uh oh!

Uh oh!

ocean90 commented Apr 11, 2019

Uh oh!

valendesigns commented Nov 18, 2018 •

edited

Loading