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

Add option for alternate identity column #746

Closed
wants to merge 3 commits into
base: 2
from

Conversation

Projects
None yet
8 participants
@zepernick
Copy link

zepernick commented Mar 4, 2015

Added a option to allow a secondary identity for the username, such as, the username and email.

The new config key is 'identity_alt'

Added a option to allow a secondary identity for the username, such a…
…s, the username and email.

The new config key is 'identity_alt'
@avenirer

This comment has been minimized.

Copy link
Contributor

avenirer commented Mar 4, 2015

I don't understand your pull request... You want the user to login with either email or username?

@zepernick

This comment has been minimized.

Copy link
Author

zepernick commented Mar 4, 2015

Yes, it provides the option for the user to use their email or username for login. It is optional in the config so that it can still allow just one identity if someone were to not want that functionality.

@avenirer

This comment has been minimized.

Copy link
Contributor

avenirer commented Mar 4, 2015

What would be happening to a user that has two usernames (associated with two or more different groups) on one and the same email, when he will login using email?

@avenirer

This comment has been minimized.

Copy link
Contributor

avenirer commented Mar 4, 2015

...same email and password...

@zepernick

This comment has been minimized.

Copy link
Author

zepernick commented Mar 4, 2015

I did not realize that the same email was being allowed on 2 different users. I have an older version of the controller and the register_user_submit() contained the following, which led me to that conclusion

$this->form_validation->set_rules('email', $this->lang->line('create_user_validation_email_label'), 'required|valid_email|is_unique['.$tables['users'].'.email]');

I reviewed the ion_auth_model and it looks like it would reject the login if more than 1 user had the same email and that was being used to authenticate. There is a if statement checking num_rows() === 1. It would return FALSE due to the fact that it would be > 1.

@avenirer

This comment has been minimized.

Copy link
Contributor

avenirer commented Mar 5, 2015

You are right about the controller, but as I remember the Ion_auth model only checks to see if the identity column (either username or password) already exists. So if you set username as identity column, the model won't look if the username's email already exists (which to me seems common sense). This way, with same email you may have more usernames depending on what groups you want to be part of at a certain moment in time. The controller is only there as a usage example.

@zepernick

This comment has been minimized.

Copy link
Author

zepernick commented Mar 5, 2015

Makes sense. I will correct the model to enforce the unique constraint on both columns if the 'identity_alt' is set. Let me know if there is interest incorporating this patch once that is done?

Assuming there is interest in merging this request.... Is there anything you would want to see happen in the login if the results returned in > 1? This case could happen if the email was used for a alternate identity on a existing installation. It could also happen in the current code if the configuration were to switch from username to email on the identity.

@zepernick

This comment has been minimized.

Copy link
Author

zepernick commented Mar 5, 2015

I reviewed the entire model and apparently there is a lot of different places that are referencing the identity besides the register() and login(). I went through and updated all references to take the alternate identity into account. I will be pushing it shortly...

@benedmunds

This comment has been minimized.

Copy link
Owner

benedmunds commented Mar 5, 2015

Awesome, thank you! Please @ tag me when it's all pushed up to I get a notification.

@zepernick

This comment has been minimized.

Copy link
Author

zepernick commented Mar 5, 2015

@benedmunds it is all pushed out

@benedmunds

This comment has been minimized.

Copy link
Owner

benedmunds commented Mar 6, 2015

Thanks for the work on this, it's very well done. I'm really not sure if this is worth complicating the library for this use case though.

Can other users vote on if they have ever needed this? +1 or -1 please.

@zepernick

This comment has been minimized.

Copy link
Author

zepernick commented Mar 6, 2015

Sure no problem, it is out there on my repo if anyone ever needs it. I had to make it work for the project I am doing so I just thought I would share.

It seems to be getting fairly common now to be able to log into sites with your username or email address. Another case might be something like username and phone # if that is being collected.

Thanks for considering it to become part of the core library.

@benedmunds

This comment has been minimized.

Copy link
Owner

benedmunds commented Mar 6, 2015

Ha story time. I added this functionality once as well but it ended up being such a support nightmare that I abandoned it.

Thanks! If people really want it I'll merge it.

@avenirer

This comment has been minimized.

Copy link
Contributor

avenirer commented Mar 6, 2015

Well... this one is a bit scary for me. I mentioned it in my comments. Maybe is just my problem but I was using Ion_auth with username as identity. Also, working with groups, I created different usernames having the same email address. This way I could see the same controllers by simply changing the username, but in the same time having the same email and password. Considering the constraint on both columns only if identity_alt is enabled I would give it a 👍 . @benedmunds are you ready for the nightmare?

@ghost

This comment has been minimized.

Copy link

ghost commented Mar 6, 2015

+1 and thanks!

@zepernick

This comment has been minimized.

Copy link
Author

zepernick commented Mar 6, 2015

The identity_alt is coded to be optional. It will run okay if it is not in the config at all for backward compatibility. The unique constraint is only enforced if identity_alt has been specified in the config.

Just to summarize again. This option will be OFF if the identity_alt is not in the config OR if it = ''

@ggjacob

