Authenticator refactor #1

Firesphere · 2017-06-01T06:02:59Z

No description provided.

Firesphere · 2017-06-01T06:06:18Z

src/Dev/FunctionalTest.php

@@ -104,6 +106,8 @@ protected function setUp()
        // basis.
        BasicAuth::protect_entire_site(false);

+        $this->logOut();


Tractorcow: Could just use $this->logOut();.

Firesphere · 2017-06-01T06:06:50Z

src/Dev/FunctionalTest.php

        }

-        $this->session()->inst_set('loggedInAs', $memberID);
+        $this->logIn($member);


Should we make this call logIn() on the current IdentityStore, rather than assuming session?
We should see if we can remove the session assignment and see if we can just rely on Security::setCurrentUser().

Done, calling the internal login method.

Firesphere · 2017-06-01T06:07:41Z

src/Dev/FunctionalTest.php

+     */
+    public function logOut()
+    {
+        $this->session()->inst_clear('loggedInAs');


@tractorcow
Should we make this call logOut() on the current IdentityStore, instead of assuming session?
In fact, for testing, maybe we just use Security::setCurrentUser(), and don't modify session / persistant state?
++++

No, we actually need to clear the session for some tests, thus I would prefer to keep it.

As per below the plusses

Firesphere · 2017-06-01T06:08:52Z

src/Forms/FormRequestHandler.php

@@ -228,23 +228,23 @@ public function httpSubmission($request)
            // First, try a handler method on the controller (has been checked for allowed_actions above already)
            $controller = $this->form->getController();
            if ($controller && $controller->hasMethod($funcName)) {
-                return $controller->$funcName($vars, $this->form, $request);
+                return $controller->$funcName($vars, $this->form, $request, $this);


If you add a 4th argument to the request handler, please make sure to add it consistently to each of the other callbacks (there are 3 other places $funcName is called).

Firesphere · 2017-06-01T06:10:26Z

src/Security/Member.php

@@ -57,7 +59,7 @@
 * @property string $DateFormat
 * @property string $TimeFormat


Later in this file, Member::logOut() doesn't do a proper logout through the current IdentityStore; But tries to manually set cookies / session.

Fixed, with deprecation notice.

Firesphere · 2017-06-01T06:11:20Z

src/Security/Member.php

@@ -391,33 +393,6 @@ public function isLockedOut()
    }


First of all, I dislike the fact that a can* method returns a validationresult, and not a boolean, as per the convention we've set across the framework.

Secondly, I agree highly with @robbieaverill . canLogin() should only be used when trying to login, but isLockedOut() could be used in many other cases (e.g. an admin dashboard for a list of users), or a summary_fields() column in a group member table.

Making it protected just because we don't currently use it outside of that class isn't a very compelling reason. If it stands alone as a unit-testable method, and isn't used solely as a "can login" synonym (which it is not) then it should be kept public.

2 against 1. Okay :) Moved back to public.

Firesphere · 2017-06-01T06:11:46Z

src/Security/Member.php

