Fix: Hash passwords when 'client_secret' exists in POSTed data#34
Fix: Hash passwords when 'client_secret' exists in POSTed data#34rchavik merged 1 commit intothomseddon:masterfrom
Conversation
|
Ouch, this actually applies to other models as well: RefreshToken, AccessToken, etc. What do you think of having a behavior to do this instead? Any suggestion for the behavior name? |
|
Good find, this is a pretty glaring oversight and is definitely not the intended behaviour. My opinion would be that it probably doesn't warrant a behaviour, especially as each model hashes a different field. Would be good to get this + the same fix for the other two models in right away. |
|
Thom, It might be useful to do this in behavior, because then we can implement the fix in beforeSave() and the implement auto-hashing in beforeFind() like discussed in #33. What do you think? |
|
Well let's see what it looks like as a behaviour.... |
|
Thom, I've updated the PR for now, but please don't merge it yet as this was done while I was somewhat intoxicated :) |
The behavior hashes configure fields when they exists in POSTed data Without this check, 'secret fields' will be 'corrupted' for some cases. For eg: when updating redirect_uri (without specifying existing secret): $this->Client->save(array( 'id' => 'foo', 'redirect_uri' => 'http://mysite.com', )); will cause 'client_secret' to be populated with a hash of an empty string. The behavior also implements a beforeFind() method, that removes the need to manually hash field in $conditions
|
Okay.. i think this is now ok. (forgot to update this PR) |
|
I plan to merge this to master, and then start a separate Comments? |
|
Seems OK to me, although I haven't tested - good work on starting to build in the tests, would be very very good to ensure consistency in the "next" branch |
|
Ok. Merged. I'm prepared the next branch. Currently, I've merged them in my fork. I'll get it upstream tomorrow evening. |
2 participants
Without this check, client_secret will be 'corrupted' in some cases, eg:
when updating redirect_uri (without specifying existing secret):