Add a "negate" option to the visibility condition for roles.

[stpaultim]

Description of the need

I would like to be able to set a visibility rule for conditions in which a user does not have a specific role. You can set this kind of role in Rules with the "negate" option.

Proposed solution

Add a negate option to visibility conditions.

Additional information

Something like this (this is a "condition" from the rules module):

Found out that we have the "negate" option on UID and NID visibility condition. Would it be difficult to add it to Roles?

This also came up in the forum (OK, I posted it in both places). https://forum.backdropcms.org/forum/visibility-rule-user-does-not-have-role

---

Translation update:

New strings were introduced: User does not have the role @role, User does not have one of the following roles: @role

Strings changed: Only the checked roles will be granted access. becomes Only the checked roles will be granted access (or denied access, if "Reverse" is checked).

[laryn]

@stpaultim This is a guess, but I noticed that the EntityIDLayoutAccess extends LayoutAccessNegatable:

• https://github.com/backdrop/backdrop/blob/ffa60e9582252d459b3e4e564b9c0139fdba00cd/core/modules/layout/plugins/access/entity_id_layout_access.inc#L6

Contrast with the UserRoleLayoutAccess which extends LayoutAccess:

• https://github.com/backdrop/backdrop/blob/1.x/core/modules/layout/plugins/access/user_role_layout_access.inc#L6

Maybe it will be as easy as changing the second one to extend LayoutAccessNegatable?

[stpaultim]

@docwilmot - Can you say anything about the feasibility of this (and/or how difficult it would be)?

[docwilmot]

Yes @laryn is right. We needed to use LayoutAccessNegatable() for the negate checkbox to appear, then similar code as in EntityIDLayoutAccess() to save negated values. It should be reasonably straightforward, and shouldn't be an API change IMO.

[stpaultim]

Sarvasri and I were just working on a demo site and ran into a situation in which we created a new layout for a specific admin page intended for authenticated users with specific permissions. BUT, this layout does not work well for the admin theme.

We would have loved to make a visibility condition on the layout that made it available to anyone except admins. We spent about an hour looking for the best alternative.

Having this option would have been really useful today - and we think that having the option to "negate" a visibility condition would be useful in many other situations as well.

Are there any contrib modules that extend options for visibility conditions?

[laryn]

I would support it.

[philsward]

I would be curious if the negate is added on a case-by-case basis or if it something that can always be available to anything using the visibility api but allowed to be hidden or disabled if a developer chooses so.

I would prefer it is something that is always available. I negate conditions all the time on my Drupal sites in panels.

If it isn't available for roles, it makes me wonder if it's a case-by-case basis.

I know @laryn pointed out the technical, but I'm not sure if it addresses whether it should have been there by default or not.

[bugfolder]

PR (with test added) submitted. H/T @laryn, @docwilmot for the approach to use.

@philsward wrote:

I would be curious if the negate is added on a case-by-case basis or if it something that can always be available to anything using the visibility api but allowed to be hidden or disabled if a developer chooses so.

It's case-by-case. It's pretty straightforward to add negation to any layout access condition that doesn't have that functionality by changing the plugin base class and tweaking it (as pointed out above) and the change is backward-compatible (existing visibility conditions still work, since the default state of the added "Reverse" checkbox is unchecked).

[philsward]

It's case-by-case

Regardless of how easy it is to implement, wouldn't it make more sense if this was bundled with the visibility condition function so anything that calls on visibility conditions always gets the negate option by default?

It seems like there's a lot of room for error where it is either forgotten or it's unknown it is needed, by leaving it a case-by-case. Bundling it would be one less thing people have to remember they have to address.

[bugfolder]

wouldn't it make more sense if this was bundled with the visibility condition function so anything that calls on visibility conditions always gets the negate option by default?

Some of the Layout access visibility conditions implement their own settings that provide the same effect but with text that is clearer than just a "Reverse (NOT)" checkbox. For example, here's the settings for the "Home page" condition:

Here's what it would look like using the "bundled" approach:

I think the former is clearer and more user-friendly.

For some of the conditions, having it negatable would be superfluous. For example, for the "Node: Type" condition, here's the settings:

You don't need a "Reverse (NOT)" option for this one because you can achieve the desired effect by flipping which checkboxes are checked.

[bugfolder]

It seems like there's a lot of room for error where it is either forgotten or it's unknown it is needed, by leaving it a case-by-case. Bundling it would be one less thing people have to remember they have to address.

Could be, but I suspect that any developer who is creating a new LayoutAccess condition would probably take a look at the existing ones to see how these things are typically done, would notice that several of them inherit from LayoutNegatableAccess, and then would decide whether that's a good fit for the condition that they're adding.