@@ -447,59 +422,41 @@ public function isPasswordExpired()
    }

    /**
-     * Logs this member in
+     * @deprecated Use Security::setCurrentUser() or IdentityStore::logIn()


You can use @deprecated 4.0..5.0 to declare the deprecation assignment version, as well as the target API removal date.

Firesphere · 2017-06-01T06:12:25Z

src/Security/Member.php

-        $this->extend('beforeMemberLoggedIn');
-
-        self::session_regenerate_id();
+        Deprecation::notice('5.0.0',


Please use the proper deprecation API, and ensure that your deprecation notice provides the user with the correct upgrade path.

Deprecation::notice("5.0.0", "This method is deprecated and now does not persist. Please use ");

I think this is done here?

Firesphere · 2017-06-01T06:13:10Z

src/Security/Member.php

-        if (!$id && !self::$_already_tried_to_auto_log_in) {
-            self::autoLogin();
-            $id = Session::get("loggedInAs");
+        if ($member = Security::getCurrentUser()) {


Please add deprecation API notice to the body of the method. See my other comment on how to add this (add phpdoc, target version, upgrade note).

This method is deprecated, but you are still calling it internally (e.g. from Member:: logged_in_session_exists()). If you deprecate any method make sure you rewrite all usages to avoid deprecated API.

I suggest to deprecate Member::logged_in_session_exists() as well

Agreed, deprecated. Also removed usages.

Firesphere · 2017-06-01T06:13:46Z

src/Security/Member.php

@@ -850,25 +704,16 @@ public function getValidator()
    /**
     * Returns the current logged in user
     *
+     * @deprecated use Security::getCurrentUser()


Please use Deprecation API for this method. (see earlier)

Firesphere · 2017-06-01T06:15:31Z

_config/security.yml

-SilverStripe\Security\MemberLoginForm:
+---
+Name: coresecurity
+---


Can we please add a Name: coresecurity block to the header, so that users can extend this block using ordered overrides?

Firesphere · 2017-06-01T06:16:18Z

_config/security.yml

+    properties:
+      authenticators:
+        default: %$SilverStripe\Security\MemberAuthenticator\Authenticator


tractorcow 2 days ago Contributor

As I commented on SilverStripe\Security\AuthenticationRequestFilter, please shift these from injecting service names to injecting the services themselves directly.

setup getAuthenticators() / setAuthenticators() on Security class, and assign an array of named handlers.

My suggestion is that you make these basic getters / setters on a protected $authenticators property, and rename the current getAuthenticators($service) to getApplicableAuthenticators($service).
Firesphere 2 days ago Contributor

That would disable the option to override the default authenticator. If I implement my own authenticator and want to override, I have to awkwardly use the YML, where as

authenticators:
default: My\Authenticator\Class

Makes more sense to me. As this would also leave the option of a custom implementation, to fall back to the default authenticator (say I lost my FB/Twitter login, 2FA thing, etc.)
tractorcow a day ago Contributor

No, it wouldn't. you can key your array just as you have above, and you can override it with a like yml config block. All that you are changing is moving from injecting a class name to a class instance, and the config is moved up one level to the Injector top level key.

Done, took some annoying test-fixes though. But meh :D

Firesphere · 2017-06-01T06:16:50Z

_config/security.yml

+
+SilverStripe\Security\AuthenticationRequestFilter:
+  handlers:


Inject these dependencies directly: you can implement setHandlers() that takes an array of handlers (you can still key them with named identifiers), and move this up into the SilverStripe\Core\Injector\Injector configuration above.

Firesphere · 2017-06-01T06:17:14Z

_config/security.yml

+  handlers:
+    session: SilverStripe\Security\MemberAuthenticator\SessionAuthenticationHandler
+    alc: SilverStripe\Security\MemberAuthenticator\CookieAuthenticationHandler


This is already true in the private statics; We can omit the yml duplication here.

Firesphere · 2017-06-01T06:17:45Z

src/Security/IdentityStore.php

+     * @param HTTPRequest $request The request of the visitor that is logging in, to get, for example, cookies.
+     * @return HTTPResponse $response The response object to modify, if needed.
+     */
+    public function logIn(Member $member, $persistent = false, HTTPRequest $request = null);


Additionally, can we make $persistant and $request optional args? We may need to programmatically invoke logIn() for users where no HTTPRequest is available (e.g. unit tests).

Firesphere · 2017-06-01T06:18:07Z

src/Security/IdentityStore.php

+     * @param HTTPRequest $request The request of the visitor that is logging out, to get, for example, cookies.
+     * @return HTTPResponse $response The response object to modify, if needed.
+     */
+    public function logOut(HTTPRequest $request = null);


As above, can we please make $request optional argument?

Firesphere · 2017-06-01T06:18:34Z

src/Security/AuthenticationRequestFilter.php

