Skip to content
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.

Sign up for GitHub

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

Limit users to the current site #2670

Merged
merged 7 commits into from
Mar 28, 2022
Merged

Limit users to the current site #2670

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1235348
Limit users to the current site
felipeelia Mar 21, 2022
e68a69b
Unit tests
felipeelia Mar 21, 2022
eba243b
e2e tests
felipeelia Mar 21, 2022
3658f04
Small refactor of the code
felipeelia Mar 22, 2022
fb530f5
Merge branch 'develop' into fix/issue-2599
felipeelia Mar 22, 2022
c1907e1
Merge branch 'develop' into fix/issue-2599
felipeelia Mar 25, 2022
b99173d
Merge branch 'develop' into fix/issue-2599
oscarssanchez Mar 28, 2022
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
18 changes: 17 additions & 1 deletion includes/classes/Indexable/User/User.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,22 @@ public function format_args( $query_vars, $query ) {

$use_filters = true;
}

// If there are no specific roles named, make sure the user is a member of the site.
if ( ! $use_filters ) {
Copy link
Contributor

@tfrommen tfrommen Mar 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

After I submitted my patch (see #2601), I was wondering if relying on $use_filters was a good idea. Right now, this works fine, just because there are no conditions that would set $use_filters to true except for the ones related to roles. However, if there ever was some new code to be added after the inital $use_filters = false; but before the roles conditions, this would no longer be true.

Instead of checking $use_filters, one could do it like so (which is a little more verbose, but absolutely specific to what we're after):

Suggested change
if ( ! $use_filters ) {
if ( empty( $query_vars['role'] ) && empty( $query_vars['role__in'] ) && empty( $query_vars['role__not_in'] ) ) {

This should work just fine with your tests as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

You have a good point @tfrommen. I went ahead and used that to do a small refactor in the entire if block. Hopefully, it'll make the code more future-proof and also easier to be understood by new programmers. Thanks!

$filter['bool']['must'][] = array(
'exists' => array(
'field' => 'capabilities.' . $blog_id . '.roles',
),
);
$filter['bool']['must_not'][] = array(
'term' => array(
'capabilities.' . $blog_id . '.roles' => 0,
),
);

$use_filters = true;
}
}
}

Expand Down Expand Up @@ -876,7 +892,7 @@ public function prepare_capabilities( $user_id ) {

if ( ! empty( $roles ) ) {
$prepared_roles[ (int) $site['blog_id'] ] = [
'roles' => array_keys( $roles ),
'roles' => array_keys( (array) $roles ),
];
}
}
Expand Down
45 changes: 45 additions & 0 deletions tests/cypress/integration/indexables/user.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,4 +104,49 @@ describe('User Indexable', () => {
expect(text).to.contain('"value": "Doe"');
});
});

it('Only returns users from the current blog', () => {
cy.login();

cy.maybeEnableFeature('users');

const newUserData = {
userLogin: 'nobloguser',
userEmail: 'no-blog-user@example.com',
};

cy.wpCli(`wp user get ${newUserData.userLogin} --field=ID`, true).then((wpCliResponse) => {
if (wpCliResponse.code === 0) {
cy.wpCli(`wp user delete ${newUserData.userLogin} --yes --network`);
cy.wpCli('wp elasticpress index --setup --yes');
}
});

// Create a user without a blog.
cy.visitAdminPage('network/user-new.php');
cy.get('#username').clearThenType(newUserData.userLogin);
cy.get('#email').clearThenType(newUserData.userEmail);
cy.get('#add-user').click();
cy.get('#message.updated').should('be.visible');

// Searching for it should not return anything.
searchUser('nobloguser');
cy.get('.wp-list-table').should('contain.text', 'No users found.');
cy.getTotal(0);
cy.get('.ep-query-debug').should('contain.text', 'Query Response Code: HTTP 200');

// Add user to the blog.
cy.visitAdminPage('user-new.php');
cy.get('#adduser-email').clearThenType(newUserData.userLogin);
cy.get('#adduser-noconfirmation').check();
cy.get('#addusersub').click();
cy.get('#message.updated').should('be.visible');

// Searching for it should return it.
searchUser('nobloguser');
cy.get('.wp-list-table').should('contain.text', 'nobloguser');
cy.getTotal(1);
cy.get('.ep-query-debug').should('contain.text', 'Query Response Code: HTTP 200');
cy.get('.query-results').should('contain.text', '"user_email": "no-blog-user@example.com"');
});
});
54 changes: 54 additions & 0 deletions tests/php/indexables/TestUser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1439,4 +1439,58 @@ public function testIntegrateSearchQueries() {

$this->assertTrue( $this->get_feature()->integrate_search_queries( false, $query ) );
}

/**
* Test users that does not belong to any blog.
*
* @since 4.1.0
*/
public function testUserSearchLimitedToOneBlog() {
// This user does not belong to any blog.
Functions\create_and_sync_user(
[
'user_login' => 'users-and-blogs-1',
'role' => '',
'first_name' => 'No Blog',
'last_name' => 'User',
'user_email' => 'no-blog@test.com',
'user_url' => 'http://domain.test',
]
);
Functions\create_and_sync_user(
[
'user_login' => 'users-and-blogs-2',
'role' => 'contributor',
'first_name' => 'Blog',
'last_name' => 'User',
'user_email' => 'blog@test.com',
'user_url' => 'http://domain.test',
]
);

ElasticPress\Elasticsearch::factory()->refresh_indices();

// Here `blog_id` defaults to `get_current_blog_id()`.
$query = new \WP_User_Query( [
'search' => 'users-and-blogs'
] );

$this->assertTrue( $this->get_feature()->integrate_search_queries( false, $query ) );
$this->assertEquals( 1, $query->total_users );
foreach ( $query->results as $user ) {
$this->assertTrue( $user->elasticsearch );
}

// Search accross all blogs.
$query = new \WP_User_Query( [
'search' => 'users-and-blogs',
'blog_id' => 0,
] );

$this->assertTrue( $this->get_feature()->integrate_search_queries( false, $query ) );
$this->assertEquals( 2, $query->total_users );
foreach ( $query->results as $user ) {
$this->assertTrue( $user->elasticsearch );
}
}
}