Add PrimaryKeySessionAuthenticator #710
Conversation
| ->expects($this->exactly(2)) | ||
| ->method('check') | ||
| ->with( | ||
| ...static::withConsecutive(['Auth'], ['Auth']), |
There was a problem hiding this comment.
This is copy and paste from the other Session test.
| public function __construct(IdentifierInterface $identifier, array $config = []) | ||
| { | ||
| $config += [ | ||
| 'identifierKey' => 'key', |
There was a problem hiding this comment.
If you're inheriting from SessionAuthenticator how does identifierKey interact with fields?
What is key here? I don't see that as a field in the auth_users table that your tests use. Is it primarily to be aligned with the identifier configuration?
There was a problem hiding this comment.
key here maps to the dataField of the identifier, this is how they pass in the data.
There was a problem hiding this comment.
fields is unused afaik.
| Make sure to match this with a Token identifier with ``key``/``id`` keys: | ||
|
|
||
| $service->loadIdentifier('Authentication.Token', [ | ||
| 'tokenField' => 'id', // lookup for DB table |
There was a problem hiding this comment.
This feels like it could be risky when combined with other authenticators.
There was a problem hiding this comment.
Token field? thats the DB lookup field internally, that should be fine
It basically builds the conditions for the ORM resolver:
return $this->getResolver()->find($conditions);
The dataField is the one coming from the authenticator and possibly matching another identifier
There was a problem hiding this comment.
This feels like it could be risky when combined with other authenticators.
I have similar concerns. While our default configs might not have any config overlap, we should ensure there isn't a possibility of opening a vulnerability even with custom config and potentially end up with something similar to the algo config vulnerability that were found in various JWT libs.
I think we should first work of #711 to directly associate identifiers with authenticators and add this new authenticator only after that.
There was a problem hiding this comment.
#712 is open
But IMO those don't collide.
The deprecation in parallel together with new docs will make sure people move over to a safer approach.
While it should still work out for everyone so far even if on the deprecated approach.
dereuromark
force-pushed
the
3.x-session-auth
branch
from
a7a1c8c to
3825a48
Compare
Replaces #707
Ports dereuromark/cakephp-tinyauth#153 as direct class into this plugin.