+use SilverStripe\ORM\ValidationException;
+
+class AuthenticationRequestFilter implements RequestFilter, IdentityStore
+{


I would feel a lot more comfortable about splitting this into two classes; AuthenticationRequestFilter should implement only RequestFilter, but should take a core IdentityStore as a dependency (e.g. IdentityStoreChain, as it contains chained nested identitystore services).

@tractorcow
Although I agree, I don't think it's current form would allow for that. I would happily discuss it with you and see where we could get to. I don't see much possibility on that perspective here personally.

Firesphere · 2017-06-01T06:18:52Z

src/Security/AuthenticationRequestFilter.php

+    /**
+     * @return array|IdentityStore[]
+     */
+    protected function getHandlers()


See my comment on security.yml; Instead of trying to construct them here, simply have getHandlers() / setHandlers(), and DI a list of named services, configured via yaml.

Firesphere · 2017-06-01T06:19:40Z

src/Security/AuthenticationRequestFilter.php

+                    break;
+                }
+            }
+        } catch (ValidationException $e) {


tractorcow 2 days ago Contributor

We should probably catch only ValidationException; if the exception is already a HTTPResponse_Exception it should bubble up automatically.

This also allows us to get the validation error for the response.
Firesphere 2 days ago Contributor

ValidationException is currently an ORM exception, hence the comment. Happy to switch :)

Done, but might need it's own AuthenticationException?

Firesphere · 2017-06-01T06:20:15Z

src/Security/AuthenticationRequestFilter.php

+        }
+
+        Security::setCurrentUser($member);
+        // @todo Coupling here isn't ideal.


It's probably ok. :) we can remove this todo.

Firesphere · 2017-06-01T06:21:07Z

src/Security/Security.php

+    /**
+     * @var array available authenticators
+     */
+    protected static $authenticators = [];


Config must be private static not protected static.
However, I've suggested this should be changed from a config var to an injected dependency, so shall we type this as @var Authenticator[] and inject these instead?

As above, now injected. Do you still want it to be private @tractorcow?

Firesphere · 2017-06-01T06:21:40Z

src/Security/Security.php

+     * @var array available authenticators
+     */
+    protected static $authenticators = [];
+


Actually, default_authenticator isn't even used anywhere. Let's remove it entirely.

Firesphere · 2017-06-01T06:22:22Z

src/Security/Security.php

+    /**
+     * @var Member Currently logged in user (if available)
+     */
+    protected static $currentUser;


Positioning is awkward. Please fix.

Please use protected static; this will otherwise be treated as config.

Firesphere · 2017-06-01T06:22:49Z

src/Security/Security.php

+     * @return Authenticator Class name of Authenticator
+     * @throws LogicException
+     */
+    protected function getAuthenticator($name = 'default')


Shall we make arg $name = 'default'?

Firesphere · 2017-06-01T06:23:17Z

src/Security/Security.php

+     * @param int $service The type of service that is requested
+     * @return array Return an array of Authenticator objects
+     */
+    public function getApplicableAuthenticators($service = Authenticator::LOGIN)


You will need to change Security::getAuthenticators() to Security::singleton()->getApplicableAuthenticators(). We are trying to move away from static services to injected services.

Firesphere · 2017-06-01T06:23:43Z

src/Security/Security.php

