-
Notifications
You must be signed in to change notification settings - Fork 466
feat: expose hasPassword and password fields on Post objects
#3073
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
3b326a5
b9e25bf
586ef48
6f4e2d0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
|
@@ -152,6 +154,7 @@ public function __construct( WP_Post $post ) { | |
| 'isPostsPage', | ||
| 'isFrontPage', | ||
| 'isPrivacyPage', | ||
| 'hasPassword', | ||
| ]; | ||
|
|
||
| if ( isset( $this->post_type_object->graphql_single_name ) ) { | ||
|
|
@@ -595,6 +598,12 @@ protected function init() { | |
|
|
||
| return false; | ||
| }, | ||
| 'hasPassword' => function () { | ||
| return ! empty( $this->data->post_password ); | ||
| }, | ||
| 'password' => function () { | ||
| return ! empty( $this->data->post_password ) ? $this->data->post_password : null; | ||
| }, | ||
|
Comment on lines
+604
to
+606
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 🤔
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup, it's protected top-level by
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ); | ||
|
|
||
|
|
@@ -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' ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -101,7 +101,6 @@ public function set_permalink_structure( $structure = '' ) { | |
| } | ||
|
|
||
| public function createPostObject( $args ) { | ||
|
|
||
| /** | ||
| * Set up the $defaults | ||
| */ | ||
|
|
@@ -212,6 +211,8 @@ static function ( $param ) { | |
| } | ||
| enclosure | ||
| excerpt | ||
| hasPassword | ||
| password | ||
| status | ||
| link | ||
| postId | ||
|
|
@@ -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' ), | ||
|
|
@@ -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' ), | ||
|
|
@@ -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 ); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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)
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
Once we tackle #930, we may want to change the cap check to
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. That field is restricted only to users with the 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 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 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: But also an admin that can Regardless, at minimum we should probably add a test to test against the
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
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 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 In both scenarios, other Post Object data remains visible (such as custom post meta).
Agreed, but to the opposite intent. Currently 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 Where are we losing each other?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think I was lost in the history of the project. We had 2 different APIs for handling the same thing.
You are indeed correct that this was already being covered by the To verify manually I executed the following query as 3 different users ( query GetPostsAndCurrentViewer {
posts {
nodes {
id
title
date
password
}
}
viewer {
name
roles {
nodes {
name
}
}
}
}Public UserAs 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 AuthorAs 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 AdminAs 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.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for clarifying - and great catch with Will push that fix + tests and ping you for the re-review when it's ready
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ); | ||
| } | ||
| } | ||





There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid too many
returnstatements within this method.