[WIP] [BC BREAK] Refactor of authentication layer #554

adamlundrigan · 2015-01-16T04:16:01Z

Why?

In a recent project I aimed to build a ZfcUser-based app to be used in both console and API (Apigility) contexts, and the authentication layer in it's current form is not well suited for either.

AuthenticationChainEvent internalizes a copy of the Request object
Db auth adapter uses getPost to pull user input from AuthenticationChainEvent::$request [Unable to use ZfcUder from console due to Authentication\Adapter\Db using getPost on $request #541]
Db auth adapter assumes it's working with a PHP session [Authentication adapter should not know anything about authentication storage #513 Session is started by this module even in CLI mode #518]
Db auth adapter assumes it's working with Bcrypt (not as big a deal as the others)

All of the above necessitated a partial rewrite of the Db auth adapter for our particular use case. As a more permanent fix I'd like to suggest the PR'd modifications to the authentication layer for ZfcUser 2.x.

This code is in no way finished and still needs additional work (see TODO below). Please dissect at your leisure and insult/nitpick/praise/rewrite to your heart's content ;)

Overview of changes

V2

AdapterChain now triggers some events:
- attach
- authenticate.pre
- authenticate.success
- authenticate.failure
Added event listener to regenerate SID on authenticate.pre when Session storage is being used

V1

Rewrote the AdapterChain functionality.
- Now just a proxy to an internalized priority list of standard Authentication adapters.
Removed knowledge of Storage from adapters, as storage is handled by AuthenticationService.
TODOs / notes
- AdapterChain no longer triggers any events. May be a good idea to put it back in.
- Ability to perform tasks on logout from the adapter has been removed. Is this something people use?
- Throw an exception (here) if there are no registered adapters?

V0

Modified AuthenticationChainEvent to store identity and credential, instead of request
Renamed storage and adapter classes from Db to Mapper, a more apt description of their inner workings.
Adapter now takes a mapper, a mapper method name, and a credential processor (Zend\Crypt\Password\PasswordInterface)
Split the SM factory into two: MapperUsernameFactory and MapperEmailFactory
Storage now takes a mapper and a backing storage object (session by default)
Rejigged the service manager initialization for storage and adapter
- Both now have factories instead of implementing ServiceManagerAwareInterface
- Added generic service aliases for both to make overriding the concrete implementation easier

TODO

~~Does adapter need to know about storage? I'm leaning towards moving the read from / write to storage somewhere else...but it's not really of top necessity now that storage is decoupled from session.~~ Done
Regenerating the session ID was inbuilt to Db auth adapter. I've removed it, but it needs to be implemented somewhere else that makes more sense (event listener attached by session storage factory?) Added as an event listener automatically when Session storage is in use.
Persisting the user's password hash if it has changed was removed from Db auth adapter. Is this something that's necessary~~, and if so where should we trigger it~~? We could autoregister an event listener on authenticate.success to handle this (Prototype here).
~~Fix the tests. The exposed behaviours of the authentication layer haven't changed, but the method of instantiation and list of collaborators has changed for a number of classes.~~ Done
~~Use Zend\Authentication\Adapter\DbTable\CallbackCheckAdapter?~~ Mapper adapter is now sufficiently simplified to negate this.

- Renamed from `Db` to `Mapper` - Added SM aliases for easier overriding (Mapper and Session)

- Externalize the password hashing and verification (CredentialProcessor) - Generalize Mapper adapter to work with any mapper find method - Provide factories for producing MapperUsername and MapperEmail adapters - Update dist configuration with new adapter name

adamlundrigan · 2015-01-16T04:20:07Z

git seems to think I deleted the Db files and created new Mapper files....ugh

Danielss89 · 2015-01-16T21:36:29Z

Awesome that you started refactor of the authentication system, i had that on my todo list too.
I want to move away from the way it is done now, by events, as it's a totally wrong usage of how events should be used.

adamlundrigan · 2015-01-16T21:54:17Z

👍 If you want to give me the gist of how you'd like to see it implemented I can take a stab at it.

ojhaujjwal · 2015-01-17T01:31:37Z

👍

- Now just a priority list of Zend\Authentication\Adapter\AbstractAdapter instances - Eliminated Storage from collaborator list (AuthenticationService handles this) - Eliminated some obsoleted classes (AbstractAdapter, AdapterChainEvent, ChainableAdapter, OptionsNotFoundException)

adamlundrigan · 2015-01-17T03:07:57Z

@Danielss89 I took a stab at simplifying the AdapterChain stuff. Internally it now just stores a priority list of standard authentication adapters and iterates over them on authenticate(). That's obsoleted pretty much everything from the old AdapterChain setup.

…List)

…Chain::authenticate

…torage backend is in use

(enabled for debugging, slipped through) [skip ci]

adamlundrigan · 2015-01-18T02:38:03Z

Travis test failure due to 5.3.x incompatibility in a test class (use $this inside an anonymous function). What's the target ZF2 / PHP version for ZfcUser 2?

Danielss89 · 2015-01-18T09:42:37Z

We aim for PHP version 5.5 and ZF 2.3

Danielss89 · 2015-01-18T09:43:19Z

Btw. i will go though this, i just don't have time until next week :) Probably next weekend.

adamlundrigan · 2015-01-24T23:24:00Z

I've added a prototype implementation of rehashing a user's password when required. I'm not 100% convinced it's the right approach so I've stashed it in a separate branch: adamlundrigan@98fbc53

Danielss89 · 2015-02-01T20:03:28Z

I'm going to look through code now and drop comments, haven't tested yet though.

Danielss89 · 2015-02-01T20:06:07Z

