Zizaco / confide

Confide is a authentication solution for Laravel 4
1.19k stars 258 forks source link

Check for existing username or email #72

Closed mccombs closed 11 years ago

mccombs commented 11 years ago

It would be great if Confide would check for either an existing username or email so you don't end up with registration containing the same information.

P.S. We're loving this extension. It's really helped us get up and going.

andrew13 commented 11 years ago

That was added: https://github.com/Zizaco/confide/commit/5e752212c00d445c170b8508f6436d0a63f2a6e3

But it was breaking updates soit was rolled back. It's actually a semi bug in Ardent that is getting addressed. A work around is to cite a different rule set when updating. See https://github.com/andrew13/Laravel-4-Bootstrap-Starter-Site/blob/master/app/models/User.php for an example.

In short, it'll be added soon. Once it can be handled properly.

andrew13 commented 11 years ago

Leaving open, until addressed.

andrew13 commented 11 years ago

Maybe as a stop gap I'll include the update params and an update function in ConfideUser to allow the unique check to happen in Confide.

andrew13 commented 11 years ago

More info: https://github.com/Zizaco/confide/issues/64

andrew13 commented 11 years ago

I'm going to go ahead and do something like https://github.com/laravelbook/ardent/issues/37#issuecomment-16342965

in confideUser

richlove1 commented 11 years ago

Interesting reading that Ardent issue, that's the exact thing I was wondering about enforcing uniqueness on update vs creation.

andrew13 commented 11 years ago

Yep, I'm basically going to add the option to add to the params that get sent to save to specify update. If that update param is set to true it'll use the updateRules, otherwise it'll use the normal rules. Seems like the cleanest solution.

richlove1 commented 11 years ago

Yeah I was wondering along those line but thinking maybe there might be a cleaner way but you're probably right, I couldn't think of a better/cleaner solution than that.

Will the solution allow the user to change their email and/or username whilst also checking for uniqueness? (I know some systems don't allow username changes but I don't see why the user shouldn't be able to if their new choice is available)

andrew13 commented 11 years ago

You're correct that it would allow a user to change their email to an existing email. I suppose there might be a way to check the email/username if it changed from the original if you could compare it to a user id field or something of that nature.

I think I decided on a slightly different route for the update rules.

Add a protected variable and getter/setter function

protected $updateRules = array();

public function getUpdateRules() 
{
return $updateRules;
}

public function setUpdateRules($set) 
{
$this->updateRules = $set;
}

Also add what is basically an alias of save

    public function amend( array $rules = array(), array $customMessages = array(), array $options = array(), \Closure $beforeSave = null, \Closure $afterSave = null )
    {
if(empty($rules)) {$rules=$this->getUpdateRules();}
        return $this->real_save( $this->, $customMessages, $options, $beforeSave, $afterSave );
    }

Using amend instead of update, ad update is already used in Eloquent.

andrew13 commented 11 years ago

Added those: https://github.com/Zizaco/confide/commit/4b0bb2cd1d00337c6cf5b498b95a67ba2f44ecd0

Use the amend function to update a user.

andrew13 commented 11 years ago

Need to add the check if a user has edited a unique field that field should be checked.

I'll do this by grabbing the rules array, finding the unique fields, then comparing two users, if unique fields are different then the updateRules array will get the unique param added to it.

This will happen within a separate function. Which means there will be two calls when updating.

$user->prepareRules($Auth::user(),$user);
//Save
$user->save($this->getUpdateRules());
// Or Amend
$user->amend();
richlove1 commented 11 years ago

Looking sensible Andrew :+1:

Zizaco commented 11 years ago

I'm not sure if this was addressed very well. I don't think the methods: amend(), prepareRules() and getRules() are a good idea. We could, inside the save/beforeSave, check if the user exists in the database (the original attribute i guess) and address the issues regarding the validation errors when updating.

Plus, I don't think is a good idea for us to try to "patch" an Ardent bug in Confide.

andrew13 commented 11 years ago

I'd 100% that patching a bug in Ardent within Confide isn't the best solution. However, the bug in Ardent has been there for sometime without action on it and not having a way to update within Confide is an issue. I'd welcome implementing a better solution.

Zizaco commented 11 years ago

I'm seriously thinking about droping Ardent use. We can do an simpler self-validating model that will address this or we can use Aware (that is now available for laravel4)

andrew13 commented 11 years ago

I'd vote for dropping it. I haven't used Aware. Is it more stable than Ardent?

andrew13 commented 11 years ago

Also, I'd disagree with checking if the user exists on each save request. For two reasons:

Simply because it unnecessary in most cases. As a developer I'd likely know whether I'm creating a user or modifying one. I'd rather save a query on save/update and use two different functions. (save/update) (Maybe we specify a different set of rules for the update function. Instead of using amend.)

Further, I'm not sure how we would differentiate save vs update. Checking if the user exists isn't good enough. That would effectively allow a user creating a user to update a user. If we added a flag to the save function, that's a possibility, but simply checking if a user exists isn't going to work.

Zizaco commented 11 years ago

What if we check if there is an id in the user? Id there is not, than we query for it's username OR email before saving it. It's one more query in the saving process, but maybe it's a worth one.

Zizaco commented 11 years ago

Done: d764178e42d6479729fe8870332b718390b3ae0e

:)