SwitchShortcutSet form does not set access on machine name element

Thanks for reporting, next step would be to add a test showing the issue.

I disagree with the approach taken in #2. That would only hide the element visually. What we really need to do is set the #access on the element so it's removed from the form entirely for those who shouldn't have permission to see it. In fact, this is sort of a security gray-area. Users have the ability to see something that they shouldn't. Fortunately, they can't do anything with it. Entering text into the element does nothing. Otherwise I'd be contacting the security team. I considered adding the Security Improvements tag, but decided not to.

I added test assertions in addition to changing the proposed fix. I also updated the issue summary.

So ran the test-only run

$this->assertSession()->elementNotExists('xpath', '//input[@id="edit-set-new"]');
    $this->assertSession()->elementNotExists('xpath', '//input[@id="edit-label"]');

Based on the comment in the test I'd expect these to fail but they pass, doesn't fail till it gets to

$this->assertSession()->elementNotExists('xpath', '//input[@id="edit-id"]');

So if we are still showing these items 1. Think we need screenshots so people can see the change. But comments should also be updated.

I'm happy to add screenshots so this issue is clearer. It's done.

Based on the comment in the test I'd expect these to fail but they pass...

No, they're passing correctly. There's no issue with those elements. In other words, they're being hidden appropriately. But there's no test coverage for it. I could have left those assertions out because they have nothing to do with this issue. But they were easy to add along with the assertion for this issue. Just consider it to be bonus coverage. See the before image I've added. There is no "New set" radio button or "Title" text field.

But comments should also be updated.

I assume you're talking about this comment:

    // Verify that users without the "administer shortcuts" permission have
    // access to the set selection radios, but not the new shortcut form
    // elements.

Users without that permission shouldn't be able to see the "New set" radio, "Title" text field, or "Machine-readable Name" field. These are the elements related to adding a new shortcut set. Would you prefer alternative text that explains it in a different way?

Suppose this works. Shortcuts UI is so weird, probably why it's up to be removed too. But this does address the issue described in the summary.

Can we have a test only PR to show the fail?

No longer needed. In the MR there’s a test only run feature

Thanks for pointing that. RTBC++

I think this functionality is very rarely used which is why nobody has really noticed it before! Backported down to 10.6.x as an eligible bug fix.

Committed and pushed 508e5ebfd25 to main and ad20c0b53e7 to 11.x and b62d87bedd6 to 11.3.x and ec7d387dd4b to 10.6.x. Thanks!