Feature/admin access control

[simongroenewolt]

Created PR to share progress status on #2099

[I-Valchev]

hi @simongroenewolt , really liking the progress so far! Nice work!

In the permissions.yaml, I notice sometimes you use ROLE_ADMIN and ROLE_DEVELOPER, and sometimes it is just admin or developer. Is that a WIP thing, or on purpose?

[simongroenewolt]

hi @simongroenewolt , really liking the progress so far! Nice work!

In the permissions.yaml, I notice sometimes you use ROLE_ADMIN and ROLE_DEVELOPER, and sometimes it is just admin or developer. Is that a WIP thing, or on purpose?

That is work in progress, they should all become 'Symfony style' roles.

[I-Valchev]

I see. Thanks for confirming!

[simongroenewolt]

@bobdenotter @I-Valchev can you chime in on this design choice: When a function / button is not available based on permissions I have 3 options:

1. show the button, generate error when pressed (bad!?)
2. don't show the button (good!)
3. show a disabled button (good?)

I think in general option 2 is the accepted best solution -- don't show stuff you cannot use. But there can be said something for option 3.

First question: Do you agree option 2 is best for the Bolt admin to use as well? (I don't know how bolt 3 handled it)

And then a followup: I've used all sorts of testing frameworks, but Behat is not one of them. Do you have an idea how to test permissions using Behat e2e tests? I don't think Behat really 'wants' to test things like 'expect error when calling this' right? It can probably test "I don't want to see the 'delete' button next to the item in the page", but that feels incomplete. A good test should both test for the button to not be shown and for the action to trigger an error if it would have been triggered after all.

[I-Valchev]

Hi @simongroenewolt ,

I completely agree that option 2 and 3 seem much more preferable than option 1. So far, I know we have been hiding the View saved version button, as an example, for viewless ContentTypes. Having said this, I also see a benefit in keeping the Editor layout consistent by disabling buttons. Perhaps displaying a message that the user does not have the correct permission level to use that action on hover.

Personally, I slightly lean toward option 3 over 2, but equally I see why some can disagree with me. @bobdenotter do you have an opinion on this?

In terms of Behat, e2e tests with behat essentially simulate user interactions with the interface. So, like you say, a good test will be to check if a button is there or not. In terms of throwing permission refused error when hitting the URL/controller, you could do this with behat, but an alternative will be to do so with unit tests.

If a user attempts to perform an action they have no permission to, I think Bolt should handle this and display a "user-friendly error" rather than a 500. If there's a user-friendly error, then in behat you can test that you're redirected to that page, for instance. Does that clear things up a bit, @simongroenewolt ?

[bobdenotter]

@simongroenewolt With the end of the year approaching, I'd like to see if we can wrap up stuff for 4.2 and start a short round of beta-testing so we can hopefully release by the end of january or somesuch.

You know i feel that new functionality doesn't have to be totally feature-complete, as long as it's an improvement over what it was. If possible, i'd like to get your PR's merged soon, so we can wrap it and apply spit-polish for a 4.2 release. Additional features can then always go into 4.3 or beyond.

As such, what do you think is essential for what's basically a "first version of expanded access control"?

[simongroenewolt]

@bobdenotter good question, I'll have to think about that a bit more, but as a first shot:

• All routes currently under access control for ADMIN should stay (more or less) that way - this is nearly done.
• 'we' should decide on the 'style' of error to throw for inaccessible stuff - can that be handled in a general way? (I wouldn't like to add lots of special messages about inaccessible stuff)
• Decide on hide vs 'show inaccessible' and make sure this is implemented at the most used places (some less used buttons showing and throwing an error can be 'ok' I guess) -> this is still work to do.
• agree on the currently supported set of 'access controllable' items (I think his is also nearly done)

and...

• TESTS! (Or at least a couple of them as examples so they can be extended later) <-- this is the part I wanted to start on soon-ish. I think behat tests can't cover this by themselves, but if I remember correctly Ivo mentioned unittests, and while this will not really be a 'unit' to test, the setup could very well work.

More later...

[bobdenotter]

All routes currently under access control for ADMIN should stay (more or less) that way - this is nearly done.

Whoop whoop! 🎊 🎉

'we' should decide on the 'style' of error to throw for inaccessible stuff - can that be handled in a general way? (I wouldn't like to add lots of special messages about inaccessible stuff)

In my opinion this can be very simple: If the user has no access to something, redirect to the login page with a flashbag "You do not have access to that page". Explaining who, why or what can be seen as 'information leaks' to wannabe crackers.

Decide on hide vs 'show inaccessible' and make sure this is implemented at the most used places

I'm in favor of "hide". Don't show the editor for blogpostings all the things they don't have access to. It'll just confuse them. :-)

agree on the currently supported set of 'access controllable' items (I think his is also nearly done)

I think that's good enough for now. Minor changes will probably be made after we're actually using it, and bump into edge cases or inconsistencies.

TESTS! (Or at least a couple of them as examples so they can be extended later) <-- this is the part I wanted to start on soon-ish.

Yes!

[kdrwila]

Hi guys, sorry if this is not a place for this kind of question but is there any ETA on this? I really need this feature in my project.

[simongroenewolt]

hi @kdrwila my aim is to get this PR into a 'good enough' state soon (I'm planning to work on it this weekend), but I cannot promise anything about when a version will be available as part of an official bolt release.

[simongroenewolt]

I'll rebase to fix conflicts, but not now!

[I-Valchev]

@simongroenewolt on this one, I also suggest we make use of @bobdenotter 's recent yaml-migrations extension.

It allows us to update bolt/project files according to a release of bolt/core. We can use this here to make sure each project that updates to 4.2 will get a default permissions.yaml. As long as the new permissions are not a breaking change (which I don't think they will be), that might be the simplest way to insert them into existing projects.

[simongroenewolt]

Hi @bobdenotter @I-Valchev I think I've made the last changes that really needed to be made (user-edit vs. profile-edit) and decided the PR is now 'ready for review'.

All issues I'd want to have addressed before rolling this out to the 'general public' have been marked 'TODO PERMISSIONS'

I'd say the system is quite usable (after the last couple of bugfixes/decisions) for all non-enterprise uses where the biggest separation of roles will be between 'ADMIN' and 'EDITOR' and not so much else.

Note that I've introduced a role 'DEVELOPER' which can 'switch user' to other users by going to the user list and pressing the button, really convenient for testing and assessing if your security config works as expected (in the web interface at least). It isn't something to be done by non-developers, hence the role 'above' ADMIN.

Still needed: (more) docs will be nice, I'm willing to write some but prefer to get asked a couple of questions I should answer instead of trying to come up with what people would need to know.

A couple of tests would still be good to have. I really wanted to add those but got stuck trying to come up with a reasonable approach that wasn't too brittle and/or testing the obvious. (Injecting the global Config and getting the permission settings using untyped lookups doesn't really help for that, but I chose to not change the general approach too much)

Another dilemma I had - are there cases in which the system should 'shut down' the site because of a security misconfiguration? If this was my own project I'd do that, but I think the 'bolt way' is to keep the site 'live' as much as possible even if the config isn't quite right.

[bobdenotter]

Currently tests on this PR are failing. At least because of this issue: #2336

I'm really tempted to merge this one in as-is, and if there's other tiny things we'll need to fix, we'll do it in followup PR's. :-)

Really liking the work you've done on this, @simongroenewolt! Really great to have this piece of functionality!

[simongroenewolt]

Wow! Great to have it merged :-)

There still are a couple of ROLE_ADMIN checks that we probably should discuss. I'll create separate issues if needed.

[svivian]

Which version of Bolt is this in? I'm using 4.0 and there is no permissions file and the docs make no mention of roles/permissions. I'm having the same issue where the Editor role does nothing (access denied in admin area) and the Admin role does everything.

Why was this even removed in the first place?!

[I-Valchev]

hi @svivian , this is available in Bolt 5. Here's more information regarding the release: https://boltcms.io/newsitem/bolt-5-has-been-released