-
-
Notifications
You must be signed in to change notification settings - Fork 834
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
Restrict who can use the lastSeenAt user sort #2634
Conversation
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.
I like the approach taken here. That being said, I'd like to see 2 integration tests showing that requesting this sort succeeds/fails depending on user permission to avoid potential regressions in the future.
$this->removeSortField('lastSeenAt'); | ||
} | ||
|
||
$query = Arr::get($this->extractFilter($request), 'q'); |
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.
You might have messed up the merge. I think this line has disappeared from master
[ci skip] [skip ci]
Fixes #2519
Changes proposed in this pull request:
Now only users with
user.viewLastSeenAt
permission can use thelastSeenAt
sort onGET /api/users
endpoint.Trying to use the sort when unauthorized will result in the standard error
{"errors":[{"status":"400","code":"invalid_parameter"}]}
Reviewers should focus on:
This is based on what was discussed in the issue. Even though we agreed on removing the sort completely, I noticed it wasn't too complicated to keep it only for users with a given permission. So I implemented the original suggestion from the first post of the issue.
There's just one question with this approach... Do we want the sort field present by default or absent by default? Since we alter it in
data
, it meansExtend\ApiController::addSortField
/Extend\ApiController::removeSortField
has limited control over it. Question is, would an extension be more likely to want to disable it or enable it? Since it's already a mod-only feature I don't see much interest in disabling it, and since enabling would expose data I don't see that happening either 💩Confirmed
composer test
).Required changes:
We need to warn about the "removed" sort in the upgrade guide.