Add buildtask and dev URL permissions

[andrewandante]

Introduces canInit() methods on Development admin controllers, and respects canView() permission checks on BuildTasks

Issue

• ENH Add more nuanced permissions to Dev Task runner #10852

[GuySartorelli]

@andrewandante Feel free to @ me when this is ready - it looks like a great idea.

[andrewandante]

Thanks @GuySartorelli - I think this is in a good space, aside from the test. I'm having a bit of trouble turning off the allow_all_cli param, any advice?

While devving this I've noticed that it needs a companion PR /dev/graphql so I'll bundle that together as well

[GuySartorelli]

I think the problem is the environment type.
By adding this to DevelopmentAdmin::canViewAll() we can see that the Director::isDev() check is evaluating true, and the cli check is evaluating false.

Debug::dump([
    'requested' => $requestedDevBuild,
    'isDev' => Director::isDev(),
    'CLI-and-allowed' => (Director::is_cli() && $allowAllCLI),
    'admin' => Permission::check("ADMIN"),
    'all-dev' => Permission::check("ALL_DEV_ADMIN"),
]);

Looks like just setting the environment variable isn't enough to flip the switch, so we need to call setEnvironment() on the kernel

DevelopmentAdmin::config()->set('allow_all_cli', false);
$kernel = Injector::inst()->get(Kernel::class);
$env = $kernel->getEnvironment();
$kernel->setEnvironment(Kernel::LIVE);
try {
    // ..... do all your test stuff
} finally {
    $kernel->setEnvironment($env);
}

[michalkleiner]

Good job, I like the idea 👍

[andrewandante]

Thanks team, updated the wording based on your recommendations 👍

[GuySartorelli]

From

To

