Skip to content

Commit

Permalink
fix: api allows user to change vote when they should not be able to (#96
Browse files Browse the repository at this point in the history
)

* fix: api allows user to change vote when they should not be able to

* Apply fixes from StyleCI

* fix: phpstan

* more test scenarios

* Apply fixes from StyleCI

* fix: phpstan

* chore: remove debug statements

* chore: return type

---------

Co-authored-by: StyleCI Bot <[email protected]>
  • Loading branch information
imorland and StyleCIBot authored Sep 16, 2024
1 parent bdb6d52 commit cfa40bd
Show file tree
Hide file tree
Showing 4 changed files with 257 additions and 26 deletions.
5 changes: 4 additions & 1 deletion src/Access/PollPolicy.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use Flarum\User\Access\AbstractPolicy;
use Flarum\User\User;
use FoF\Polls\Poll;
use Illuminate\Support\Arr;

class PollPolicy extends AbstractPolicy
{
Expand Down Expand Up @@ -58,9 +59,11 @@ public function vote(User $actor, Poll $poll)

public function changeVote(User $actor, Poll $poll)
{
if ($poll->allow_change_vote && $actor->hasPermission('polls.changeVote')) {
if ($actor->hasPermission('polls.changeVote')) {
return $this->allow();
}

return (bool) Arr::get($poll->settings, 'allow_change_vote', false);
}

public function edit(User $actor, Poll $poll)
Expand Down
73 changes: 50 additions & 23 deletions src/Commands/MultipleVotesPollHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,10 @@
use FoF\Polls\PollRepository;
use Illuminate\Contracts\Container\Container;
use Illuminate\Contracts\Events\Dispatcher;
use Illuminate\Database\ConnectionResolverInterface;
use Illuminate\Database\DatabaseManager;
use Illuminate\Database\Eloquent\Collection;
use Illuminate\Support\Arr;
use Illuminate\Validation\Factory;
use Pusher;

class MultipleVotesPollHandler
{
Expand All @@ -49,7 +49,7 @@ class MultipleVotesPollHandler
private $validation;

/**
* @var ConnectionResolverInterface
* @var DatabaseManager
*/
private $db;

Expand All @@ -63,7 +63,7 @@ class MultipleVotesPollHandler
* @param SettingsRepositoryInterface $settings
* @param Container $container
*/
public function __construct(PollRepository $polls, Dispatcher $events, SettingsRepositoryInterface $settings, Container $container, Factory $validation, ConnectionResolverInterface $db)
public function __construct(PollRepository $polls, Dispatcher $events, SettingsRepositoryInterface $settings, Container $container, Factory $validation, DatabaseManager $db)
{
$this->polls = $polls;
$this->events = $events;
Expand Down Expand Up @@ -96,25 +96,10 @@ public function handle(MultipleVotesPoll $command)
$maxVotes = $options->count();
}

$validator = $this->validation->make([
'options' => $optionIds,
], [
'options' => [
'present',
'array',
'max:'.$maxVotes,
function ($attribute, $value, $fail) use ($options) {
foreach ($value as $optionId) {
if (!$options->contains('id', $optionId)) {
$fail('Invalid option ID.');
}
}
},
],
]);
$this->validateInput($optionIds, $maxVotes, $options);

if ($validator->fails()) {
throw new ValidationException(['options' => $validator->getMessageBag()->first('options')]);
if ($this->isChangingVotes($optionIds, $myVotes->pluck('option_id')->toArray())) {
$actor->assertCan('changeVote', $poll);
}

$deletedVotes = $myVotes->filter(function ($vote) use ($optionIds) {
Expand All @@ -124,7 +109,6 @@ function ($attribute, $value, $fail) use ($options) {
return !$myVotes->contains('option_id', $optionId);
});

/** @phpstan-ignore-next-line */
$this->db->transaction(function () use ($myVotes, $options, $newOptionIds, $deletedVotes, $poll, $actor) {
// Unvote options
if ($deletedVotes->isNotEmpty()) {
Expand Down Expand Up @@ -240,4 +224,47 @@ public static function pusher(Container $container, SettingsRepositoryInterface
);
}
}

protected function isChangingVotes(array $optionIds, array $myVotes): bool
{
// Cast the values to integers
foreach ($optionIds as $optionId => $value) {
$optionIds[$optionId] = (int) $value;
}

foreach ($myVotes as $voteId => $value) {
$myVotes[$voteId] = (int) $value;
}

// Check the arrays have the same values
$same = (count(array_diff($optionIds, $myVotes)) === 0 && count(array_diff($myVotes, $optionIds)) === 0);

return !$same;
}

protected function validateInput(?array $optionIds, int $maxVotes, Collection $options): void
{
$validator = $this->validation->make([
'options' => $optionIds,
], [
'options' => [
'present',
'array',
'max:'.$maxVotes,
function ($attribute, $value, $fail) use ($options) {
if (is_array($value)) {
foreach ($value as $optionId) {
if (!$options->contains('id', $optionId)) {
$fail('Invalid option ID.');
}
}
}
},
],
]);

if ($validator->fails()) {
throw new ValidationException(['options' => $validator->getMessageBag()->first('options')]);
}
}
}
3 changes: 1 addition & 2 deletions src/Poll.php
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
* @property-read bool $allow_multiple_votes
* @property-read int $max_votes
* @property-read bool $hide_votes
* @property-read bool $allow_change_vote
* @property int $vote_count
* @property Post $post
* @property User $user
Expand All @@ -45,7 +44,7 @@
* @property string|null $image
* @property string|null $image_alt
*
* @phpstan-type PollSettings array{'public_poll': bool, 'allow_multiple_votes': bool, 'max_votes': int}
* @phpstan-type PollSettings array{'public_poll': bool, 'allow_multiple_votes': bool, 'max_votes': int, 'allow_change_vote' : bool}
*/
class Poll extends AbstractModel
{
Expand Down
202 changes: 202 additions & 0 deletions tests/integration/api/ChangeVoteTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,202 @@
<?php

/*
* This file is part of fof/polls.
*
* Copyright (c) FriendsOfFlarum.
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace FoF\Polls\Tests\integration\api;

use Carbon\Carbon;
use Flarum\Testing\integration\RetrievesAuthorizedUsers;
use Flarum\Testing\integration\TestCase;
use FoF\Polls\PollVote;

class ChangeVoteTest extends TestCase
{
use RetrievesAuthorizedUsers;

public function setUp(): void
{
parent::setUp();

$this->extension('fof-polls');

$this->prepareDatabase([
'users' => [
$this->normalUser(),
['id' => 3, 'username' => 'polluser', 'email' => '[email protected]', 'password' => 'too-obscure', 'is_email_confirmed' => true],
['id' => 4, 'username' => 'moderator', 'email' => '[email protected]', 'password' => 'too-obscure', 'is_email_confirmed' => true],
],
'discussions' => [
['id' => 1, 'title' => 'Discussion 1', 'comment_count' => 1, 'participant_count' => 1, 'created_at' => '2021-01-01 00:00:00'],
],
'posts' => [
['id' => 1, 'user_id' => 1, 'discussion_id' => 1, 'number' => 1, 'created_at' => '2021-01-01 00:00:00', 'content' => 'Post 1', 'type' => 'comment'],
],
'polls' => [
['id' => 1, 'question' => 'Testing Poll--Global', 'subtitle' => 'Testing subtitle', 'image' => 'pollimage-abcdef.png', 'image_alt' => 'test alt', 'post_id' => null, 'user_id' => 1, 'public_poll' => 0, 'end_date' => null, 'created_at' => '2021-01-01 00:00:00', 'updated_at' => '2021-01-01 00:00:00', 'vote_count' => 0, 'allow_multiple_votes' => 0, 'max_votes' => 0, 'settings' => '{"max_votes": 0,"hide_votes": false,"public_poll": false,"allow_change_vote": false,"allow_multiple_votes": false}'],
['id' => 2, 'question' => 'Testing Poll--Global 2', 'subtitle' => 'Testing subtitle', 'image' => 'pollimage-abcdef.png', 'image_alt' => 'test alt', 'post_id' => null, 'user_id' => 1, 'public_poll' => 0, 'end_date' => null, 'created_at' => '2021-01-01 00:00:00', 'updated_at' => '2021-01-01 00:00:00', 'vote_count' => 0, 'allow_multiple_votes' => 0, 'max_votes' => 0, 'settings' => '{"max_votes": 0,"hide_votes": false,"public_poll": false,"allow_change_vote": true,"allow_multiple_votes": false}'],
],
'poll_options' => [
['id' => 1, 'answer' => 'Option 1', 'poll_id' => 1, 'vote_count' => 0, 'image_url' => 'pollimage-hijklm.png', 'created_at' => '2021-01-01 00:00:00', 'updated_at' => '2021-01-01 00:00:00'],
['id' => 2, 'answer' => 'Option 2', 'poll_id' => 1, 'vote_count' => 0, 'image_url' => 'pollimage-nopqrs.png', 'created_at' => '2021-01-01 00:00:00', 'updated_at' => '2021-01-01 00:00:00'],
['id' => 3, 'answer' => 'Option 3', 'poll_id' => 2, 'vote_count' => 0, 'image_url' => 'pollimage-hijklm.png', 'created_at' => '2021-01-01 00:00:00', 'updated_at' => '2021-01-01 00:00:00'],
['id' => 4, 'answer' => 'Option 4', 'poll_id' => 2, 'vote_count' => 0, 'image_url' => 'pollimage-nopqrs.png', 'created_at' => '2021-01-01 00:00:00', 'updated_at' => '2021-01-01 00:00:00'],
],
'group_user' => [
['user_id' => 4, 'group_id' => 4],
],
'group_permission' => [
['permission' => 'discussion.polls.start', 'group_id' => 4],
['permission' => 'startGlobalPoll', 'group_id' => 4],
['permission' => 'uploadPollImages', 'group_id' => 4],
['permission' => 'polls.changeVote', 'group_id' => 4],
],
'poll_votes' => [
['id' => 1, 'poll_id' => 1, 'option_id' => 1, 'user_id' => 1, 'created_at' => Carbon::now(), 'updated_at' => Carbon::now()],
['id' => 2, 'poll_id' => 1, 'option_id' => 1, 'user_id' => 2, 'created_at' => Carbon::now(), 'updated_at' => Carbon::now()],
['id' => 3, 'poll_id' => 1, 'option_id' => 1, 'user_id' => 4, 'created_at' => Carbon::now(), 'updated_at' => Carbon::now()],
['id' => 4, 'poll_id' => 2, 'option_id' => 3, 'user_id' => 1, 'created_at' => Carbon::now(), 'updated_at' => Carbon::now()],
['id' => 5, 'poll_id' => 2, 'option_id' => 3, 'user_id' => 2, 'created_at' => Carbon::now(), 'updated_at' => Carbon::now()],
['id' => 6, 'poll_id' => 2, 'option_id' => 3, 'user_id' => 4, 'created_at' => Carbon::now(), 'updated_at' => Carbon::now()],
],
]);
}

public function usersWhoCanChangeVote(): array
{
return [
[1],
[4],
];
}

/**
* @test
*/
public function validation_error_when_no_data_is_passed()
{
$response = $this->send(
$this->request('PATCH', '/api/fof/polls/1/votes', [
'authenticatedAs' => 4,
'json' => [],
])
);

$this->assertEquals(422, $response->getStatusCode());

$data = json_decode($response->getBody(), true);

$this->assertEquals('The options must be an array.', $data['errors'][0]['detail']);
$this->assertEquals('/data/attributes/options', $data['errors'][0]['source']['pointer']);
}

/**
* @test
*
* @dataProvider usersWhoCanChangeVote
*/
public function user_with_permission_can_change_vote_on_no_change_poll(int $userId)
{
$response = $this->send(
$this->request('PATCH', '/api/fof/polls/1/votes', [
'authenticatedAs' => $userId,
'json' => [
'data' => [
'optionIds' => [
2,
],
],
],
])
);

$this->assertEquals(200, $response->getStatusCode());

$vote = PollVote::where('user_id', $userId)->where('poll_id', 1)->first();

$this->assertEquals(2, $vote->option_id);
}

/**
* @test
*/
public function user_without_permission_cannot_change_vote_on_no_change_poll()
{
$response = $this->send(
$this->request('PATCH', '/api/fof/polls/1/votes', [
'authenticatedAs' => 2,
'json' => [
'data' => [
'optionIds' => [
2,
],
],
],
])
);

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

$vote = PollVote::where('user_id', 2)->where('poll_id', 1)->first();

$this->assertEquals(1, $vote->option_id);
}

/**
* @test
*
* @dataProvider usersWhoCanChangeVote
*/
public function user_with_permission_can_change_vote_on_change_poll(int $userId)
{
$response = $this->send(
$this->request('PATCH', '/api/fof/polls/2/votes', [
'authenticatedAs' => $userId,
'json' => [
'data' => [
'optionIds' => [
4,
],
],
],
])
);

$this->assertEquals(200, $response->getStatusCode());

$vote = PollVote::where('user_id', $userId)->where('poll_id', 2)->first();

$this->assertEquals(4, $vote->option_id);
}

/**
* @test
*/
public function user_without_permission_can_change_vote_on_change_poll()
{
$response = $this->send(
$this->request('PATCH', '/api/fof/polls/2/votes', [
'authenticatedAs' => 2,
'json' => [
'data' => [
'optionIds' => [
4,
],
],
],
])
);

$this->assertEquals(200, $response->getStatusCode());

$vote = PollVote::where('user_id', 2)->where('poll_id', 2)->first();

$this->assertEquals(4, $vote->option_id);
}
}

0 comments on commit cfa40bd

Please sign in to comment.