Skip to content

🔐 Implement authorization checks for removing relations #108

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

🔐 Implement authorization checks for removing relations #108

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7b2a202
Implement authorization for removing and changing relationships
erikgaal Mar 30, 2019
71acdd4
Apply fixes from StyleCI
erikgaal Mar 30, 2019
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
26 changes: 26 additions & 0 deletions src/Eloquent/Concerns/Authorizable.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,32 @@ public function authorizedToAdd(Model $model): bool
return $this->authorized($method, [$model]);
}

/**
* Determine if the current user can add the given model to the model or throw an exception.
*
* @param \Illuminate\Database\Eloquent\Model $model
* @throws \Illuminate\Auth\Access\AuthorizationException
*/
public function authorizeToRemove(Model $model): void
{
$method = 'remove'.str_singular(class_basename($model));

$this->authorize($method, [$model]);
}

/**
* Determine if the current user can add the given model to the model.
*
* @param \Illuminate\Database\Eloquent\Model $model
* @return bool
*/
public function authorizedToRemove(Model $model): bool
{
$method = 'remove'.str_singular(class_basename($model));

return $this->authorized($method, [$model]);
}

/**
* Determine if the current user can attach the given model to the model.
*
Expand Down
45 changes: 45 additions & 0 deletions src/Eloquent/Concerns/InteractsWithRelations.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,9 +94,17 @@ protected function fillConnections(array $connections)
* @param Relations\BelongsTo $relation
* @param mixed $id
* @return void
* @throws \Illuminate\Auth\Access\AuthorizationException
*/
protected function connectBelongsToRelation(Relations\BelongsTo $relation, $id)
{
$current = $relation->first();

if ($current) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean we always call authorizeToRemove when there’s an existing relation? Seems a bit overkill if you are just updating it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean with just updating it?

$old = [
  'related_id' => '1',
]
$input = [
  'related_id' => '1',
]

I do however believe that when you're not actually changing the relation, it should not need to authorize changing the fields. (e.g. only check dirty/changed fields)

Copy link
Contributor

Choose a reason for hiding this comment

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

If you would update it with 'related_id' => '2' it would trigger the authorizeToRemove that feels unnecessary. Definitely agree on skipping authorisation when it's not dirty.

$currentSchema = $this->registry->getSchemaForModel($current);
$currentSchema->authorizeToRemove($this->getModel());
}

