Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions src/Model/Post.php
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,15 @@
* @property array $editLock
* @property string $enclosure
* @property string $guid
* @property bool $hasPassword
* @property int $menuOrder
* @property string $link
* @property string $uri
* @property int $commentCount
* @property string $featuredImageId
* @property int $featuredImageDatabaseId
* @property string $pageTemplate
* @property string $password
* @property int $previewRevisionDatabaseId
*
* @property string $captionRaw
Expand Down Expand Up @@ -152,6 +154,7 @@ public function __construct( WP_Post $post ) {
'isPostsPage',
'isFrontPage',
'isPrivacyPage',
'hasPassword',
];

if ( isset( $this->post_type_object->graphql_single_name ) ) {
Expand Down Expand Up @@ -595,6 +598,12 @@ protected function init() {

return false;
},
'hasPassword' => function () {
return ! empty( $this->data->post_password );

Choose a reason for hiding this comment

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

Avoid too many return statements within this method.

},
'password' => function () {
return ! empty( $this->data->post_password ) ? $this->data->post_password : null;
},
Comment on lines +604 to +606
Copy link
Collaborator

Choose a reason for hiding this comment

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

why did the capability check get removed from this field? Is it already protected by other logic in the model? Want to make sure we're not exposing passwords on posts to folks that shouldn't be able to access the password 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup, it's protected top-level by Model::has_restricted_cap() and tested here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

see my other comment: #3073 (comment)

'toPing' => function () {
$to_ping = get_to_ping( $this->databaseId );

Expand Down Expand Up @@ -690,12 +699,6 @@ protected function init() {

return ! empty( $thumbnail_id ) ? absint( $thumbnail_id ) : null;
},
'password' => [
'callback' => function () {
return ! empty( $this->data->post_password ) ? $this->data->post_password : null;
},
'capability' => isset( $this->post_type_object->cap->edit_others_posts ) ?: 'edit_others_posts',
],
'enqueuedScriptsQueue' => static function () {
global $wp_scripts;
do_action( 'wp_enqueue_scripts' );
Expand Down
16 changes: 16 additions & 0 deletions src/Registry/Utils/PostObject.php
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,22 @@ public static function get_fields( WP_Post_Type $post_type_object ) {
return absint( $post->ID );
},
],
'hasPassword' => [
'type' => 'Boolean',
'description' => sprintf(
// translators: %s: custom post-type name.
__( 'Whether the %s object is password protected.', 'wp-graphql' ),
$post_type_object->name
),
],
'password' => [
'type' => 'String',
'description' => sprintf(
// translators: %s: custom post-type name.
__( 'The password for the %s object.', 'wp-graphql' ),
$post_type_object->name
),
],
];

if ( 'page' === $post_type_object->name ) {
Expand Down
138 changes: 136 additions & 2 deletions tests/wpunit/PostObjectQueriesTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,6 @@ public function set_permalink_structure( $structure = '' ) {
}

public function createPostObject( $args ) {

/**
* Set up the $defaults
*/
Expand Down Expand Up @@ -212,6 +211,8 @@ static function ( $param ) {
}
enclosure
excerpt
hasPassword
password
status
link
postId
Expand Down Expand Up @@ -270,6 +271,8 @@ static function ( $param ) {
'slug' => 'test-title-for-postobjectqueriestest',
'toPing' => null,
'pinged' => null,
'hasPassword' => false,
'password' => null,
'modified' => \WPGraphQL\Utils\Utils::prepare_date_response( get_post( $post_id )->post_modified ),
'modifiedGmt' => \WPGraphQL\Utils\Utils::prepare_date_response( get_post( $post_id )->post_modified_gmt ),
'title' => apply_filters( 'the_title', 'Test Title for PostObjectQueriesTest' ),
Expand Down Expand Up @@ -2396,7 +2399,7 @@ public function testQueryPostBySlugWithNonAsciiSlug() {
);

// assert that the response is what we expect
self::assertQuerySuccessful(
$this->assertQuerySuccessful(
$actual,
[
$this->expectedField( 'post.__typename', 'Post' ),
Expand All @@ -2406,4 +2409,135 @@ public function testQueryPostBySlugWithNonAsciiSlug() {
]
);
}

public function testPasswordProtectedPost() {
$subscriber = $this->factory()->user->create(
[
'role' => 'subscriber',
]
);

$post_author_one = $this->factory()->user->create(
[
'role' => 'author',
]
);

$post_author_two = $this->factory()->user->create(
[
'role' => 'author',
]
);

$post = $this->factory()->post->create_and_get(
[
'post_type' => 'post',
'post_status' => 'publish',
'post_password' => 'mypassword',
'title' => 'Password Protected post',
'content' => 'Some content',
'post_author' => $post_author_one,
]
);

$query = '
query GetPasswordProtectedPost( $id: ID! ) {
post( id: $id, idType: DATABASE_ID ) {
title
content
status
password
hasPassword
}
}
';

// Test password protected post as unauthenticated user.
$actual = $this->graphql( [
'query' => $query,
'variables' => [
'id' => $post->ID,
],
] );

$this->assertArrayNotHasKey( 'errors', $actual );
$this->assertEquals( $post->post_title, $actual['data']['post']['title'], 'Title should be returned when unauthenticated' );
$this->assertEquals( 'publish', $actual['data']['post']['status'], 'Status should be "publish" when unauthenticated' );
$this->assertNull( $actual['data']['post']['content'], 'Content should be null when unauthenticated' );
$this->assertNull( $actual['data']['post']['password'], 'Password should be null when unauthenticated' );
$this->assertTrue( $actual['data']['post']['hasPassword'], 'hasPassword should be true when unauthenticated' );

// Test password protected post as a subscriber.
wp_set_current_user( $subscriber );
$actual = $this->graphql( [
'query' => $query,
'variables' => [
'id' => $post->ID,
],
] );

$this->assertArrayNotHasKey( 'errors', $actual );
$this->assertEquals( $post->post_title, $actual['data']['post']['title'], 'Title should be returned when lacking permissions' );
$this->assertEquals( 'publish', $actual['data']['post']['status'], 'Status should be "publish" when lacking permissions' );
$this->assertNull( $actual['data']['post']['content'], 'Content should be null when lacking permissions' );
$this->assertNull( $actual['data']['post']['password'], 'Password should be null when lacking permissions' );
$this->assertTrue( $actual['data']['post']['hasPassword'], 'hasPassword should be true when lacking permissions' );

// Test password protected post as different author.
wp_set_current_user( $post_author_two );
$actual = $this->graphql( [
'query' => $query,
'variables' => [
'id' => $post->ID,
],
] );

$this->assertArrayNotHasKey( 'errors', $actual );
$this->assertEquals( $post->post_title, $actual['data']['post']['title'], 'Title should be returned when lacking permissions' );
$this->assertEquals( 'publish', $actual['data']['post']['status'], 'Status should be "publish" when lacking permissions' );
$this->assertNull( $actual['data']['post']['content'], 'Content should be null when lacking permissions' );
$this->assertNull( $actual['data']['post']['password'], 'Password should be null when lacking permissions' );
$this->assertTrue( $actual['data']['post']['hasPassword'], 'hasPassword should be true when lacking permissions' );

// Test password protected post as current author.
wp_set_current_user( $post_author_one );

$actual = $this->graphql( [
'query' => $query,
'variables' => [
'id' => $post->ID,
],
] );

$this->assertArrayNotHasKey( 'errors', $actual );
$this->assertEquals( $post->post_title, $actual['data']['post']['title'], 'Title should be returned when authenticated' );
$this->assertEquals( 'publish', $actual['data']['post']['status'], 'Status should be "publish" when authenticated' );
$this->assertNotEmpty( $actual['data']['post']['content'], 'Content should be returned when authenticated' );
$this->assertEquals( $post->post_password, $actual['data']['post']['password'], 'Password should be returned when authenticated' );
$this->assertTrue( $actual['data']['post']['hasPassword'], 'hasPassword should be true when authenticated' );

// Test password protected post as admin.
wp_set_current_user( $this->admin );
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need to also test this with other roles.

The code used to check $this->post_type_object->cap->edit_others_posts before returning the password.

So I believe this means the password will be exposed to admins but not other roles like subscriber. (similar to the wp-admin, where an admin could see the password of a post, but a subscriber could not)

Copy link
Collaborator

Choose a reason for hiding this comment

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

The logic of just checking the capability might not even be enough though, because I think if you don't have that capability BUT you are the author of the post itself you should be able to see the password?

Copy link
Collaborator Author

@justlevine justlevine Mar 27, 2024

Choose a reason for hiding this comment

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

What other caps do you want to test against?

This PR intentionally doesn't change the existing logic for handling password-protected posts (which should be addressed in #930 ), and the test currently checks:

  • an unauthenticated user
  • an authenticated user that doesn't have the correct cap (a regular subscriber)
  • an authenticated user that does have the correct cap (admin)

Once we tackle #930, we may want to change the cap check to edit_post (as handled in WP_REST_Posts_Controller::can_access_password_content() versus the even more restrictive than currently frontend check by post_password_required() where everyone needs to supply a password), but that security loosening should probably be considered a breaking change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@justlevine ya I mean the password field itself.

In normal WordPress, not everyone is able to see the password that has been set for a post, even if they can see other fields of that post.

For example, if I'm an admin, I can set the password on a post and therefore also see the value of the existing password that is set for a post.

However, if I am authenticated in WordPress as an "author", I can see the "password protected post" and fields like the title, author, date, etc. . .BUT, I cannot see the "password" field.

CleanShot 2024-03-27 at 15 55 16

That field is restricted only to users with the edit_others_posts capability.

Since WordPress itself does not expose the "password" value to users that do not have proper permission to see/update the password, WPGraphQL also should not "leak" the password to users that should not be able to see the password.

So, being able to resolve the value of password field is more nuanced than the Post itself. The has_restricted_cap() covers the Post node as a whole, but this field has more granular requirements. Not only do you have to be able to see the Node itself, BUT you also have to be able to see the password, which not every user will have permission to do so.

That's what this code does. . .it further restricts this field on the model to resolve only if the user has proper capabilities to see the password field.

I think the current logic is a bit naiive and should be more robust to not JUST check if the current user has edit_other_post cap, but also other conditions such as "is the current user the author of the post"

For example, an Author can actually publish their own post and set a password on it, and so the author of a post should be able to see the password for their own post:

CleanShot 2024-03-27 at 15 57 22

But also an admin that can edit_others_posts should also be able to see the password field.

Regardless, at minimum we should probably add a test to test against the author role and ensure that they can get the title and date of a pw protected post they did not author, but cannot get the password .

Copy link
Collaborator Author

@justlevine justlevine Mar 28, 2024

Choose a reason for hiding this comment

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

@jasonbahl in b9e25bf, I added asserts against the current post author (password/content shown) and a different post author (password/content hidden), and fixed the subscriber assert (forgot to call set_current_user() 🤦). So hopefully that covers your concerns, but tbh I'm still not sure I fully understand:

For example, if I'm an admin, I can set the password on a post and therefore also see the value of the existing password that is set for a post.

However, if I am authenticated in WordPress as an "author", I can see the "password protected post" and fields like the title, author, date, etc. . .BUT, I cannot see the "password" field.
[image]
That field is restricted only to users with the edit_others_posts capability.

To clarify, on the frontend, WordPress uses post_password_required(), and requires everyone - including admin or the post author to provide a password, in order to see the post_content and password, while everything else remains visible.

On the dashboard list views/ edit screen, WP uses WP_REST_Posts_Controller::can_access_password_content(), allowing the current post author and any users with the edit_others_posts capability to open the Edit screen and view the password.

In both scenarios, other Post Object data remains visible (such as custom post meta).

So, being able to resolve the value of password field is more nuanced than the Post itself. The has_restricted_cap() covers the Post node as a whole, but this field has more granular requirements. Not only do you have to be able to see the Node itself, BUT you also have to be able to see the password, which not every user will have permission to do so.

Agreed, but to the opposite intent. Currently get_restricted_cap()'s check for post_password results in a stricter permissions model than either WP's frontend or REST API behavior, and makes the existing $field_config['capability'] check redundant.

In an ideal world (i.e. after or as part of #930 ) we would find what the happy medium should be (e.g. should post meta be hidden by default), but regardless of what is decided, none of the password-restricted fields are going to be doable for with just a simple $field_config['capability'] check.

Where are we losing each other?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Where are we losing each other?

I think I was lost in the history of the project.

We had 2 different APIs for handling the same thing.

  • The capability check on individual Model fields
  • The allowed_restricted_fields / get_restricted_cap() on the Model itself

You are indeed correct that this was already being covered by the allowed_restricted_fields / get_restricted_cap().

To verify manually I executed the following query as 3 different users (public, author, admin)

query GetPostsAndCurrentViewer {
  posts {
    nodes {
      id
      title
      date
      password
    }
  }
  viewer {
    name
    roles {
      nodes {
        name
      }
    }
  }
}

Public User

As a public user, I can see that Password Protected posts exist, but I cannot see the password nor the password protected content. This is good. However, I cannot see whether the post hasPassword: true/false either, so we need to add hasPassword as an allowed restricted field. Public users should be able to identify whether a post has a password or not, but they should not be able to access the password itself.

CleanShot 2024-04-01 at 11 15 15

Author

As an Author, I can see that password protected posts exist, and for a PW protected post that I am the author of, I can see both the content and the password. But for a PW protected post authored by another user, I can see that it exists, but I cannot see the content nor the password. However, just like a public user, I cannot see the value of the hasPassword field.

CleanShot 2024-04-01 at 11 15 39

Admin

As an admin user I can see all the posts, their content, their passwords, etc whether I am the author of the post or not. This is as expected.

CleanShot 2024-04-01 at 11 14 34

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for clarifying - and great catch with hasPassword!

Will push that fix + tests and ping you for the re-review when it's ready

Copy link
Collaborator

Choose a reason for hiding this comment

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

@justlevine I just pushed changes 😉


$actual = $this->graphql( [
'query' => $query,
'variables' => [
'id' => $post->ID,
],
] );

$this->assertArrayNotHasKey( 'errors', $actual );
$this->assertEquals( $post->post_title, $actual['data']['post']['title'], 'Title should be returned when authenticated' );
$this->assertEquals( 'publish', $actual['data']['post']['status'], 'Status should be "publish" when authenticated' );
$this->assertNotEmpty( $actual['data']['post']['content'], 'Content should be returned when authenticated' );
$this->assertEquals( $post->post_password, $actual['data']['post']['password'], 'Password should be returned when authenticated' );
$this->assertTrue( $actual['data']['post']['hasPassword'], 'hasPassword should be true when authenticated' );

// @todo add case with password supplied once supported.
// @see https://github.com/wp-graphql/wp-graphql/issues/930

// cleanup
wp_delete_post( $post->ID, true );
wp_delete_user( $subscriber );
}
}