Skip to content

Commit

Permalink
Restore beta.9 behavior of assertCan()
Browse files Browse the repository at this point in the history
In flarum#1854, I changed the implementation of `assertCan()` to be
more aware of the user's log-in status. I came across this when unifying
our API's response status code when actors are not authenticated or not
authorized to do something.

@luceos rightfully had to tweak this again in ea84fc4, because the
behavior changed for one of the few API endpoints that checked for a
permission that even guests can have.

It turns out having this complex behavior in `assertCan()` is quite
misleading, because the name suggests a simple permission check and
nothing more.

Where we actually want to differ between HTTP 401 and 403, we can do
this using two method calls, and enforce it with our tests.

If this turns out to be problematic or extremely common, we can revisit
this and introduce a method with a different, better name in the future.

This commit restores the method's behavior in the last release, so we
also avoid another breaking change for extensions.
  • Loading branch information
franzliedke authored and wzdiyb committed Feb 16, 2020
1 parent 4564394 commit 108a1bf
Show file tree
Hide file tree
Showing 4 changed files with 6 additions and 16 deletions.
1 change: 1 addition & 0 deletions src/Group/Command/CreateGroupHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ public function handle(CreateGroup $command)
$actor = $command->actor;
$data = $command->data;

$this->assertRegistered($actor);
$this->assertCan($actor, 'createGroup');

$group = Group::build(
Expand Down
17 changes: 3 additions & 14 deletions src/User/AssertPermissionTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -55,28 +55,17 @@ protected function assertRegistered(User $actor)
* @param User $actor
* @param string $ability
* @param mixed $arguments
* @throws NotAuthenticatedException
* @throws PermissionDeniedException
*/
protected function assertCan(User $actor, $ability, $arguments = [])
{
// Identify whether guest or user has the permission.
$can = $actor->can($ability, $arguments);

// For non-authenticated users, we throw a different exception to signal
// that logging in may help.
if (! $can) {
$this->assertRegistered($actor);
}

// If we're logged in, then we need to communicate that the current
// account simply does not have enough permissions.
$this->assertPermission($can);
$this->assertPermission(
$actor->can($ability, $arguments)
);
}

/**
* @param User $actor
* @throws NotAuthenticatedException
* @throws PermissionDeniedException
*/
protected function assertAdmin(User $actor)
Expand Down
2 changes: 1 addition & 1 deletion src/User/Command/RegisterUserHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public function handle(RegisterUser $command)
$data = $command->data;

if (! $this->settings->get('allow_sign_up')) {
$this->assertPermission($actor->can('administrate'));
$this->assertAdmin($actor);
}

$password = Arr::get($data, 'attributes.password');
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/api/users/ListTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ public function disallows_index_for_guest()
$this->request('GET', '/api/users')
);

$this->assertEquals(401, $response->getStatusCode());
$this->assertEquals(403, $response->getStatusCode());
}

/**
Expand Down

0 comments on commit 108a1bf

Please sign in to comment.