Skip to content

[5.2] Fix Permissions for Manually Running Scheduled Tasks#36719

Merged
Hackwar merged 16 commits intojoomla:5.2-devfrom
ditsuke:fix-disabled-run-task-btn-on-no-auth-36677
Jan 18, 2025
Merged

[5.2] Fix Permissions for Manually Running Scheduled Tasks#36719
Hackwar merged 16 commits intojoomla:5.2-devfrom
ditsuke:fix-disabled-run-task-btn-on-no-auth-36677

Conversation

@ditsuke
Copy link
Contributor

@ditsuke ditsuke commented Jan 17, 2022

Pull Request for Issue #36677.

Summary of Changes

  • Explicitly disables the "run task" button on insufficient authorization.
  • Adds tooltip to give context on disabling the button.
  • Change default permission level for running tasks/grant access to creator.
  • Prevent manual runs from being publicly accessible regardless of permission levels. Ref: #36453#issuecomment-1001793962 / @PhilETaylor

Testing Instructions

  • Create a new task with an administrator (not superuser) account.
  • Still as administrator, try to run it from the Scheduled tasks manager.
  • Try to run other tasks created by the super user.
  • Login as super user.
  • Check that you can run all tasks.
  • Thrash some task.
  • Check the trashed tasks.

Actual result BEFORE applying this Pull Request

As administrator (not superuser), there is no indication that the user is not authorized to run the task. The button is usable.
In all cases there is an error message.
Only a super user can run tasks.

Expected result AFTER applying this Pull Request

As administrator (not superuser):

  • The button is enabled for tasks created by yourself and you can run these tasks.
  • The button is disabled for tasks which were created by the super user (.e.g those from a new installation). Reason for being disabled is available through a tooltip on hover.

As super user buttons of all tasks are enabled, and you can run these tasks.

When a task is trashed, the button is disabled, and a tool tip shows the reason.

Documentation Changes Required

N/A

- Explicitly disables the "run task" button on
  insufficient authorization.
- Adds tooltip to give context on disabling.
@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-4.1-dev labels Jan 17, 2022
@brianteeman
Copy link
Contributor

How can it make sense for a user to be able to create a new task and setup lazy loading etc but not be able to run the task directly

@ditsuke
Copy link
Contributor Author

ditsuke commented Jan 17, 2022

How can it make sense for a user to be able to create a new task and setup lazy loading etc but not be able to run the task directly

Definitely, now that I think of it the creator should be able to run them manually as a default. I'll work on enabling that behavior before opening this PR.

@brianteeman
Copy link
Contributor

brianteeman commented Jan 17, 2022

Possibly need a new permission for that "Edit own"

Task creators should now be able to run them even if they do not have
the `core.testrun` permission. The current permission checks should be
reviewed.
*
* @since __DEPLOY_VERSION__
*/
public static function isAuthorizedToRun(object $taskRecord, User $user): bool
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brianteeman what do you think about this check?

We need this field in the authorization check. Begs the question, should
we just get all fields?
@HLeithner HLeithner changed the base branch from 4.1-dev to 4.2-dev June 27, 2022 13:06
@HLeithner
Copy link
Member

This pull request has automatically rebased to 4.2-dev.

@HLeithner HLeithner changed the base branch from 4.2-dev to 4.3-dev May 2, 2023 16:30
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 4.3-dev.

@HLeithner HLeithner changed the base branch from 4.3-dev to 4.4-dev September 30, 2023 22:45
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 4.4-dev.

@HLeithner HLeithner changed the title [4.1] Fix Permissions for Manually Running Scheduled Tasks [4.4] Fix Permissions for Manually Running Scheduled Tasks Apr 24, 2024
@HLeithner HLeithner changed the base branch from 4.4-dev to 5.2-dev November 15, 2024 13:22
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 5.2-dev.

@HLeithner HLeithner changed the title [4.4] Fix Permissions for Manually Running Scheduled Tasks [5.2] Fix Permissions for Manually Running Scheduled Tasks Nov 15, 2024
@richard67
Copy link
Member

I was able to resolve the merge conflicts, which were mainly caused by PRs #40216 and #42746 .

It needs to be tested if this PR here and #42746 still work.

@richard67 richard67 marked this pull request as ready for review January 18, 2025 12:51
@richard67
Copy link
Member

I have tested this item ✅ successfully on eb813f6


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36719.

@dautrich
Copy link

I have tested this item 🔴 unsuccessfully on eb813f6


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36719.

@dautrich
Copy link

I've tested on J5.2.3 with a user belonging to user group "Administrator".

  • Without the patch applied, I was able to click on an existing task, but the task showed an error.
  • With the patch applied, the task execution button of the existing task were "greyed" out and showed a tooltip "Not authorized".
    grafik
  • But with the patch applied, I was able to create additional tasks and run them:
    grafik

@richard67
Copy link
Member

richard67 commented Jan 18, 2025

@dautrich Of course you shall be able to run your own tasks as user in the administrator group. And as administrator you shall also be able to create tasks. But the ones created by the super user are disabled. To me it seems it is workings as intended.

@richard67
Copy link
Member

P.S. Without the PR, one error is that you can not run tasks which you have created when you are an administrator. This is also fixed by this PR, that’s why you can create tasks and run them as administrator.

@dautrich
Copy link

Then the testing instructions seem to be misleading. It says: "Create a new task with an admin (not superuser) account." And that's what I did.

@richard67
Copy link
Member

Hmm I see the testing instructions are not precise.

@richard67
Copy link
Member

@dautrich I've updated the testing instructions. Thanks for the hint.

@richard67
Copy link
Member

P.S. I've updated again testing instructions and added a test for trashed tasks.

@exlemor
Copy link

exlemor commented Jan 18, 2025

I have tested this item ✅ successfully on eb813f6

I have tested this successfully.

(thanks for the updated testing instructions Richard!)

( Cool PR btw, thanks to all that made it possible ).


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36719.

@dautrich
Copy link

I have tested this item ✅ successfully on eb813f6


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36719.

@richard67
Copy link
Member

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36719.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jan 18, 2025
@richard67
Copy link
Member

Thanks for testing.

@Hackwar Hackwar enabled auto-merge (squash) January 18, 2025 21:14
@Hackwar Hackwar added this to the Joomla! 5.2.4 milestone Jan 18, 2025
@Hackwar
Copy link
Member

Hackwar commented Jan 18, 2025

Thank you!

@Hackwar Hackwar merged commit 0bb707c into joomla:5.2-dev Jan 18, 2025
3 of 4 checks passed
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jan 18, 2025
Kostelano added a commit to JPathRu/localisation that referenced this pull request Feb 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Language Change This is for Translators

Projects

None yet

Development

Successfully merging this pull request may close these issues.