if (! $id) {
$relation->dissociate();

Expand All @@ -119,9 +127,17 @@ protected function connectBelongsToRelation(Relations\BelongsTo $relation, $id)
* @param Relations\BelongsTo $relation
* @param array $attributes
* @return void
* @throws \Illuminate\Auth\Access\AuthorizationException
*/
protected function fillBelongsToRelation(Relations\BelongsTo $relation, $attributes = [])
{
$current = $relation->first();

if ($current) {
$currentSchema = $this->registry->getSchemaForModel($current);
$currentSchema->authorizeToRemove($this->getModel());
}

if (! $attributes) {
$relation->dissociate();

Expand All @@ -145,9 +161,16 @@ protected function fillBelongsToRelation(Relations\BelongsTo $relation, $attribu
* @param Relations\HasOne $relation
* @param string $id
* @return void
* @throws \Illuminate\Auth\Access\AuthorizationException
*/
protected function connectHasOneRelation(Relations\HasOne $relation, $id)
{
$current = $relation->first();

if ($current) {
$this->authorizeToRemove($current);
}

if (! $id) {
$relation->delete();

Expand All @@ -171,6 +194,12 @@ protected function connectHasOneRelation(Relations\HasOne $relation, $id)
*/
protected function fillHasOneRelation(Relations\HasOne $relation, $attributes)
{
$current = $relation->first();

if ($current) {
$this->authorizeToRemove($current);
}

if (! $attributes) {
$relation->delete();

Expand All @@ -195,9 +224,17 @@ protected function fillHasOneRelation(Relations\HasOne $relation, $attributes)
* @param Relations\HasMany $relation
* @param array $ids
* @return void
* @throws \Illuminate\Auth\Access\AuthorizationException
*/
protected function connectHasManyRelation(Relations\HasMany $relation, array $ids)
{
$models = $relation->get();

foreach ($models as $model) {
$currentSchema = $this->registry->getSchemaForModel($model);
$currentSchema->authorizeToRemove($this->getModel());
}

$this->queue(function () use ($relation, $ids) {
$models = $relation->getModel()->findMany($ids);

Expand All @@ -215,9 +252,17 @@ protected function connectHasManyRelation(Relations\HasMany $relation, array $id
* @param Relations\HasMany $relation
* @param array $values
* @return void
* @throws \Illuminate\Auth\Access\AuthorizationException
*/
protected function fillHasManyRelation(Relations\HasMany $relation, array $values)
{
$models = $relation->get();

foreach ($models as $model) {
$currentSchema = $this->registry->getSchemaForModel($model);
$currentSchema->authorizeToRemove($this->getModel());
}

$this->queue(function () use ($relation, $values) {
$related = $relation->getRelated();
$relation->delete();
Expand Down
63 changes: 63 additions & 0 deletions tests/Feature/AuthorizationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,27 @@ public function it_cant_save_has_one_if_not_authorized()
$this->assertNull($user->phone);
}

/** @test */
public function it_cant_remove_has_one_if_not_authorized()
{
$user = factory(User::class)->create();
factory(Phone::class)->create(['user_id' => $user->id]);

$_SERVER['graphql.user.removePhone'] = false;

$this->withExceptionHandling()->graphql('mutation($id: ID!, $input: UpdateUserInput!) { updateUser(id: $id, input: $input) { id } }', [
'id' => $user->id,
'input' => [
'phone' => null,
],
]);

unset($_SERVER['graphql.user.removePhone']);

$user = User::first();
$this->assertNotNull($user->phone);
}

/** @test */
public function it_cant_create_and_add_has_many_if_not_authorized_to_create()
{
Expand Down Expand Up @@ -348,6 +369,27 @@ public function it_cant_add_has_many_if_not_authorized()
$this->assertTrue($user->articles->isEmpty());
}

/** @test */
public function it_cant_remove_has_many_if_not_authorized()
{
$user = factory(User::class)->create();
factory(Article::class)->create(['user_id' => $user->id]);

$_SERVER['graphql.user.removeArticle'] = false;

$this->withExceptionHandling()->graphql('mutation($id: ID!, $input: UpdateUserInput!) { updateUser(id: $id, input: $input) { id } }', [
'id' => $user->id,
'input' => [
'articles' => [],
],
]);

unset($_SERVER['graphql.user.removeArticle']);

$user = User::first();
$this->assertTrue($user->articles->isNotEmpty());
}

/** @test */
public function it_cant_create_and_add_belongs_to_if_not_authorized_to_create()
{
Expand Down Expand Up @@ -417,6 +459,27 @@ public function it_cant_add_belongs_to_if_not_authorized()
$this->assertNull($article->user);
}

/** @test */
public function it_cant_remove_belongs_to_if_not_authorized()
{
factory(User::class)->create();
$article = factory(Article::class)->create();

$_SERVER['graphql.user.removeArticle'] = false;

$this->withExceptionHandling()->graphql('mutation($id: ID!, $input: UpdateArticleInput!) { updateArticle(id: $id, input: $input) { id } }', [
'id' => $article->id,
'input' => [
'user' => null,
],
]);

unset($_SERVER['graphql.user.removeArticle']);

$article = Article::first();
$this->assertNotNull($article->user);
}

/** @test */
public function it_cant_attach_belongs_to_many_if_not_authorized()
{
Expand Down
5 changes: 5 additions & 0 deletions tests/Fixtures/Policies/PhonePolicy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,9 @@ public function delete(): bool
{
return $_SERVER['graphql.phone.deletable'] ?? true;
}

public function removeUser(): bool
{
return $_SERVER['graphql.phone.removeUser'] ?? true;
}
}
10 changes: 10 additions & 0 deletions tests/Fixtures/Policies/UserPolicy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,21 @@ public function addPhone(): bool
return $_SERVER['graphql.user.addPhone'] ?? true;
}

public function removePhone(): bool
{
return $_SERVER['graphql.user.removePhone'] ?? true;
}

public function addArticle(): bool
{
return $_SERVER['graphql.user.addArticle'] ?? true;
}

public function removeArticle(): bool
{
return $_SERVER['graphql.user.removeArticle'] ?? true;
}

public function addComment(): bool
{
return $_SERVER['graphql.user.addComment'] ?? true;
Expand Down