Update proposal on the use of Member :: unique_identifier_field and SS_DEFAULT_ADMIN_USERNAME #9493

alessandromarotta · 2020-04-22T12:57:12Z

Considering this issue #4990 and other similar ones opened, I propose some updates about the:

use of SS_DEFAULT_ADMIN_EMAIL instead of SS_DEFAULT_USERNAME
improvement of the Member :: unique_identifier_field variable
updating documentation

The code is tested and working in my local installation.

…S_DEFAULT_ADMIN_USERNAME Considering this issue #4990 and other similar ones, I updated some parts of the code about the: - use of SS_DEFAULT_ADMIN_EMAIL instead of SS_DEFAULT_USERNAME - improvement of the Member :: unique_identifier_field variable The code is tested and working in my local installation.

The value of SS_DEFAULT_ADMIN_USERNAME is registered in "Member->Email", so why is it called "SS_DEFAULT_ADMIN_USERNAME"? I propose to name it to SS_DEFAULT_ADMIN_EMAIL for the default "unique_identifier_field" ('Email')

dhensby · 2020-04-22T14:24:30Z

I've noticed that you've created this pull request from one of the "core branches" rather than a branch specifically intended for this pull request. This can cause problems if we ask you to target this PR at a different branch. No action is needed at the moment, this is for your information.

In the future please create a new branch for each individual pull request. See our guide about contributing PRs

dhensby · 2020-04-22T14:46:29Z

Hi @alessandromarotta, thanks for taking the time to look into this and to provide a PR with some fixes. Raising an issue first next time would probably be better as it would allow the core team to provide thoughts on the merit of the change as well as some guidance on how we think it's best resolved.

