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

Entity protected fields interferes with Digest Authentication #10855

Closed
1 of 3 tasks
SAMGARTNER opened this issue Jul 4, 2017 · 7 comments
Closed
1 of 3 tasks

Entity protected fields interferes with Digest Authentication #10855

SAMGARTNER opened this issue Jul 4, 2017 · 7 comments
Assignees
Milestone

Comments

@SAMGARTNER
Copy link

This is a (multiple allowed):

  • bug

  • enhancement

  • feature-discussion (RFC)

  • CakePHP Version: 3.3.15

  • Platform and Target: Mac OS X Yosemite With Server App MySQL.

What you did

I was trying to implement digest authentication and I have the Users table with the password field already being used for JWT so I added a second field called digest. Also inside the User Entity I added the field 'digest' to the Hidden Fields in order to hide this data from the API request.

protected $_hidden = [
        'password','mydigest'
    ];

What happened

When I try to get into the site I receive an error that points me to the line 115 of the DigestAuthenticate.php file . This happens even with the default 'password' setting.

$field = $this->_config['fields']['password'];
$password = $user[$field];

What you expected to happen

DigestAuthenticate relies on BaseAuthenticate that gets the user from the database but with the hidden fields stripped. In my case I had to replace

$user = $this->_findUser($digest['username']);

with

$users = TableRegistry::get('Users');
        $user = $users->find('all')
    	->where(['username' => $digest['username']])
    	->first();

Not sure if this is something desirable, of course I had to manually strip both 'password' and 'mydigest' from the user object but this brakes the settings pattern.

P.S. Remember, an issue is not the place to ask questions. You can use Stack Overflow
for that or join the #cakephp channel on irc.freenode.net, where we will be more
than happy to help answer your questions.

Before you open an issue, please check if a similar issue already exists or has been closed before.

@ADmad ADmad added this to the 3.4.10 milestone Jul 4, 2017
@ADmad ADmad added the auth label Jul 4, 2017
@markstory
Copy link
Member

Hidden fields are not elided from the database query results, and should be accessible to DigestAuthentication. Have you tried removing mydigest from the hidden fields? Does that resolve the problem?

@SAMGARTNER
Copy link
Author

Yes that was the first thing that I tried, but since the password field is being hidden by default it kinda creeped me out to remove it, since it's basically a second password. Of course DigestAuthenticate unsets the field but in all other parts the user object will have it. Not trying to pretend to be an expert here.

@markstory
Copy link
Member

@jigzat Even with the digest hash hidden, CakePHP should still be able to read the field. The hash check is done here and the array access methods on entities do not 'protect' direct property reads.

@ADmad
Copy link
Member

ADmad commented Jul 5, 2017

@markstory I think the issue is that _findUser() uses toArray() on the entity which would strip out any hidden fields. Hence the password and mydigest field are already gone from $user by the time execution reaches the lines you linked.

@markstory
Copy link
Member

@ADmad Yes that would make it not work. I totally missed that toArray() call before.

@markstory markstory self-assigned this Jul 5, 2017
@burzum
Copy link
Contributor

burzum commented Jul 6, 2017

@ADmad we'll probably need to fix this in the Authentication plugin as well?

markstory added a commit that referenced this issue Jul 6, 2017
When the digest key is a hidden field it will not be included in the
results of _findUser() which makes it impossible for users to
authenticate. In the case where there was no password comparison done,
and the passwordfield is a hidden field, removing the passwordField from
the hidden list allows digest auth to work. Digest auth subsequently
removes the password field after checking the key.

Refs #10855
@markstory
Copy link
Member

Pull request open now.

burzum pushed a commit that referenced this issue Jul 11, 2017
When the digest key is a hidden field it will not be included in the
results of _findUser() which makes it impossible for users to
authenticate. In the case where there was no password comparison done,
and the passwordfield is a hidden field, removing the passwordField from
the hidden list allows digest auth to work. Digest auth subsequently
removes the password field after checking the key.

Refs #10855
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants