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

Already on GitHub? Sign in to your account

DirectAuthenticate adapter #949

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
Member

dereuromark commented Nov 12, 2012

To manually log in a user I like this better than using AuthComponent::login($data) since you could potentially have unclean data you pass into login().
Using the adapter unifies the login - always using the same method and settings like "contain" etc.

Let me know if you think this is worth adding as a core class.
I would also be OK leaving it as a plugin solution.

@ceeram ceeram commented on the diff Nov 12, 2012

...Cake/Controller/Component/Auth/DirectAuthenticate.php
+
+/**
+ * Find a user record using the standard options.
+ *
+ * The $conditions parameter can be a (string)username or an array containing conditions for Model::find('first').
+ *
+ * @param array $conditions An array of find conditions.
+ * @return Mixed Either false on failure, or an array of user data.
+ */
+ protected function _findUser($conditions, $password = null) {
+ $userModel = $this->settings['userModel'];
+ list($plugin, $model) = pluginSplit($userModel);
+ $fields = $this->settings['fields'];
+
+ $user = parent::_findUser($conditions);
+ if (isset($user[$fields['password']])) {
@ceeram

ceeram Nov 12, 2012

Member

doesnt the parent already unset the pw?

@dereuromark

dereuromark Nov 12, 2012

Member

only if its passed - which it isnt here, of course. we pass the conditions array directly, hence no unset triggered.

@ceeram

ceeram Nov 12, 2012

Member

Sorry, you are right about that, i think we should always unset it in BaseAuthenticate if present in the userdata, not only if its in the conditions.

Member

ceeram commented Nov 12, 2012

Im not sure about this, this might be confusing for end users.
The way authcomponent wroks in 2.x with its adapters allows for easy adding you own adapters like this.
A pull request against http://github.com/ceeram/Authenticate would be happily merged ;)

Owner

markstory commented Nov 12, 2012

I'm really not comfortable with this. Someone is going to screw up using this and we'll be partly on the hook for it. This isn't really an authentication adapter, as it does zero authentication.

Member

dereuromark commented Nov 12, 2012

Same could be argued allowing $this->Auth->login($data) to directly login using $data = true etc;
Mine at least has the information in the name ("Direct" implies kinda what it does).

But @ceeram's idea makes more sense anyway.
Sounds like a good idea to PR to such an Authenticate plugin instead then ;)

Member

ceeram commented Nov 12, 2012

I suppose you mean Auth->login(), it is document that if you pass an argument to login() it will not authenticate.
login() with argument makes sense that it actually logs in without autenticating.
An authenticate adapter that logs in a user which is present in db could be usefull in some cases, but i agree with Mark it might confuse end users as it does not really autenticate any user provided data, its just a shortcut to login() some user by its username.

Member

dereuromark commented Nov 12, 2012

All right then.

Owner

markstory commented Nov 13, 2012

I'm less concerned about confusing people, and more concerned with having authentication classes that don't do any authentication. I think that is the root problem for me. Adding adapters to shortcut things when there are existing terse ways to accomplish the end goal also provides two ways to do things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment