The scheduler date-entry fields are still displayed on node edit forms even if the user does not have permission to use Scheduler.

In 7.x we had the code:

function scheduler_form_alter(&$form, $form_state) {
  // Load the real code only when needed. First check if this a node form and
  // that the user has permission to use Scheduler.
  if (!empty($form['#node_edit_form']) && user_access('schedule (un)publishing of nodes')) {
    // Check if scheduling has been enabled for this node type.

This has been lost in the conversion to 8.x.
New tests in testExtraFields() will be added to check for this, as it is not covered so far.

CommentFileSizeAuthor
#17 publish_and_unpublish-2651448-17.patch1.03 KBjoekers
#11 publish_and_unpublish-2651448-11.patch916 bytesjoekers
#2 publish_and_unpublish-2651448-2.patch920 bytesjoekers

Comments

jonathan1055 created an issue. See original summary.

joekers’s picture

joekers
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new publish_and_unpublish-2651448-2.patch920 bytes

We seemed to have removed the permission check when we moved the code from scheduler.edit.inc to scheduler.module.

I added the permission check back in which prevents access to the scheduler fields if the user doesn't not have permission. Because the fields are always defined on the node, I also had to bypass the field validation if the scheduler fields are required, not sure if there is a better way to do this?

pfrenssen’s picture

pfrenssen
Location Sofia
commented
Priority: Normal » Major
Issue tags: +Needs tests 
+++ b/scheduler.module
@@ -198,6 +198,15 @@ function scheduler_form_node_form_alter(&$form, FormStateInterface $form_state)
+    // Bypass validation for users who do not have permission.
+    $form['publish_on']['widget'][0]['value']['#required'] = FALSE;
+    $form['unpublish_on']['widget'][0]['value']['#required'] = FALSE;

I agree, digging so deep in the widget array doesn't feel right.

Could you check if it works to deny access on the publish/unpublish fields instead of disabling the #required state?

$form['publish_on']['#access'] = FALSE;
$form['unpublish_on']['#access'] = FALSE;

My guess is that the validators are also disabled when the entire field is disabled, but I'm not sure.

Since this is security related, it would be great to have a test for this. Raising priority to major too.

joekers’s picture

joekers
Primary language English
commented

Denying access to the fields was the first thing I tried but it didn't work so the only other thing I could think of was to set #required to FALSE.

I'll have a look to see how other modules achieve this.

I don't have any experience writing tests but I'm happy to give it a go :)

jonathan1055’s picture

jonathan1055 commented

I have added tests to cover this bug
http://cgit.drupalcode.org/scheduler/commit/?id=00d1135

[edit] ooops, sorry Joe, I did not see your comment about writing tests. I put this together and commited as I knew we needed tests. before I read the above thread in detail.

Status: Needs review » Needs work

The last submitted patch, 2: publish_and_unpublish-2651448-2.patch, failed testing.

joekers’s picture

joekers
Primary language English
commented

Cool, I didn't start writing tests for it but I'll take a look at your code to see how it's done.

  • jonathan1055 committed b11ee08 on 8.x-1.x 
    Issue #2594615, #2651448: by jonathan1055: Strengthened user-permission...

  • jonathan1055 committed 98b44cf on 8.x-1.x 
    Issue #2594615, #2651448: by jonathan1055: Removed accidental _ from...
jonathan1055’s picture

jonathan1055 commented

Oddly the commit in #9 to fix my mistake of leaving _ in the test names has not shown up in the actual 8.x code repository. Maybe I did something wrong with the merge, which show no differences, yet the two branches do have differences, the testing branch has the correct test names. Make the change again, to get back to alignment.
http://cgit.drupalcode.org/scheduler/commit/?id=e2bc5ef

joekers’s picture

joekers
Primary language English
commented
StatusFileSize
new publish_and_unpublish-2651448-11.patch916 bytes

Re-rolled the patch because of #2538002: Cannot save Scheduler permission with ( ) in machine name key

jonathan1055’s picture

jonathan1055 commented
Status: Needs work » Needs review

Thanks. Setting the status to 'needs review' to trigger the tests to be run.

Status: Needs review » Needs work

The last submitted patch, 11: publish_and_unpublish-2651448-11.patch, failed testing.

jonathan1055’s picture

jonathan1055 commented
Status: Needs work » Needs review

As before, we get an extra test pass.

Did you have any luck in answering pfrennsen's query about how to disable the validators? It does seem wrong to have to set #required to FALSE and in doing so have to hardcode the deep set of array key names.

It also surprises me that with all the customisable/configurable flexibility we now have in Drupal8 that when we don't want a user to have access to a field we have to leave it in the form but just hide it and make sure it does not throw a validation error. There must be some way to say "do not let the user anywhere near this field, just as if it was not there in the first place"

joekers’s picture

joekers
Primary language English
commented

I did take a brief look but I couldn't see anything in core. I know that if you hide a required field from the form display that it will not cause any validation errors, but I think that's only because the field is never added to the form in the first place - so that doesn't help.

I agree there must be a cleaner way to achieve this. Maybe there's a preprocess form or something so we can decide what fields we want to appear on the form? Hopefully Pieter knows a way, or I'll try asking in IRC.

jonathan1055’s picture

jonathan1055 commented

I think we should commit this fix, as it does solve a major problem. Joe, if you can add a @todo referencing this issue, and stating that the solution is not ideal but better than nothing, I will commit it. If we find a better solution later we can change it again.

joekers’s picture

joekers
Primary language English
commented
StatusFileSize
new publish_and_unpublish-2651448-17.patch1.03 KB

Cool - updated patch.

Status: Needs review » Needs work

The last submitted patch, 17: publish_and_unpublish-2651448-17.patch, failed testing.

  • jonathan1055 committed c665714 on 8.x-1.x authored by joekers 
    Issue #2651448 by joekers: Publish and Unpublish fields are shown for...
jonathan1055’s picture

jonathan1055 commented
Status: Needs work » Fixed

That's another test now passing.

2		Scheduler.Drupal\scheduler\Tests\SchedulerPermissionsTest
✓		- setUp
✓		- testUserPermissions

I just made a minor alteration in the comment. Using the text @see is apparently better as this string is parsed by the automated documentation process.

Thanks Joe, good to get this committed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.