[philsward]

For some of the conditions, having it negatable would be superfluous.

Right, which is why the suggestion to "disable it" was presented.

To me, the functionality should always be there in the most basic form. From there, it is overridden to say something friendlier or be disabled completely. It could include the option of checkbox or an array for radios with the boolean as the default.

Point is, the functionality doesn't exist for people who need it unless it has been expressly added.

If it did exist from the start, the functionality is always available. It may not be user friendly which could always be changed later to match the use-case, but that said, it would probably be changed up front to be user friendly from the start because it's in the developers face when testing. Omitting it from the start means it's out of sight out of mind and in the case of this issue at hand, easily forgotten about.

I would rather have the option that is ugly but works, than to have no option at all and desperately need it.

I suspect that any developer who is creating a new LayoutAccess condition would probably take a look at the existing ones to see how these things are typically done, would notice that several of them inherit from LayoutNegatableAccess

Oh, like this issue here for roles? 😜

[bugfolder]

Oh, like this issue here for roles? 😜

I was kinda waiting for that. The use case for Roles negation isn't quite so obvious from the get-go. In most cases for Roles, like Node Types, you can choose which roles to get the condition just by selecting which roles' checkboxes are checked. But because of the way that role permissions are additive, checkboxes don't allow you to deny visibility to a higher-permission role while permitting the lower-permission authenticated role. So, it (probably) wasn't obvious at the time that condition was set up that negatable offered anything more than what checkboxes allowed. (I'm guessing; I wasn't there for the original code.)

(Also, I suspect that the author of UserRoleLayoutAccess was fully aware of the existence and functionality of LayoutAccessNegatable, just didn't see the need at that time (7 years ago, according to git blame). Now we do, hence this issue.)

At any rate, there is a method to enable or disable it in code, by choice of what base class the condition inherits from. So the in-code disable option is already present.

It sounds like maybe you're suggesting the choice to enable/disable it should be in the UI, not in code? Or are you suggesting something different?

At any rate, at this point it might be more efficient to simply re-examine the 4 existing conditions that inherit from LayoutAccess and ask whether they would benefit from inheriting from LayoutAccessNegatable.

[philsward]

It sounds like maybe you're suggesting the choice to enable/disable it should be in the UI, not in code?

In code

At any rate, at this point it might be more efficient to simply re-examine the 4 existing conditions that inherit from LayoutAccess and ask whether they would benefit from inheriting from LayoutAccessNegatable.

Possibly.

I'm just asking questions to think outside the box to see if there is a better, more bullet-proof way of dealing with the negate option.

Leaving it up-to a case-by-case basis means the functionality is only there if someone remembers to add it.

Enforcing it by default with an option to disable it in code means the developer has to actually think about the possible use-cases and whether it's a good idea to disable it or not.

[bugfolder]

It sounds like maybe you're suggesting the choice to enable/disable it should be in the UI, not in code?

In code

Cool. Then that option exists now; the choice to enable/disable is made at the time that one writes the class declaration for the condition.

Leaving it up-to a case-by-case basis means the functionality is only there if someone remembers to add it.

The developer creating an access condition needs to make an affirmative choice of which base class to inherit from. If they choose LayoutAccess, it's not negatable. If they choose LayoutAccessNegatable, then it's negatable. If they "don't remember" to make a choice, their code won't even run.

[philsward]

For some of the conditions, having it negatable would be superfluous. For example, for the "Node: Type" condition

@bugfolder on a quick side note, I can think of a use-case where it actually makes a lot of sense to negate it.

I don't think it's fair for a developer to assume (not you specifically, just in general) there is no foreseeable use-case so let's not add it. Better to have it and not need it than need it and not have it.

[bugfolder]

I can think of a use-case where it actually makes a lot of sense to negate it [in the Node:Type condition].

That use-case would make an excellent follow-up issue (and one that could probably be quickly addressed).

I don't think it's fair for a developer to assume...there is no foreseeable use-case so let's not add it. Better to have it and not need it than need it and not have it.

In the case of the Role ID access condition, the original developer had the negatable option, but chose not to use it. Now that the use case is clear, it's easy to make the switch (hence the brevity of the attached PR).

[stpaultim]

I've tested this PR for a simple scenario and my slightly more complicated specific scenario and found that it works as expected. I will mark this works for me, because it does fix the issue raised in this issue.

I appreciate the argument @philsward is making about a possibly better solution that would reduce the likelihood of having to make this kind of fix in the future. BUT, it's not clear to me if this is actually possible, necessary or desirable.

I believe that the best course of action here is to proceed with this PR as is, with a note about Phil's concern and let our Core Committers weigh in on whether or not this is the right approach. If anyone else has any specific ideas for how to do this better, they can weigh in as well.

[hosef]

I took a look at the PR and the changes all look fine to me.

One change that I would prefer if it's not too hard would be to include the test as part of testRoleSelection() instead of making a new test method.

[bugfolder]

One change that I would prefer if it's not too hard would be to include the test as part of testRoleSelection() instead of making a new test method.

I'm not sure why, but if I move the test code from testRoleNegationSelection() into (but after) the previous test in testRoleSelection(), things start failing at the new test (and I can't figure out why). So at this point, I'd be inclined to leave it as-is (i.e., "it's too hard").

