Problem/Motivation

Because this is the legacy behavior #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities is setting the default value of the user status field to TRUE, i.e. users are activated by default.

Because this has access implications users should be blocked by default and only activated when explicitly wanted.

Proposed resolution

  • Change the default value of the of the user status field to FALSE
  • Adapt tests that rely on the current behavior

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#7 2248969-7-user-status-default-false.patch3.91 KBtstoeckler
#7 2248969-5-7-interdiff.txt728 byteststoeckler
#5 2248969-5-user-status-default-false.patch3.91 KBtstoeckler
#3 2248969-3-user-status-default-false.patch711 byteststoeckler
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

+1, as it is the same behavior as node

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Related issues: +#2257761: Interactive installer tests cause emails to be sent out

Just referencing this here so as to make sure that we're aware of potential side-effects.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Active » Needs review
FileSize
2248969-3-user-status-default-false.patch711 bytes

Let's see what breaks.

Status: Needs review » Needs work

The last submitted patch, 3: 2248969-3-user-status-default-false.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Needs work » Needs review
FileSize
2248969-5-user-status-default-false.patch3.91 KB

Here we go. This should be green.

We should seriously refactor AccountForm/RegisterForm, the amoung of if-checks there on various different conditions is pretty bad. But I couldn't really think of a way to cleanly do that as part of this issue, so I just added one more.

No interdiff, because I messed up my local branch and the previous patch was so trivial that I couldn't be bothered to recreate an interdiff retroactively.

blueminds’s picture

blueminds CreditAttribution: blueminds commented 
+++ b/core/modules/user/src/RegisterForm.php
@@ -47,6 +48,14 @@ public function form(array $form, array &$form_state) {
+    // if needed. When administers create users from the user interface,

typo: ... administrators ...

Otherwise looks good.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
FileSize
2248969-5-7-interdiff.txt728 bytes
2248969-7-user-status-default-false.patch3.91 KB

Ahh, thanks. English is hard sometimes... :-)

blueminds’s picture

blueminds CreditAttribution: blueminds commented

:) 1+ RTBC

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Needs review » Reviewed & tested by the community

@blueminds, assuming you wanted to change the status per your comment?!

blueminds’s picture

blueminds CreditAttribution: blueminds commented

yup :)

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented 
+++ b/core/modules/user/src/RegisterForm.php
@@ -47,6 +48,14 @@ public function form(array $form, array &$form_state) {
+    // Because the user status has security implications, users are blocked by
+    // default when created programmatically and need to be actively activated
+    // if needed. When administrators create users from the user interface,
+    // however, we assume that they should be created as activated by default.

Wouldn't it be more appropriate to expose the status checkbox on the administrative user creation form instead of presuming any particular state?

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

It is exposed, but the default value of the checkbox is taken from $account->get('status'). This is cleaner than setting the #default_value manually afterwards (which I generally would have preferred), because where #default_value is set it also checks a bunch of other conditions, so we would have to recreate that check.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed fa9e2b5 and pushed to 8.x. Thanks!

  • Commit fa9e2b5 on 8.x by alexpott: 
    Issue #2248969 by tstoeckler: Default the user status field to FALSE.

Status: Fixed » Closed (fixed)

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