This comment has been minimized.

Copy link

ggjacob commented Mar 11, 2015

+1

@avenirer

This comment has been minimized.

Copy link
Contributor

avenirer commented Mar 12, 2015

It would be nice to redo what you've worked so far, and respect some of the codeigniter's style guide (and the general coding style of the library): don't do $altIdCol, do $alt_id_col (or even $alt_identity_col). Don't do $doIdCheck... Also, this is too complicated to read: $this->identity_column_alt = $altIdCol !== FALSE ? $altIdCol : ''; Also, you should mention some of this new feature inside the config file, and maybe in the documentation?

@benedmunds

This comment has been minimized.

Copy link
Owner

benedmunds commented Mar 12, 2015

Agreed. Although ATM I'm still very much leaning towards not merging.

@avenirer

This comment has been minimized.

Copy link
Contributor

avenirer commented Mar 12, 2015

I guess you're right. It's adding complexity where there is no need for complexity. Could I change my opinion to -1?

@benedmunds

This comment has been minimized.

Copy link
Owner

benedmunds commented Mar 12, 2015

You can. I'm closing for now. People can feel free to campaign that I merge if they want it badly. Otherwise they can always merge this branch into their local to add the functionality.

@benedmunds benedmunds closed this Mar 12, 2015

@iamthestreets

This comment has been minimized.

Copy link

iamthestreets commented Oct 20, 2015

I know this is closed, but I am trying to implement this and was wondering if I can get some explanations on this implementation?

What is this doing:
$this->identity_column_alt = $altIdCol !== FALSE ? $altIdCol : '';

The way I read it is if the $altIdCol is not FALSE get the config value $this->config->item('identity_alt', 'ion_auth'); else set it to an empty value.

How does $altIdCol become FALSE?

Also, why do this:

if(empty($this->identity_column_alt) === FALSE) {
            $this->db->or_where($this->identity_column_alt, $identity);
         }
@zepernick

This comment has been minimized.

Copy link
Author

zepernick commented Oct 21, 2015

Hello,

$altIdCol              = $this->config->item('identity_alt', 'ion_auth');
        $this->identity_column_alt = $altIdCol !== FALSE ? $altIdCol : '';

I was trying to make this optional in the config for backward compatibility. The config->item will return false if the item did not exist in the configuration. It evaluates the false and defaults the identity_column_alt property to '' if the key did not exist in the configuration file.

I just checked the CI3 docs and it looks like this is returning a NULL now instead of FALSE. That check should be updated in the code if using the newest version of CI.

https://codeigniter.com/user_guide/libraries/config.html

Here is a explination of this code

if(empty($this->identity_column_alt) === FALSE) {
            $this->db->or_where($this->identity_column_alt, $identity);
         }

The empty() === false is making sure that a alternate id was setup in the config file. This would have been better expressed as $this->identity_column_alt !== ''. The or_where ends up changing the sql to search for the username in the username field or the identity_column_alt , which in my case, I was using the email. Lets say you processed the login form with username email@somewhere.com. The sql would end up looking like (user = 'email@somewhere.com' or email='email@somewhere.com').

@iamthestreets

This comment has been minimized.

Copy link

iamthestreets commented Oct 21, 2015

Thanks.

Here is what I ended up doing and it seems to be working. Let me know if you see any issues or potential issues with it. I am not concerned with backwards compatibility.

In my config I have it set to:
$config['identity'] = 'email';
$config['identity_alt'] = 'username';

In the model I have added this in the construct:
$this->load->helper('email');
$this->identity_alt_column = $this->config->item('identity_alt', 'ion_auth');

And in the functions I am doing this:

$identity_column = (valid_email($identity) ? $this->identity_column : $this->identity_alt_column);

$query = $this->db->select($this->identity_column . ', ' . $this->identity_alt_column . ', id, password, active, last_login')
                          ->where($identity_column, $identity)
                          ->limit(1)
                          ->order_by('id', 'desc')
                          ->get($this->tables['users']);
@FraOre

This comment has been minimized.

Copy link

FraOre commented Nov 10, 2015

+1

@FraOre

This comment has been minimized.

Copy link

FraOre commented Nov 11, 2015

Hi Ben,

are you sure that this branch can not be marged in? I thing can be a great opportunity for the users can login by email or username.

Please think about it!

@itodorovic

This comment has been minimized.

Copy link

itodorovic commented Feb 26, 2016

@iamthestreets solution is the best IMHO. Check if identity is valid email address, if so use it, otherwise use username for authentication. Covers all use cases properly, even those users with multiple usernames and single email.

Edit: attached diff
diff.txt
, works like a charm here.

@dumb

This comment has been minimized.

Copy link

dumb commented Jun 7, 2016

Just wanted to thank @zepernick for this. Even though it unfortunately wasn't merged, I found it super useful and have implemented it in my setup. Cheers!

@benedmunds

This comment has been minimized.

Copy link
Owner

benedmunds commented Jun 7, 2016

👏

@zepernick

This comment has been minimized.

Copy link
Author

zepernick commented Jun 7, 2016

@dumb You're welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.