I just found out that a user who has only "Create translation jobs", "Submit translation jobs", and "Accept and reject translation jobs" permissions, but not the "Administer translation management" one, can see the Job "Delete" button, but not actually delete anything. It seems that the deletion operation itself requires the administer permission, but the buttons on the form (and in the bulk operations dropdown) are shown either way.

Comments

blueminds’s picture

Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new12.34 KB

Added #access to the delete button at the job checkout page.

About the VBO action - not sure here. I replaced the generic VBO delete action with rules action. But we are not able to define that a specific rules action should consider "administer tmgmt" permissions. So for the moment the Delete Job action is listed, it even will say that all deleted successfully, but it won't for a user without administer tmgmt permissions. This is completely wrong UX, but at least jobs will not get deleted by someone that is not supposed to delete them.

Anyway, any suggestions how to make the UX better are welcome.

berdir’s picture

Ok, so views has built-in entity access, have a look at _views_bulk_operations_entity_access().

There it checks the access mask of an action, which can control what of view/edit/delete is required to execute it.

The problem is that the rules integration (ViewsBulkOperationsRulesComponent) has only view hardcoded, and a custom access() method, that in turn only supports rules_action()->access().

Which leads to RulesAbstractPlugin::access(), which apparently supports an access callback, I'm not exactly sure if $info there is what you define in hook_rules_action_info(), but it might be :)

Worth a try...

blueminds’s picture

Yes, that worked.

berdir’s picture

Assigned: Unassigned » miro_dietiker

Yes, confirmed that this is working well.

Not sure how far we want to go here.

a) Should we apply this pattern to the other default rules too? You can view the overview without having permissions for submitting jobs, so the other actions should be hidden too then.
b) We also have default rules for tmgmt_local, which currently expose a custom permission for access control, but that's ugly. We could clean this up by adding access callbacks there too. What's sad is that there's no access arguments option, so we have to add a callback for every different permission check.
c) Test coverage. I don't think we have much test coverage right now, probably a bit for the local translator but I guess nothing for the overview. I think at least doing a permission check for the existence would be useful here.

Miro?

berdir’s picture

Title: Job deletion not working without "Administer translation management" permission » Add access callbacks to all rules actions
Status: Needs review » Needs work

Ok, we discussed this. Let's do this:

1. Add an access callback for checkout and cancel to check for entity access submit, can be the same function I think.
2. Do the same for the local task rules/actions, remove the permission flag from the default rules.
3. Add some basic access tests. Visit the overview with all access combinations verify what operations are visible ( only view => no operations, submit => checkout and cancel, => administer (only that permission) => all). Make sure to always assert for visible and not visible.

Don't add tests for actually executing those operations, unless we already have them in the local task tests, then update.

miro_dietiker’s picture

Assigned: miro_dietiker » Unassigned

:-)

blueminds’s picture

Status: Needs work » Needs review
StatusFileSize
new21.33 KB
new9.35 KB

Access callback for rules implemented. One fail is expected - for some reason user with "provide translation services" does not get the right to assign task to him. In case of uid1 it works so it has to do with permissions, but was not able to find what. Any ideas?

Status: Needs review » Needs work

The last submitted patch, 7: 1884108-delete_action_permission-3.patch, failed testing.

berdir’s picture

Status: Needs work » Needs review
StatusFileSize
new22.27 KB
new3.01 KB

Ok, this took me a while.

The trick is to not define our own access callback, but alter the generated component action for our default component, so that we can circumvent the default rules access logic completely. This is also the only option that allows us to differentiate between the assign to me and assign component.

blueminds’s picture

Status: Needs review » Reviewed & tested by the community

looks good

berdir’s picture

Ah, actually, this allows us to solve one of the @todo's in the test. This overlaps a bit with the access test coverage but whatever ;)

berdir’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.