Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Let users use their username OR password as credential to log in #15725

Closed
wants to merge 6 commits into from

Conversation

peterpeter
Copy link
Contributor

@peterpeter peterpeter commented May 1, 2017

Pull Request for Issue #15712 .
With this patch it is possible for users to use their email instead of the username (plus the password), as it is more and more a common behaviour on most other websites, -systems, -shops.
First approach

Summary of Changes

Added Parameter and changed sql query in joomla authenticate plugin.
Added check and language strings for the form labels of mod_login and com_users

Testing Instructions

Add this patch, go in administration to plugins - authentication - joomla, open it and set the param ' Login with email allowed?' to yes and save your choice.

Expected result

With patch added, the form labels for the fields 'username' changed from 'Username' to 'Username or email' in both, mod_login and com_users on frontend.
And it should be possible to login with your email and passwort.

With setting the param 'Login with email allowed?' to 'No' log in with email should be no more possible, and the form labels should change back to 'Username'.

Follow ups

The behaviour changes in both, front- and backend. Would be maybe not that bad to have an additional choice wether to add this feature just in frontend or in both, front- and backend. Any thoughts?

…ential to log in through the joomal authenticate plugin
@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-staging labels May 1, 2017
@brianteeman
Copy link
Contributor

I am not in favour of this change for security reasons. If a site owner wants to do this there are several extensions available. We should be increasing security not reducing it.

@Bakual
Copy link
Contributor

Bakual commented May 2, 2017

Is there an issue we try to solve here which can't already be done using 3rd party extensions?
If it can be solved with an extension already, I don't see the need for us to include it into core. Looks like quite a situational usecase to me.

@peterpeter
Copy link
Contributor Author

There are individual uses cases to solve and there are common web standards/behaviour to be included. In my opinion this patch is more for the latter. Again, I think Joomla should behave the same as other applications out of the box, after the installation, and not with additional extensions.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15725.

@@ -11,6 +11,21 @@

JHtml::_('behavior.keepalive');
JHtml::_('behavior.formvalidator');

$isJoomlaAuthenticationEnabled = JPluginHelper::isEnabled('authentication' , 'joomla');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove any conditional which check for a specific plugin implementation. The module should be unaware of the active plugin(s) and its parameters.
Another authentication plugin should be able to provide the same features.

$isJoomlaAuthenticationEnabled = JPluginHelper::isEnabled('authentication' , 'joomla');
$usernameTextToken = 'MOD_LOGIN_VALUE_USERNAME';

if($isJoomlaAuthenticationEnabled)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, no checking for a specific plugin.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, agree, that was about to change the form label language token. Will remove that later this evening (now @work ;) )

@@ -47,6 +47,12 @@ public function onUserAuthenticate($credentials, $options, &$response)
->from('#__users')
->where('username=' . $db->quote($credentials['username']));

// should we check the email too=
if((bool)$this->params->get('login_with_email_allowed', 0))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the default value for login_with_email_allowed is 0, how can this be cast to a boolean?

@brianteeman
Copy link
Contributor

The correct way to do this is with a NEW authentication plugin specifically to allow login by email. That is why we have authentication plugins.

(Then we end up with exactly what the available extensions can already do - so there is no point ijn adding this to the core)

@peterpeter
Copy link
Contributor Author

peterpeter commented May 5, 2017

Again: I made this PR because I think that common standards should be in the core and available out of the box, as SEO, Microdata, RSS, Tags....too. The security impact is not that bad and as it is off by default at least the site-admins decision. Every different authentication method should be implemented as own plugin, true. But this is just an addition to a existing one - and not a new or different one.

If this PR not get merged is not a big thing to me. But I think it's a chance to get more even with other web-applications. Just my 2 cents.

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on f6612ba

I have tested this and it works as described - still not personally convinced its an improvement though


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15725.

@ghost
Copy link

ghost commented May 23, 2017