It looks like it should be easy. But isn't. If someone can show how to do this, I'd welcome the suggested change.

[docwilmot]

If at the end of the current test you go back and edit the Visibility Condition (add negate)

    // Allow authenticated users to see the layout only.
    $edit = array(
      'roles[authenticated]' => TRUE,
      'negate' => TRUE,
    );

Then admin should 200, anon should 200 and authenticated should 404.

[bugfolder]

Yeah, that's what I was trying in principle, but you need more than that. You need to backdropPost() something, and that's what I was getting hung up on.

I'd started with just creating a new layout, exactly like what's in the previous test, like this:

    // Test negation of role-based selection.

    // Create another layout at another custom path.
    $layout_name = strtolower($this->randomName());
    $layout_path = $layout_name;
    $edit = array(
      'name' => $layout_name,
      'title' => $layout_name,
      'path' => $layout_path,
      'layout_template' => 'moscone_flipped',
    );

    // This adds a condition, not saving the layout yet.
    $this->backdropPost('admin/structure/layouts/add', $edit, t('Add visibility condition'));

But that backdropPost() is the first thing that failed.

[docwilmot]

It should be something like (at the end of the current test):

    // Go back and configure the VC to negate the role selection set earlier.
    $edit = array(
      'roles[authenticated]' => TRUE,
      'negate' => TRUE,
    );
    $this->backdropPost('admin/structure/layouts/manage/' . $layout_name . '/condition/edit/layout/0', $edit, t('Save visibility condition'));

In case you havent figured, generally our tests simulate a user navigating Backdrop without JS enabled. So if you want to know which path to backdropPost to, disable JS in your browser and only then click the Visibility condition's "Configure" button/link, and that will take you to that path without AJAX popping up its helpful dialogs.

[bugfolder]

Well, that code still fails to set the visibility roles or the negate checkbox. But the point about disabling JS is great, thanks! (I learn something new.) I'll play with this for a while.

[bugfolder]

Hmm, still no luck. Adding this code to the end of the previous test:

    $edit = array(
      'roles[authenticated]' => FALSE,
      'roles[anonymous]' => TRUE,
      'negate' => TRUE,
    );
    $this->backdropPost('admin/structure/layouts/manage/' . $layout_name . '/condition/edit/layout/0', $edit, t('Save visibility condition'));

And while the test successfully finds HTML at that path, it fails to set any of the changed fields, and fails to find the "Save visiblity condition" button.

[docwilmot]

Did you log back in the admin user? Notice there is a logout at the end of that test in order to test anon access. You have to add a $this->backdropLogin($admin_user); before the code block I posted above.

[bugfolder]

Heh, I just figured that out, then saw your note. Yup, that was it, and revised test now working. Cleaning up, then will resubmit PR. Thanks for all your help!

[bugfolder]

PR updated. Back to "needs code review", I guess. Not putting it back to "needs testing" because the only thing that changed is the automated test.

[klonos]

In case you havent figured, generally our tests simulate a user navigating Backdrop without JS enabled.

...the point about disabling JS is great, thanks! (I learn something new.)

Thanks for that @docwilmot ...it of course makes total sense (we don't have JS testing in D7/Backdrop), but it never happened to cross my mind either. I will be looking at tests with a different set of eyes now.

[quicksketch]

I gave this a quick look in the UI and code. The form looks a little weird without a label separating the checkbox for Negate, but I trust the opinion of you folks. If someone else feels strongly about the presentation of this checkbox (in this and other Layout condition plugins), let's handle that in a separate issue.

Thanks @bugfolder, @docwilmot, @stpaultim, @philsward, @laryn, @hosef, and @klonos! I merged https://github.com/backdrop/backdrop/pull/3857 into 1.x for 1.21.0!