src/ZfcUser/Authentication/Adapter/AdapterChain.php

-
-        return $result;
+        $this->adapters = new PriorityList();
+        $this->adapters->isLIFO(false);


Please expand function name, LastInFirstOut?

That would be Zend Framework's problem: https://github.com/zendframework/zf2/blob/master/library/Zend/Stdlib/PriorityList.php#L172

Hahahaha yeah ok :D

Stdlib is a bit of a mess in that regard. SplPriorityQueue and SplQueue are wrappers around the SPL classes to make them serializable. PriorityQueue uses Zend\Stdlib\SplPriorityQueue internally to get around SplPriorityQueue's destructive iterator, while PriorityList is an entirely standalone implementation. PriorityList with LIFO=false is functionally equivalent to PriorityQueue, just implemented differently. PriorityList with LIFO=true is technically a priority stack, but there is no PriorityStack in Stdlib. The only difference between PriorityStack and PriorityQueue is how they break priority ties. As far as I can tell it's all necessitated by odd behaviours in the Spl classes. And all of this has absolutely nothing to do with this PR, I just got off-track but I spent the time writing it so I might as well post it.

I'm going to try to sub in \SplPriorityQueue to see if it will work for this use case. If not, I'll sub in Zend\Stdlib\PriorityQueue which is equivalent to what I have currently but will cause no future head-scratching over isLIFO(false).

nuxwin · 2015-12-20T11:00:33Z

Also, see my answer here about IDEA for chained authentication adapters:

#321 (comment)

adamlundrigan added 5 commits January 15, 2015 23:50

Update AdapterChainEvent to take identity/credential rather than request

a49a4a9

Simplified Authentication Storage

c8cd716

- Renamed from `Db` to `Mapper` - Added SM aliases for easier overriding (Mapper and Session)

In AdapterChainEvent, clear credential if identity is cleared

64d37c3

Allow setting Mapper auth adapter storage from constructor

96d5200

Danielss89 added Enhancement Refactoring labels Jan 16, 2015

Danielss89 added this to the 2.0.0 milestone Jan 16, 2015

adamlundrigan changed the title ~~[WIP] [RFC] Rewriting authentication layer~~ [WIP] [RFC] Refactor of authentication layer Jan 16, 2015

adamlundrigan added 6 commits January 17, 2015 14:14

Ensure that adapters are processed FIFO (LIFO is default for Priority…

4c6d1bd

…List)

Prevent returning results that are not instances of Result in Adapter…

6342b5b

…Chain::authenticate

Remove unused references to ModuleOptions in some factories

1e2365e

Update ModuleOptions to reflect new default auth adapter name

0440720

Fix the tests

e47a62d

CS fixes

084311f

adamlundrigan changed the title ~~[WIP] [RFC] Refactor of authentication layer~~ [WIP] [BC BREAK] Refactor of authentication layer Jan 17, 2015

adamlundrigan added 3 commits January 17, 2015 22:41

Update AdapterChain to trigger relevant events

4153c18

Add event listener which regenerates SID on authenticate if Session s…

038ac7a

…torage backend is in use

Remove HTML code coverage from phpunit.xml

774efa7

(enabled for debugging, slipped through) [skip ci]

Simplified AdapterChain by using EventManagerAwareTrait

fef538d

Update/Add docblocks were appropriate

49b53c7

This was referenced Jan 25, 2015

[RFC] Drop controller plugin and view helper #558

Open

[RFC] Simplifying the mapper layer #559

Open

Danielss89 reviewed Feb 1, 2015
View reviewed changes

adamlundrigan mentioned this pull request Aug 4, 2016

ZF3 Compatibility #642

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] [BC BREAK] Refactor of authentication layer #554

[WIP] [BC BREAK] Refactor of authentication layer #554

adamlundrigan commented Jan 16, 2015

adamlundrigan commented Jan 16, 2015

Danielss89 commented Jan 16, 2015

adamlundrigan commented Jan 16, 2015

ojhaujjwal commented Jan 17, 2015

adamlundrigan commented Jan 17, 2015

adamlundrigan commented Jan 18, 2015

Danielss89 commented Jan 18, 2015

Danielss89 commented Jan 18, 2015

adamlundrigan commented Jan 24, 2015

Danielss89 commented Feb 1, 2015

Danielss89 Feb 1, 2015

adamlundrigan Feb 1, 2015

Danielss89 Feb 1, 2015

adamlundrigan Feb 1, 2015

nuxwin commented Dec 20, 2015

[WIP] [BC BREAK] Refactor of authentication layer #554

Are you sure you want to change the base?

[WIP] [BC BREAK] Refactor of authentication layer #554

Conversation

adamlundrigan commented Jan 16, 2015

Why?

Overview of changes

V2

V1

V0

TODO

adamlundrigan commented Jan 16, 2015

Danielss89 commented Jan 16, 2015

adamlundrigan commented Jan 16, 2015

ojhaujjwal commented Jan 17, 2015

adamlundrigan commented Jan 17, 2015

adamlundrigan commented Jan 18, 2015

Danielss89 commented Jan 18, 2015

Danielss89 commented Jan 18, 2015

adamlundrigan commented Jan 24, 2015

Danielss89 commented Feb 1, 2015

Danielss89 Feb 1, 2015

Choose a reason for hiding this comment

adamlundrigan Feb 1, 2015

Choose a reason for hiding this comment

Danielss89 Feb 1, 2015

Choose a reason for hiding this comment

adamlundrigan Feb 1, 2015

Choose a reason for hiding this comment

nuxwin commented Dec 20, 2015