I have tested this item ✅ successfully on f6612ba


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15725.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label May 23, 2017
@ghost
Copy link

ghost commented May 23, 2017

RTC after two successful tests.

@brianteeman
Copy link
Contributor

This is a new feature so I have added the new feature option

@franz-wohlkoenig please remove the RTC - just because a new feature is tested as working doesnt mean it is a good thing and can be merged - as you can see both @Bakual and myself expressed our doubts about the usefullness of this

@joomla-cms-bot joomla-cms-bot removed Feature RTC This Pull Request is Ready To Commit labels May 23, 2017
@ghost
Copy link

ghost commented May 23, 2017

@brianteeman RTC removed. My Thought was that Maintainers can decide independent from Status & 2 sucessfully Tests didn't get lost.

@idefax
Copy link

idefax commented May 23, 2017

I have tested this item ✅ successfully on f6612ba

Works perfect.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15725.

@idefax
Copy link

idefax commented May 23, 2017

This is a very functional and great feature for the users, since it's very common to login with email address.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15725.

@rdeutz
Copy link
Contributor

rdeutz commented May 23, 2017

we have not merged better solution to solve the problem, see #10347

@infograf768
Copy link
Member

Some thought:
I disagree with the statement that this would degrade security. The password and 2FA do the job OK imho.
I disagree also with the statement that because some 3rd party extensions exist, we should not include this in core. Remember my plugins Unicode slugs and Registration approval for 1.5, and many others.
I also disagree with the solution that was proposed in #10347 as it was not BC.

I will reset this as RTC as it was tested and works fine.
The final decision is indeed the choice of the release leader and PLT as rightfully posted by @franz-wohlkoenig

@infograf768 infograf768 added the RTC This Pull Request is Ready To Commit label May 27, 2017
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label May 27, 2017
@mbabker
Copy link
Contributor

mbabker commented May 27, 2017

By adding an additional WHERE clause, and because the username and email columns are only unique within themselves, it is possible now for this query to return a non-unique result set (as in 2 records versus one if somehow user A has a username of [email protected] and user B has an email address of [email protected]). This scenario needs to be addressed.

@zero-24
Copy link
Contributor

zero-24 commented May 27, 2017

Please also check com_joomlaupdate login form iirc thy check on username only too.

@josephsimony
Copy link

The subject doesn't sound right (it was strange to read of a feature like that).
I guess the starter meant: Let users use their username OR email as credential to log in

@uglyeoin
Copy link
Contributor

I like this PR and think it would be useful. I think it is a UX thing. Users expect certain behaviour and if they cannot remember their username they will likely try their email address. I would argue that the email address is unique and thus could replace the username option. In that situation it would solve the problem this PR solves and I think it overcomes the security issue @brianteeman mentioned. It would break b/c though by doing it that way though. In any case I think users would try to use their email address in some cases.

@mbabker
Copy link
Contributor

mbabker commented Jul 14, 2017

A system should only have one identifier for a user. Either a username or a password. A username can be any unique value identifying that user, be it an email address, a customized username, or a system generated identifier; we don't impose a restriction that says the username has to be a value that is not used in one of the other columns in the users table.

A site can use language overrides to change the labels if they want to steer users toward using their email address as their usernames. There are also plugins which can accomplish the same.

The long and short is I still don't see something that requires us to make a core change to allow an email address to be a username as you can already do it, it is just stored as a separate field in the database. If we were to add such an option, it would introduce site management issues as we would have to force that the username AND email columns be unique (a username and email can only be the same if it is the same record, a username cannot be duplicated as an email address in another record (and vice-versa) as that would mean the system would potentially try to log into 2 different accounts). This patch does not address the data integrity issue, as I pointed it out nearly 2 months ago.

@rdeutz
Copy link
Contributor

rdeutz commented Jul 14, 2017

the "problem" is solved else where, closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Language Change This is for Translators
Projects
None yet
Development

Successfully merging this pull request may close these issues.