Support for custom login credentials

[datamweb]

Hi all,
When we create a custom column in table users, for example phone_number.
And then we set the value to

    /**
     * --------------------------------------------------------------------
     * Valid login fields
     * --------------------------------------------------------------------
     * Fields that are available to be used as credentials for login.
     */
    public array $validFields = [
      //  'email',
        //'username',
        'phone_number',
    ];

It is not possible to log into the system. If there should be no problem to login.

We need to make some changes in this area.

shield/src/Authentication/Authenticators/Session.php

Lines 251 to 258 in 7855d91

	$idType = (! isset($credentials['email']) && isset($credentials['username']))
	? self::ID_TYPE_USERNAME
	: self::ID_TYPE_EMAIL_PASSWORD;
	$this->loginModel->recordLoginAttempt(
	$idType,
	$credentials['email'] ?? $credentials['username'],
	$success,

Do you agree?

[paulbalandan]

Is there a valid reason to support login credentials other than email and username? If there's none, then this proposal only adds unnecessary code.

[kenjis]

If you change the login ID, you should customize Session authenticator.
The Session authenticator does not know how to handle phone_number (and the password).

[datamweb]

The primary goals for Shield are:

It must be very flexible and allow developers to extend/override almost any part of it.
It must have security at its core. It is an auth lib after all.
To cover many auth needs right out of the box, but be simple to add additional functionality to.

[paulbalandan]

to extend/override almost any part of it.

I partly beg to disagree on this. As with any other software, the creator has a vision in mind of what they want for the software, be it security-wise or function-wise. Implementing a software that is very open to extensions because the end users might need that extendibility some day seems not right for me. As with the YAGNI principle, "Always implement things when you actually need them, never when you just foresee that you need them."

[MGatner]

YAGNI surprised me, so I read a bit and it is part of extreme programming which needs to be taken as a whole by its definition:

It is meant to be used in combination with several other practices, such as continuous refactoring, continuous automated unit testing, and continuous integration...
YAGNI's dependency on supporting practices is part of the original definition of XP.

I do think there is some concern here around the "Open/Closed Principle" (from SOLID) but given that we already have configurable login fields it seems that supporting custom credentials was part of the original intent. I'd be curious to see an actual PR, to see how "open" we would have to be to support the phone number example.

[paulbalandan]

Although we can doesn't mean we should do it. Adapting the code to cater all possible configurable login fields would mean a "hidden" feature Shield would not want to advocate/promote. That is like telling a developer that they can explicitly use any arbitrary field for the login credentials since Shield can manage whatever login field they configure, which can be exploited later on into some security vulnerability. That's not worth the risk, in my opinion.

This reservation of mine stems from my theoretical understanding of an authentication system, and I'm not yet familiar with the inner workings of Shield so I might be wrong. If Shield has the necessary guards in place, then I might have a change of heart. 😊

[lonnieezell]

Here's the use cases I've heard for custom fields:

• an employee ID only where the system is on an internal company network
• a school ID for use within a school's network only
• a phone number

All of these are valid for their intended purposes, and we have the $validFields that is there to support them. Assuming they are a field within the user table, it is a simple thing to support. Shield already ensures you can only use one of those valid fields as a credential during login with the Session controller. I see no compelling reason, or current security risk with allowing that and ensuring it works as intended. Restricting to a username column is no different than restricting to a phone_number column.

I believe the constants in question were added after the initial implementation and we lost some of the flexibility originally intended. This isn't the first time something like this has been brought up. So we have two options that I can see:

1. Fix it so the $validFields actually works
2. Remove/rename that field so only email/username works.

I am in favor of fixing it so it actually works.

[kenjis]

Those constants are just strings changed to constants. While certainly not flexible, this implementation was not originally "open" in the OCP meaning.

shield/src/Authentication/Authenticators/Session.php

Lines 242 to 249 in edd3510

	$idType = ! isset($credentials['email']) && isset($credentials['username'])
	? 'username'
	: 'email';
	$this->loginModel->recordLoginAttempt(
	$idType,
	$credentials['email'] ?? $credentials['username'],
	$success,

[pixobit]

The only reason I'm switching to shield from a custom implementation, is because of higher security. However, i was only able to decide in favor of shield, because of its flexibility. If shield would sooner or later put more limitations rather than help me, then it should be advertised so.
I usually tend to avoid libraries that limit me, rather than helping me.
I'm completely in favor in supporting phone_numbers, since its very much a valid sign in field.

[kenjis]

Assuming they are a field within the user table, ... Restricting to a username column is no different than restricting to a phone_number column.

I agree with it.

[datamweb]

I don't know what the concerns about shield security are about.
Codeigniter allows writing custom validators(see).
On the other hand, Shield allows the use of custom validation rules.

If we look carefully at the code, we will find that support for this feature was considered from the beginning. After merge #181 we have this problem.

Therefore, if we guide users that they must execute custom rules when using custom fields,
If these validations are implemented correctly, what is the difference between email, username , phone_number and ... that will compromise the security of the shield.

I'd be very interested to see an example of the risk created in code.

However, maybe I'm wrong!

[khan-umer]

I agree with @lonnieezell use cases , in most of sites provide other login option like login with OTP which is missing in shield Auth.

I think there will be no challenge for this feature & I hope we will get some solution in upcoming release.