+        $authenticators = static::$authenticators;
+
+        /** @var Authenticator $class */
+        foreach ($authenticators as $name => $class) {


Re-assignment of array value by-ref makes this code really awkward to understand. If you implement my above suggestion (inject service instances, not register service names) you can simplify this logic greatly.

Firesphere · 2017-06-01T06:26:24Z

src/Security/Security.php

-            Session::set('MemberLoginForm.force_message', 1);
-            $loginResponse = $me->login();
+            Security::singleton()->setLoginMessage($message, ValidationResult::TYPE_WARNING);
+            $loginResponse = Security::singleton()->login(new HTTPRequest('GET', '/'));


I suggest to make Security::login() method take all-optional arguments, meaning $request can be null. At the moment, the $request passed in is pretty much only used to detect a custom login arg (Security/login/cms) so it should be safe enough to make it optional.

Instead of creating a new object, should we just use the singleton (with all DI configured properly?).

I.e.

$loginResponse = Security::singleton()->login();

Firesphere · 2017-06-01T06:27:00Z

src/Security/Security.php

-     * @throws LogicException
-     */
-    protected function getAuthenticator()
+    public static function setCurrentUser($currentUser = null)


I suggest to type-cast the argument as a nullable Member instance.
public static function setCurrentUser(Member $currentUser = null)

Firesphere · 2017-06-01T06:27:51Z

src/Security/Security.php

     */
-    public function GetLoginForms()
+    public function getLoginForms()


Shouldn't this be

return $authenticator->getloginHandler($this->Link())->loginForm()

?

This method is ment to return a list of forms, not a list of HTTPResponses
Firesphere 2 days ago Contributor

It seems to be not even called anywhere anymore...
tractorcow a day ago Contributor

There are login forms that have multiple login methods (e.g. oauth / member login). It's ment to display multiple forms, grouped with tabs.

Not sure, I think getLoginForms is now delegated to delegateToHandlers (or whatever we called it). That builds a tabbed group of forms now.

Firesphere · 2017-06-01T06:29:18Z

src/Security/Security.php

+
+        if ($member) { // If we don't have a member, there's not much to log out.
+            /** @var Authenticator $authenticator */
+            $authenticator = $this->getAuthenticator('default'); // Always use the default authenticator to log out


Shouldn't this be

$authenticators = $this->getAuthenticators(Authenticator::LOGOUT);

Which is the point of exposing capability-gnostic authenticators?
+++
Yes, yes it should.

Firesphere · 2017-06-01T06:52:06Z

src/Security/MemberAuthenticator/LogoutHandler.php

+ * The logout process destroys all traces of the member on the server (not the actual computer user
+ * at the other end of the line, don't worry)
+ *
+ * @package SilverStripe\Security\MemberAuthenticator


We don't need @Package anymore.

Firesphere · 2017-06-01T06:52:57Z

src/Security/MemberAuthenticator/LostPasswordHandler.php

-        if ($this->performLogin($data)) {
-            return $this->logInUserAndRedirect($data);
-        }
+        $this->link = $link;


Changing it to link() is 100x less impact than changing it to getLink(). So we'll probably start with that.

I concur with @sminnee. Given that link($action) is a non-basic getter (it takes an argument), it doesn't fit the convention for get methods.

However, for this pull request, please keep consistent case, unless you are willing to rename the method framework-wide. :)

Leaving as is. It works for now, but worth looking into later.

Firesphere · 2017-06-01T06:54:06Z

src/Security/MemberAuthenticator/LostPasswordHandler.php

+            new FieldList(
+                new FormAction(
+                    'forgotPassword',
+                    _t('SilverStripe\\Security\\Security.BUTTONSEND', 'Send me the password reset link')


Namespaced localisation was incorrectly de-namespaced here

Done for every _t() I could find

Firesphere · 2017-06-01T06:54:42Z

src/Security/MemberAuthenticator/LostPasswordHandler.php

-        );
-        $changedPasswordLink = Security::singleton()->Link('changepassword');
-        return $this->redirect($this->addBackURLParam($changedPasswordLink));
-    }
 }


As above, let's just shift this to RequestHandler

Firesphere · 2017-06-01T06:55:17Z

tests/php/Security/MemberAuthenticatorTest.php

+    }
+
+    /* TO DO - reenable
+    public function testGenerateCMSLoginForm()


Re-enable this please. :D

What is it exactly supposed to do? The CMSLoginForm is horrible currently, but the test itself is a bit unclear on what it is trying to achieve @dhensby @sminnee @tractorcow @chillu

Firesphere · 2017-06-01T06:55:48Z

tests/php/Security/MemberAuthenticatorTest.php

        // If the user has never logged in, then the tempid should be empty
        $tempID = $member->TempIDHash;
        $this->assertEmpty($tempID);

        // If the user logs in then they have a temp id
-        $member->logIn(true);
+        Injector::inst()->get(IdentityStore::class)->logIn($member, true, new HTTPRequest('GET', '/'));


Make $request optional and let's stop passing in dummy objects.

Further down the line, I'm only returning the `Member` on the doLogin, so it's possible for the Handler or Extending Handler to move to a second step. Also cleaned up some minor typos I ran in to. Nothing major. This solution works and is manually tested for now. Supports multiple login forms that end up in the correct handler. I haven't gotten past the handler yet, as I've yet to refactor my Yubiauth implementation. FIX: Corrections to the multi-login-form support. Importantly, the system provide a URL-space for each handler, e.g. “Security/login/default” and “Security/login/other”. This is much cleaner than identifying the active authenticator by a get parameter, and means that the tabbed interface is only needed on the very first view. Note that you can test this without a module simply by loading the default authenticator twice: SilverStripe\Security\Security: authenticators: default: SilverStripe\Security\MemberAuthenticator\Authenticator other: SilverStripe\Security\MemberAuthenticator\Authenticator FIX: Refactor delegateToHandler / delegateToHandlers to have less duplicated code.

Authenticators is now a map of keys -> service names. The key is used in things such as URL segments. The “default_authenticator” value has been replaced with the key “default” in this map, although in time a default authenticator may not be needed. IX: Refactor login() to avoid code duplication on single/multiple handlers IX: Refactor LoginHandler to be more amenable to extension IX: Fixed permissionFailure hack his LoginHandler is expected to be the starting point for other custom authenticators so it should be easier to repurpose components `of it. IX: Fix database-is-ready checks in tests. IX: Fixed MemberAuthenticatorTest to match the new API IX: Update security URLs in MemberTest

Move to canLogin in the authentication check. Protected isLockedOut Enable login to be called with a different login service (CMSLogin), enabling CMS Log in. Seems the styling and/or output is still broken. logOut could be managed from the Authenticator instead of the member

NEW: Add IdentityStore for registering log-in / log-out data NEW: Add AuthenticationRequestFilter for managing login NEW: Add Security:setCurrentUser() / Security::getCurrentUser() NEW: Add FunctionalTest::logOut()

Repairing tests and regressions Consistently use `Security::getCurrentUser()` and `Security::setCurrentUser()` Fix for the logout handler to properly logout, some minor wording updates Remove the login hashes for the member when logging out. BasicAuth to use `HTTPRequest`

- Move the success and message to a validationresult - Fix tests for validationresult return - We need to clear the session in Test logOut method - Rename to MemberAuthenticator and CMSMemberAuthenticator for consistency. - Unify all to getCurrentUser on Security - ChangePasswordHandler removed from Security - Update SapphireTest for CMS login/logout - Get the Member ID correctly, if it's an object. - Only enable "remember me" when it's allowed. - Add flag to disable password logging - Remove Subsites coupling, give it an extension hook to disable itself - Change cascadeLogInTo to cascadeInTo for the logout method logic naming - Docblocks - Basicauth config

- Moved the Authenticators from statics to normal - Moved MemberLoginForm methods to the getFormFields as they make more sense there - Did some spring-cleaning on the LostPasswordHandler - Removed the BuildResponse from ChangePasswordHandler after spring cleaning

Firesphere commented Jun 1, 2017

View reviewed changes

Firesphere force-pushed the authenticator-refactor branch 11 times, most recently from c9055f9 to c8eb1cf Compare June 7, 2017 07:31

Sam Minnee and others added 5 commits June 7, 2017 21:11

NEW: Add AuthenticationHandler interface

f9ea752

NEW: Add IdentityStore for registering log-in / log-out data NEW: Add AuthenticationRequestFilter for managing login NEW: Add Security:setCurrentUser() / Security::getCurrentUser() NEW: Add FunctionalTest::logOut()

Firesphere force-pushed the authenticator-refactor branch 2 times, most recently from 19f0e57 to 2a76c8e Compare June 7, 2017 09:30

Firesphere force-pushed the authenticator-refactor branch from 2a76c8e to 082db89 Compare June 8, 2017 05:51

Firesphere force-pushed the authenticator-refactor branch from 7322875 to 5fce330 Compare June 8, 2017 08:10

Cleanup and RequestFilter refactor

62753b3

Firesphere merged commit 62753b3 into master Jun 10, 2017

		@@ -57,7 +59,7 @@
		* @property string $DateFormat
		* @property string $TimeFormat


		SilverStripe\Security\AuthenticationRequestFilter:
		handlers:

Authenticator refactor #1

Authenticator refactor #1

Conversation

Firesphere commented Jun 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Firesphere Jun 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Firesphere Jun 1, 2017 •

edited