API Moving verification logic to a trait for easier re-usability #102
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ScopeyNZ
force-pushed
the
pulls/4/extract-login-logic
branch
from
af517de
to
beb2451
Compare
- Moves configuration for the required number of verified methods to the EnforcementManager - `StoreInterface` now requires a member. A store cannot be created without one - Parts of the code have been updated to handle the new `?StoreInterface` return type of `getStore` - Updated API responses to return `403` instead of redirects when session timeouts occur - Added a `LoginHandler` trait that provides methods for handling requests and responses to and from the MFA React app - `SessionStore` implements `Serializable` so it can be added to the session directly - Moved tracking of completed "verified methods" to `StoreInterface`. - Moved loading of existing `StoreInterface`s to the interface
ScopeyNZ
force-pushed
the
pulls/4/extract-login-logic
branch
from
beb2451
to
0b2a6c0
Compare
| @@ -666,7 +570,7 @@ protected function doPerformLogin(HTTPRequest $request, Member $member) | |||
| // These next two lines are pulled from "parent::doLogin()" | |||
| $this->performLogin($member, $data, $request); | |||
| // Allow operations on the member after successful login | |||
| $this->extend('onLoginSuccess', $member); | |||
| parent::extend('afterLogin', $member); | |||
There was a problem hiding this comment.
For public visibility, here's a summary of what we discussed internally with this change:
- We can't delegate to the parent method on success, because the member data is gone at this point. We overload it instead.
- The
afterLoginhook comes from the parent class, and I removed it thinking we didn't need it in NEW Add extension points for audit logging in method registration, login and skip points #103. - The
onLoginSuccesshook is something we added and don't actually need. - We're asking the parent to express the extension hook, because people may have listeners attached to the parent class rather than this child.
robbieaverill
approved these changes
Apr 23, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
StoreInterfacenow requires a member. A store cannot be created without one?StoreInterfacereturn type ofgetStore403instead of redirects when session timeouts occurLoginHandlertrait that provides methods for handling requests and responses to and from the MFA React appSessionStoreimplementsSerializableso it can be added to the session directlyStoreInterface.StoreInterfaces to the interface