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

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

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

adamlundrigan
Copy link
Contributor

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.

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
Copy link
Contributor Author

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

@Danielss89 Danielss89 added this to the 2.0.0 milestone Jan 16, 2015
@adamlundrigan adamlundrigan changed the title [WIP] [RFC] Rewriting authentication layer [WIP] [RFC] Refactor of authentication layer Jan 16, 2015
@Danielss89
Copy link
Member

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
Copy link
Contributor Author

👍 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
Copy link
Contributor

👍

 - 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
Copy link
Contributor Author

@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.

@adamlundrigan adamlundrigan changed the title [WIP] [RFC] Refactor of authentication layer [WIP] [BC BREAK] Refactor of authentication layer Jan 17, 2015
@adamlundrigan
Copy link
Contributor Author

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
Copy link
Member

We aim for PHP version 5.5 and ZF 2.3

@Danielss89
Copy link
Member

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

@adamlundrigan
Copy link
Contributor Author

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
Copy link
Member

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


return $result;
$this->adapters = new PriorityList();
$this->adapters->isLIFO(false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please expand function name, LastInFirstOut?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hahahaha yeah ok :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

nuxwin commented Dec 20, 2015

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

#321 (comment)

@adamlundrigan adamlundrigan mentioned this pull request Aug 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants