Problem/Motivation

There is a chance I have my site mis configured.

I would like the anonymous (public) to be able to see boards so have granted "Access Burndown Board" to anonymous.

I do not want them to be able to alter the tasks on the board so I set these all to authenticated only.

  • Edit Task entities
  • Edit any {project name} entiities
  • Edit own {project name} entities
  • Modify Tasks in a Sprint

The problem is that if I go to the board as a user who is not logged in I can drag tasks around to other order or swim lanes and they changes get made to the task. As far as I can tell this is an incorrect escalation of privileges. A use that can not edit tasks, should not be able to alter the swimlane or weight of an item on the board.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork burndown-3591854

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

swirt created an issue. See original summary.

jeremylichtman’s picture

jeremylichtman commented

I think there's two related issues here:

  • The ability to move the items on the board (i.e. a js load)
  • The ability for that to "stick" (i.e. the AJAX endpoint isn't checking perms)

I'll take a look today. It might make sense to add a new permission for "Can drag items on board".

jeremylichtman’s picture

jeremylichtman commented
Assigned: Unassigned » jeremylichtman
jeremylichtman’s picture

jeremylichtman commented

There actually is a permission 'reorder burndown backlog'.

I have to make a mod to ensure that is respected by the AJAX endpoint.

Will look into how we can not load the js for users that shouldn't have that permission too.

jeremylichtman’s picture

jeremylichtman commented

I've created a MR here: https://git.drupalcode.org/project/burndown/-/merge_requests/25

@swirt Do you want to test this out?

It updates the permission for the AJAX endpoint to be the correct (already existing) one, and also only adds in the reordering js library if the user meets that permission.

I think this would allow you to make the board itself viewable, and then simply not grant the 'reorder burndown backlog' perm to anonymous users.

swirt’s picture

swirt
he/him
Primary language English
commented

I can test this out later tonight. Does 're-order' mean from one status/swim lane to another, or is that about priority (dragging top to bottom)?

swirt’s picture

swirt
he/him
Primary language English
commented

Reorder the backlog sounds like adulting the weight/priority. Changing status/swimlane seems more like altering a property of the task so I expected that would be tied directly to the task edit perm..

jeremylichtman’s picture

jeremylichtman commented

It's the same endpoint for everything. The board sends over a list of which items are in each column, along with their sort order.

swirt’s picture

swirt
he/him
Primary language English
commented

I left a note in the PR but since they are not using the issue fork they are not updating this issue.

I tried the patch, but I think it needs to have services.yml update to include "- '@current_user'" because it is creating this error.

The website encountered an unexpected error. Try again later.

ArgumentCountError: Too few arguments to function Drupal\burndown\Controller\BoardController::__construct(), 2 passed in /var/www/html/web/modules/contrib/burndown/src/Controller/BoardController.php on line 65 and exactly 3 expected in Drupal\burndown\Controller\BoardController->__construct() (line 55 of modules/contrib/burndown/src/Controller/BoardController.php).
Drupal\burndown\Controller\BoardController::create() (Line: 36)
Drupal\Core\DependencyInjection\ClassResolver->getInstanceFromDefinition() (Line: 100)
Drupal\Core\Utility\CallableResolver->getCallableFromDefinition() (Line: 35)
Drupal\Core\Controller\ControllerResolver->getControllerFromDefinition() (Line: 50)
Drupal\Core\Controller\ControllerResolver->getController() (Line: 166)
jeremylichtman’s picture

jeremylichtman commented

I pushed a fix. The controller had both a __construct and create method for some reason, and they were interfering with each other.

jeremylichtman’s picture

jeremylichtman commented

@swirt once this is okay on your end, I will merge and push a tag with this and the previous fix.

swirt’s picture

swirt
he/him
Primary language English
commented

I will test this out this morning.

swirt’s picture

swirt
he/him
Primary language English
commented

I left a note on the PR. This recent approach fails with an error which is caused by the class not having a constructor. the create() is not a replacement for __construct(), it is the thing that feeds the __construct(). 

The website encountered an unexpected error. Try again later.

TypeError: Cannot use "::class" on null in Drupal\Core\Utility\CallableResolver->getCallableFromDefinition() (line 102 of core/lib/Drupal/Core/Utility/CallableResolver.php).
Drupal\Core\Controller\ControllerResolver->getControllerFromDefinition() (Line: 50)
Drupal\Core\Controller\ControllerResolver->getController() (Line: 166)
Symfony\Component\HttpKernel\HttpKernel->handleRaw() (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle() (Line: 53)
Drupal\Core\StackMiddleware\Session->handle() (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle() (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle() (Line: 32)
swirt’s picture

swirt
he/him
Primary language English
commented
Status: Active » Needs work
jeremylichtman’s picture

jeremylichtman commented

Actually, I just forgot to commit the final line of the create method (which returns the instance). [facepalm]

swirt’s picture

swirt
he/him
Primary language English
commented

I am seeing a loss of styling when I do not have permission to change order on the board. (see comment on MR)

jeremylichtman’s picture

jeremylichtman commented

I've added a new library for the unsortable boards, that only contains the css.

I think this should resolve the styling issue for the most part.

One thing that may still be a problem is opening up task details. If I recall correctly, there's some js involved, and they open as a form in a popup. We may need to build some other sort of interaction for users who don't have access.

swirt’s picture

swirt
he/him
Primary language English
commented

Visually it is working so that part is resolved. The dragging and dropping is prevented, so that is great.

As you mentioned, the controls are still on the cards though.

  1. close icon - is there and sill functions. Anon (someone without perms) should not have the perms to close.
  2. edit icon - is there but correctly gives an access denied. It would be better to not have it only show if permed to edit.
  3. send task to backlog - is present and functions. Anon should not have the perm to remove from board.
  4. ticket id - is still linked to the edit page which correctly gives access denied. Anon should not have this linked

All of these should probably be bound to the edit perms.

jeremylichtman’s picture

jeremylichtman commented

Mind adding some screen snaps for me?

I will work on this over the weekend.

swirt’s picture

swirt
he/him
Primary language English
commented

I just added a screen cap to the merge request. https://git.drupalcode.org/project/burndown/-/merge_requests/25#note_104...

jeremylichtman’s picture

jeremylichtman commented

I've added checks for permissions to the various links on the task card.

Also added a new permission "send tasks to backlog", and updated the API endpoint accordingly.

I think this should cover most scenarios, although wouldn't be surprised if something else comes up. It wasn't a use-case I had in mind 6 years ago.

swirt’s picture

swirt
he/him
Primary language English
commented

Sorry for the delay. I've been away. I tried out the latest updates to the MR using this as the patch
https://git.drupalcode.org/project/burndown/-/merge_requests/25.diff
As an anon user, I am no longer able to re-order anything on the board. However I am still able to click the "Send task to backlock" arrow icon and send see it removed from the board. It hits the API at `/burndown/api/board/send_to_backlog/{task ID}` which gives a "success response that lets it through.

I'd like to work on this, but the branch you are working on is in the origin repo and not the issue fork do I don't have perms to edit. I am going to take a try at moving your code to the issue fork.

swirt opened merge request !27

swirt’s picture

swirt
he/him
Primary language English
commented

Ok, disregard what I said about the perms not working. I think composer was hanging on to the previous version of the patch. This seems to be working.

Though I am seeing a new deprecation error that I was not seeing before applying this patch.

"Deprecated: burndown_allowed_estimate_values(): Optional parameter $entity declared before required parameter $cacheable is implicitly treated as a required parameter in /var/www/html/web/modules/contrib/burndown/burndown.module on line 458"

I'll try to get that resolved now that this PR !27 exists on the Issue fork.

swirt’s picture

swirt
he/him
Primary language English
commented
Status: Needs work » Reviewed & tested by the community

Nice work on this @jeremylichtman.
https://git.drupalcode.org/project/burndown/-/merge_requests/27 is tested and working great.

jeremylichtman’s picture

jeremylichtman commented
Status: Reviewed & tested by the community » Fixed

Released on 1.0.58.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.