Even then the language is a little bit inconsistent - we should probably swap "see" to "view", "run" to "execute", and "access" to "view and execute" (or change execute to run, either way so long as it's consistent)

[GuySartorelli]

Looking good! Do you have any thoughts about "see" vs "view", and "run" vs "execute" as per #10979 (review)? I think it might be good to use consistent language there but It's not a deal breaker either way IMO.

[andrewandante]

Do you have any thoughts about "see" vs "view", and "run" vs "execute

Yeah it should definitely be cleaned up. I think "view" and "execute" feel the most correct. Will consolidate and squash

[andrewandante]

With the adjustment to priority 80 in the GraphQL module PR

[GuySartorelli]

I was just running this locally as a last double check before merging and I noticed a big problem in live mode.

As an admin user, when I go to any of the /dev/* endpoints, I still get the "Confirm potentially dangerous action" screen - which is good

However, as a non-admin user who has been granted access via the new permissions, I do not get that screen. That screen should always appear in live mode for users who have permission to access the endpoint.

[andrewandante]

Interesting - it looks like the way this works is that it only triggers the confirmation if the user has a permission that it thinks is behind the URL - if I'm understanding correctly, this means this list: https://github.com/silverstripe/silverstripe-framework/blob/5/_config/requestprocessors.yml#L100-L101

I could add the list of new permissions to this but it feels a bit flaky? It means that each time a new task/permission or whatever gets added, that would need to also be added to the config - and would be easy to forget. The alternative would be to add some sort of wildcard check, rather than "just" ADMIN

I'm not certain on the best approach to this. It feels like writing a /dev middleware is overkill but that would allow me to overload hasAccess() with a DevelopmentAdmin::canInit() check instead? Do you have a feeling for the approach?

[GuySartorelli]

We wouldn't be able to set the AffectedPermissions in a way that's useful because it's a blanket middleware. If we added all the new permissions, then it would always ask for confirmation if someone only had access to /dev/config but was trying to access /dev/build for example.

Possible solutions:

1. Subclass URLSpecialsMiddleware or PermissionAwareConfirmationMiddleware (or even just add a new simpler middleware since the permission checks are more complex than those middleware classes are intended to handle) specifically for the /dev/* endpoints
2. Add a new property to PermissionAwareConfirmationMiddleware which gives it a callback for checking permissions. It can then go "if the member has the permission code, or if the callback returns true, ask for confirmation"
3. Have a distinct middleware singleton defined for each /dev/* endpoint, with the relevant permissions set (easy to do but ends up with config spaghetti and easy to forget for new endpoints)
4. Add the permission codes to a private static on the controller, and add a way to tell PermissionAwareConfirmationMiddleware to check the private static of whatever controller will eventually handle the request, and check for those permissions.
5. Uhhhh...?? Maybe move the confirmation thing out of middleware into the controllers, or into some service that can be called from the controllers? I don't like that idea though.

In either of 1 or 2 you're gonna need a way to call the relevant canInit() method from within the middleware - and it can't just be the DevelopmentAdmin::canInit(), it needs to be the canInit() for the relevant controller, or else you'll still get into the scenario I described above where someone with one permission gets asked for confirmation for an endpoint they don't have permission to access.

Of all of these I think I like 4 the most, as it feels like the least spaghetti solution. It also has the added bonus of making the permission codes for a given /dev/* endpoint configurable for mad scientists.

In some of these scenarios, if canInit() is overridden in a project to include some check that isn't directly permission-based, they may get into a position where they again have users not being asked for confirmation. I think we can just call that out of scope though.

[andrewandante]

Alright, after a lot of experimentation, I've added a specific DevAdmin middleware. Determining the final controller was too difficult inside the existing middleware implementation so this was cleanest I think, and should "absorb" additional /dev endpoints as they are added.

[GuySartorelli]

Looking good - but there's still a few more things to tidy up

Problem running some tasks

When I run a dev/task with someone who does have permissions for /dev/tasks, I get the output from running the task, but I also get a "you don't have access to this page" message:

That doesn't happen with all tasks - I only tried the "Deletes all temporary test databases" task (which gives that output) and the "Login Session Garbage Collection Task" (which works correctly and gives the normal output).

If might just be that the "Deletes all temporary test databases" task doesn't actually work for arbitrary users, and needs a canView() applied.

/dev/build not protected

/dev/build is listed as an explicit exception to the new middleware in dev_urls-confirmation-exceptions config, and is instead protected by the url_specials-middleware config block. We probably want to move it out of the URL specials middleware and into the new middleware.

Currently I get no confirmation screen for /dev/build

sub-urls not protected

Without this PR, running a BuildTask (e.g. /dev/task/CleanupTestDatabasesTask) or going to /dev/graphql/build prompts a confirmation - but with the PR, there's no confirmation.

[andrewandante]

Thanks again for your thorough testing!

For the first point, the TestDatabases task has a specific permissions check for ADMIN inside the run method, which is what throws that. Refactoring to a canView() is probably the way to go for that specific scenario.

For the dev/build, I think moving it across will largely work, though given the special edge-cases around it I'll likely have to explicitly code those into the middleware. Might be best anyway!

Last point - good point, will check in on that. Probably need to find some way to check permissions on the parent. Phew!

[andrewandante]

Hmm. So I'm working on the dev/build endpoint, and for a reason I haven't tracked down yet, it wants to move me to stage=Stage - which fails inside VersionedHTTPMiddleware::checkPermissions() (because of Versioned::can_choose_site_stage()). This means if the user doesn't also have CMS permissions, they can't run the dev/build. I can fix it by adding CAN_DEV_BUILD to Versioned::non_live_permissions but that doesn't feel right? Maybe I'm overthinking it, but at that point the $request->getURL() is not /dev/build so I can't use the same logic we are using in other places. Will keep digging but if there's something immediately obvious I'd love to hear it 😅

Edit: it's coming from VersionedStateExtension::updateLink() - I guess the ReadingMode is stage when you hit a /dev URL?
Edit2: yep, inside DevelopmentAdmin::init(). In that case I think I do need to add CAN_DEV_BUILD to the permission lists for Versioned.

[andrewandante]

Alright I think with all that, I've resolved those three concerns

[GuySartorelli]

Awesome, nice work! I'll take a proper look on Monday and put it through its paces.

[GuySartorelli]

This is the only change needed, at a glance.

[GuySartorelli]

It's veeeeery nearly there. All of the permissions are working great, now the only problem is the one from #10979 (comment) where I still see "Running Task Deletes all temporary test databases".

The task doesn't actually run because the run() method exits out early due to the condition in that method, but the runner itself it still calling that method which is a problem.
The task is also still visible in the /dev/tasks list, which it shouldn't be. I think? I keep getting confused about when canview is meant to work in this scenario - and I think that confusion is also a bad thing. Probably we should just make it so that the canview checks in tasks are always respected, because if I'm getting confused then other people will also get confused.

[andrewandante]

Ah yep, I see the issue there. The "can view all tasks" permission means that you can see the task, but the canView() check on the task itself says you can't.

My instinct here is to actually update the canView() check for this task to include the parent permission, though I don't know if that's the intention of the check for the task in the first place. Headache 🤔

Actually it kind of feels like the init() on each BuildTask should be checking canView(), but that feels like it's going to introduce an API break...

[GuySartorelli]

Actually it kind of feels like the init() on each BuildTask should be checking canView(), but that feels like it's going to introduce an API break...

There is no BuildTask::init().... not sure what you mean?

I think the getTasks() and runTask() methods in TaskRunner should do this before returning/running a task:

if ($task->hasMethod('canView') && !$task->canView()) {
    // skip, disallow, can't view it can't run it, not allowed.
}

That logic could probably be put into the taskEnabled method instead, which is already called from getTasks() and should probably be called in runTask() instead of calling $inst->isEnabled() directly.

This should be fine in terms of backwards compatibility, because a canView() method on a BuildTask hasn't meant anything up until now. We're free to start calling it now and acting on its response.

[andrewandante]

There is no BuildTask::init().... not sure what you mean?

Exactly, an API break 😉

Yeah I think with what you've proposed it means the task overrides the global permission, which is probably more intuitive. I might have to update one of the descriptions to reflect that, but it does make sense.

[GuySartorelli]

Oh, you were meaning you proposed introducing a new method? It's not an API break to add new methods so long as those methods aren't abstract. But in this case I don't think we need a new init method.

[andrewandante]

Test failures seem unrelated to my changes

[GuySartorelli]

Great, thanks!

MariaDB failures are unrelated - see silverstripe/silverstripe-graphql#561 and #11046