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.

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

Merged
merged 9 commits into from
Mar 2, 2021

Conversation

clarkwinkelmann
Copy link
Member

Fixes #2519

Changes proposed in this pull request:
Now only users with user.viewLastSeenAt permission can use the lastSeenAt sort on GET /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 means Extend\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

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).

Required changes:

We need to warn about the "removed" sort in the upgrade guide.

Copy link
Member

@askvortsov1 askvortsov1 left a 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');
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 might have messed up the merge. I think this line has disappeared from master

@askvortsov1 askvortsov1 requested a review from SychO9 March 2, 2021 02:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

List user endpoint allows guessing user last time even when not disclosed
3 participants