Regarding the general idea of changing references from "username" => "email", I completely agree that semantically that would be more accurate and probably more helpful in most cases, but we should also take into account that there's actually no validation on the Email field to enforce that it is an email, which means this change won't actually save that much of the confusion (which is that Member.Email isn't always a valid email address).

We also need to weigh up the benefit of having a more concise and predictable codebase with the impact this is going to have on developers who use our software and will need to make periodic upgrades. We need to ask ourselves, "will this change yield a bigger benefit than the effort it takes to upgrade our projects?". This change would either need reworking so that it is "backwards compatible" (ie: people can still use the existing settings without anything breaking) or it would need to be targeted at v5 (master branch) where we can freely add changes that can "break" projects when people upgrade.

One of the challenges here is that we appear to have a configurable "member_identifier_field" but this setting hasn't really had the traction or adoption it should have so it's a bit of a half-implemented feature that very few developers are aware of. Whilst it's there with good intention, it's not used like it should be as we can see by how many times Email is hard coded into the codebase.

My feeling at the moment is that the semantic improvement to the code base does not justify the amount of pain this will cause developers, though if a fully backward compatible implementation were available I would consider that as a possibility depending on the thoughts of other core contributors.

I'll leave this open for further comments from contributors.

alessandromarotta · 2020-04-22T16:05:17Z

Hi @dhensby,

Regarding the general idea of changing references from "username" => "email", I completely agree that semantically that would be more accurate and probably more helpful in most cases, but we should also take into account that there's actually no validation on the Email field to enforce that it is an email, which means this change won't actually save that much of the confusion (which is that Member.Email isn't always a valid email address).

I introduced the EmailField in the MemberLoginForm.php when the "unique_identifier_field" is set to "Email" (default), so now we have at least the HTML5 validation. We may also introduce the domain DNS validation (checkdnsrr) of the email address.

We also need to weigh up the benefit of having a more concise and predictable codebase with the impact this is going to have on developers who use our software and will need to make periodic upgrades. We need to ask ourselves, "will this change yield a bigger benefit than the effort it takes to upgrade our projects?". This change would either need reworking so that it is "backwards compatible" (ie: people can still use the existing settings without anything breaking) or it would need to be targeted at v5 (master branch) where we can freely add changes that can "break" projects when people upgrade.

Well, if the "unique_identifier_field" variable is not set, nothing really needs to be changed, other than using SS_DEFAULT_ADMIN_EMAIL instead of SS_DEFAULT_USERNAME in the ".env" configuration file.

One of the challenges here is that we appear to have a configurable "member_identifier_field" but this setting hasn't really had the traction or adoption it should have so it's a bit of a half-implemented feature that very few developers are aware of. Whilst it's there with good intention, it's not used like it should be as we can see by how many times Email is hard coded into the codebase.

I believe that this feature has not been used because it didn't work.
I tried to set Member::unique_identifier_field = "Username" in a brand new installation of SilverStripe 4.5.2 and I realized that in reality it did not do what it should: set the unique_identifier_field for the Login form and store the value of SS_DEFAULT_ADMIN_USERNAME in the db field "Username". The only change made was the Label of the "Email" field of the Login form (which obviously continued to be called "Email" and it was treated as a Email field)

Now we can set any type of unique_identifier_field that we want.
Do we want to set the Username field for the login?

".env" file:
[...]
SS_DEFAULT_ADMIN_USERNAME = "admin"
SS_DEFAULT_ADMIN_EMAIL = "admin_email"
[...]

"mysite.yml" file:
SilverStripe\Security\Member:
unique_identifier_field: Username

Do we want to set the "FidelityCardID" field for the login?

".env" file:
[...]
SS_DEFAULT_ADMIN_FIDELITYCARDID = "myFidelityCardNumber"
SS_DEFAULT_ADMIN_EMAIL = "admin_email"
[...]

"mysite.yml" file:
SilverStripe \ Security \ Member:
unique_identifier_field: FidelityCardID

Of course the email should always be a required field for obvious reasons (for example recovering the password)

My feeling at the moment is that the semantic improvement to the code base does not justify the amount of pain this will cause developers, though if a fully backward compatible implementation were available I would consider that as a possibility depending on the thoughts of other core contributors.

I made the changes to the code trying to maintain a backward compatibility with the current state. The only change required, if unique_identifier_field is not set, is to set SS_DEFAULT_ADMIN_EMAIL instead of SS_DEFAULT_ADMIN_USERNAME for new Silverstripe installations, for this reason I have also updated the documentation.

Thanks for your attention,
Alessandro

Correction of $member->$uniqueIdentifier to $member->$uniqueIdentifierFieldName

alessandromarotta · 2020-04-22T18:07:38Z

This is the situation with the current code: only the label is changed in the Login form.

This is the situation in the database: with "unique_identifier_field" set to "Username" the value of "SS_DEFAULT_ADMIN_USERNAME" is recorded in the "Email" field.

dhensby

Just for comitters - latest build is still failing - travis isn't showing up for some reason:

https://travis-ci.org/github/silverstripe/silverstripe-framework/builds/678260262

… $defaultUsername

…name and $defaultUsername" This reverts commit f903fa3.

Re-introduced the function getDefaultUsername()

Want to keep the actual status (username stored in email field)? .env: SS_DEFAULT_ADMIN_USERNAME = "admin" SS_DEFAULT_ADMIN_PASSWORD = "admin_password" Want to use the email for the Login with the HTML5 validation? .env: SS_DEFAULT_ADMIN_EMAIL = "admin@email.test" SS_DEFAULT_ADMIN_PASSWORD = "admin_password" Want to use the LicenseID for the Login? .env: SS_DEFAULT_ADMIN_LICENSEID = "TA123456789" SS_DEFAULT_ADMIN_EMAIL = "admin@email.test" SS_DEFAULT_ADMIN_PASSWORD = "admin_password" mysite.yml: SilverStripe\Security\Member: db: LicenseID: Varchar unique_identifier_field: LicenseID p.s: now if miss a SS_DEFAULT_ADMIN_* varabile, we get a BadMethodCallException that reminds us to check de default admin configuration

dhensby

Whilst I think this is with good intention, it's clearly going to be very hard to implement this properly and thoroughly. There's also missing tests which mean some obvious logical errors are not being caught (such as the default admin username and email and password needing to be set to be able to login when the uniquer identifier field is not email).

I'd actually rather completely remove the concept of unique_identifier_field as it's not implemented properly and rather provide guidance to developers about how to customise login forms and authenticators to use other fields if that is desired.

Yes, the Email field is misnamed on Member but I don't think we can do anything about that in SS 4

dhensby · 2020-05-17T16:55:47Z

docs/en/02_Developer_Guides/09_Security/03_Authentication.md

 SS_DEFAULT_ADMIN_PASSWORD="password"
 ```

-When a user logs in with these credentials, then a [Member](api:SilverStripe\Security\Member) with the Email 'admin' will be generated in
+When a user logs in with these credentials, then a [Member](api:SilverStripe\Security\Member) with the Email 'admin@email.here' will be generated in


please use example.com for documentation as that is it's expressed purpose in RFC 2606

dhensby · 2020-05-17T16:56:31Z

docs/en/03_Upgrading/01_Upgrading_project.md

@@ -467,7 +467,7 @@ For example, take the following `_ss_environment.php` file.
 <?php
 // Environment
 define('SS_ENVIRONMENT_TYPE', 'dev');
-define('SS_DEFAULT_ADMIN_USERNAME', 'admin');
+define('SS_DEFAULT_ADMIN_EMAIL', 'admin@email.here');


this would never have been set in v3 as it's not supported there, so this change to the upgrade guide is wrong

dhensby · 2020-05-17T17:04:11Z

docs/en/00_Getting_Started/03_Environment_Management.md

@@ -102,7 +102,7 @@ SilverStripe core environment variables are listed here, though you're free to d
 | `SS_DATABASE_CHOOSE_NAME`| Boolean/Int.  If defined, then the system will choose a default database name for you if one isn't give in the $database variable.  The database name will be "SS_" followed by the name of the folder into which you have installed SilverStripe.  If this is enabled, it means that the phpinstaller will work out of the box without the installer needing to alter any files.  This helps prevent accidental changes to the environment. If `SS_DATABASE_CHOOSE_NAME` is an integer greater than one, then an ancestor folder will be used for the  database name.  This is handy for a site that's hosted from /sites/examplesite/www or /buildbot/allmodules-2.3/build. If it's 2, the parent folder will be chosen; if it's 3 the grandparent, and so on.|
 | `SS_DEPRECATION_ENABLED` | Enable deprecation notices for this environment.|
 | `SS_ENVIRONMENT_TYPE`| The environment type: dev, test or live.|
-| `SS_DEFAULT_ADMIN_USERNAME`| The username of the default admin. This is a user with administrative privileges.|
+| `SS_DEFAULT_ADMIN_EMAIL`| The email address of the default admin. This is a user with administrative privileges.|


I think the renaming of this is misguided - what it's trying to be is the value for the unique_identifier_field regardless of what that happens to be.

It's the username in the sense of basic auth would be base64_encode('[username]:[password]'). The fact it is actually an email (or not) or that it is some other field in the database is not relevant. it's designed to replace the username and password combination that would be used for login and logging in as default admin doesn't check the DB, so the name of the field is not important.

tl;dr: The username part doesn't literally mean a DB field or imply anything about the expected format - email does so it should not be changed

dhensby · 2020-05-17T17:04:53Z

src/Security/DefaultAdminService.php

     */
-    public static function setDefaultAdmin($username, $password)
+    public static function setDefaultAdmin($username, $password, $email = null) 


as in my previous comment, none of these changes should be made to this function

dhensby · 2020-05-17T17:06:27Z

src/Security/DefaultAdminService.php

-                && !empty(Environment::getEnv('SS_DEFAULT_ADMIN_PASSWORD'));
+            $uniqueIdentifierFieldName = Member::config()->get('unique_identifier_field');
+            if($uniqueIdentifierFieldName != 'Email') {
+                return !empty(Environment::getEnv('SS_DEFAULT_ADMIN_' . strtoupper($uniqueIdentifierFieldName)))


Why would all 3 of these need values to pass if the identifier field is not email?

dhensby · 2020-05-17T17:10:03Z

src/Security/MemberAuthenticator/MemberAuthenticator.php

+                $tempMember->$uniqueIdentifierFieldName = $username;
+
+            if(!empty(DefaultAdminService::getDefaultAdminEmail()))
+                $tempMember->Email = DefaultAdminService::getDefaultAdminEmail();


Why are we using the default admin email here? This logic path is explicitly for when we are not authenticating as default admin

dhensby · 2020-05-17T17:11:15Z

src/Security/MemberAuthenticator/MemberLoginForm.php

        $fields = FieldList::create(
            HiddenField::create("AuthenticationMethod", null, $this->getAuthenticatorClass(), $this),
            // Regardless of what the unique identifer field is (usually 'Email'), it will be held in the
            // 'Email' value, below:
            // @todo Rename the field to a more generic covering name
-            $emailField = TextField::create("Email", $label, null, null, $this),
+            $uniqueIdentifierField = ($uniqueIdentifierFieldName == 'Email' && !empty(DefaultAdminService::getDefaultAdminEmail())) ? EmailField::create("Email", $label, null, null, $this) : TextField::create($uniqueIdentifierFieldName, $label, null, null, $this), 


This must remain a text field otherwise the login field will break for every site out there that does not use emails for their default admin user

it would be a EmailField only when SS_DEFAULT_ADMIN_EMAIL is set, otherwhise it would be a TextField

Why is the login field determined by the admin's type? What if that is different to normal members?

dhensby · 2020-05-17T17:12:02Z

src/Security/MemberAuthenticator/MemberLoginForm.php


-        if (Security::config()->get('remember_username')) {
-            $emailField->setValue($this->getSession()->get('SessionForms.MemberLoginForm.Email'));
+        if (Security::config()->get('remember_' . strtolower($uniqueIdentifierFieldName))) { 


dynamic configs are a bad idea - this will also break for all existing sites that use emails as usernames

alessandromarotta · 2020-05-17T18:26:04Z

Whilst I think this is with good intention, it's clearly going to be very hard to implement this properly and thoroughly. There's also missing tests which mean some obvious logical errors are not being caught (such as the default admin username and email and password needing to be set to be able to login when the uniquer identifier field is not email).

I'd actually rather completely remove the concept of unique_identifier_field as it's not implemented properly and rather provide guidance to developers about how to customise login forms and authenticators to use other fields if that is desired.

Yes, the Email field is misnamed on Member but I don't think we can do anything about that in SS 4

This would be great: I'd like to have the possibility to use a custom login data (different from email)

Alessandro Marotta added 2 commits April 22, 2020 14:38

Update MemberAuthenticator.php

1c86f54

Correction of $member->$uniqueIdentifier to $member->$uniqueIdentifierFieldName

dhensby suggested changes Apr 22, 2020

View reviewed changes

Alessandro Marotta added 5 commits April 22, 2020 21:29

Update references to DefaultAdminService::getDefaultAdminUsername and…

f903fa3

… $defaultUsername

Revert "Update references to DefaultAdminService::getDefaultAdminUser…

3bde54a

…name and $defaultUsername" This reverts commit f903fa3.

Update DefaultAdminService for backward compatibility

17ec1fe

Re-introduced the function getDefaultUsername()

Update DefaultAdminService.php

32856c8

alessandromarotta closed this May 10, 2020

dhensby reviewed May 17, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update proposal on the use of Member :: unique_identifier_field and SS_DEFAULT_ADMIN_USERNAME #9493

Update proposal on the use of Member :: unique_identifier_field and SS_DEFAULT_ADMIN_USERNAME #9493

alessandromarotta commented Apr 22, 2020

dhensby commented Apr 22, 2020

dhensby commented Apr 22, 2020

alessandromarotta commented Apr 22, 2020

alessandromarotta commented Apr 22, 2020

dhensby left a comment

dhensby left a comment

dhensby May 17, 2020

dhensby May 17, 2020

dhensby May 17, 2020

dhensby May 17, 2020

dhensby May 17, 2020

dhensby May 17, 2020

dhensby May 17, 2020

alessandromarotta May 17, 2020

dhensby May 17, 2020

dhensby May 17, 2020

alessandromarotta commented May 17, 2020

Update proposal on the use of Member :: unique_identifier_field and SS_DEFAULT_ADMIN_USERNAME #9493

Update proposal on the use of Member :: unique_identifier_field and SS_DEFAULT_ADMIN_USERNAME #9493

Conversation

alessandromarotta commented Apr 22, 2020

dhensby commented Apr 22, 2020

dhensby commented Apr 22, 2020

alessandromarotta commented Apr 22, 2020

alessandromarotta commented Apr 22, 2020

dhensby left a comment

Choose a reason for hiding this comment

dhensby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alessandromarotta commented May 17, 2020