Problem

  1. Install Drupal with Minimal profile.
  2. Create a new user role.
  3. Go to /admin/config/people/accounts
  4. Bogus default value:

    user-admin-role-setting.png

Cause

  1. The default value in user.settings is admin_role: ''
  2. user_admin_settings() contains:
      $roles = user_role_names();
      unset($roles[DRUPAL_ANONYMOUS_RID]);
      unset($roles[DRUPAL_AUTHENTICATED_RID]);
      $roles[0] = t('disabled');
    
      $form['admin_role']['user_admin_role'] = array(
        '#type' => 'select',
        '#title' => t('Administrator role'),
        '#default_value' => $config->get('admin_role'),
        '#options' => $roles,
    
  3. The default value is 0 (integer), whereas the actual default value is '' (empty string).

Proposed solution

  1. Replace this:
      $roles[0] = t('disabled');
    

    With this:

        '#empty_value' => '',
        '#empty_option' => t('- Disabled -'),
    
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Status: Active » Needs review
FileSize
1.01 KB

Attached patch fixes the bug.

andymartha’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
41.06 KB
47.81 KB

When pursuing these steps on Drupal 8.x-dev downloaded on March 6, 2013:
Install Drupal with Minimal profile.
Create a new user role.
Go to /admin/config/people/accounts

The default admin role is verified as a random role, often the newest created role. See attached "before" screenshot.
After applying patch drupal8.user-admin-role-default-value.1.patch in #1 from Sun, the default admin role is "none". See attached "after" screenshot.

Dries’s picture

Status: Reviewed & tested by the community » Needs review

I committed it to 8.x.

Do we need tests for this?

sun’s picture

Issue tags: +Needs tests
FileSize
3.73 KB

Yeah, since such a bogus default value - if overlooked/missed - could have a pretty negative security impact, we should add tests.

Apparently, there's no dedicated test for the admin role feature already, so I've created one and moved the only test into it.

Status: Needs review » Needs work
Issue tags: -Needs tests, -Sunrise Sanity Cruise

The last submitted patch, drupal8.user-admin-role-test.4.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
Aleksey Zubko’s picture

benjifisher’s picture

pplantinga’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests, +Sunrise Sanity Cruise

The last submitted patch, drupal8.user-admin-role-test.4.patch, failed testing.

pplantinga’s picture

Status: Needs work » Needs review
FileSize
760 bytes

I don't know if we necessarily need a dedicated test for admin role. If so, maybe we should create a followup.

sun’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

I wasn't aware that this was still open for adding a test.

Just adding an assertion to the existing (as opposed to my larger test changes earlier) is fine with me.

Patch still applies cleanly for me locally.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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