SS4 Compatibility #23

ScopeyNZ · 2018-07-02T23:53:41Z

This PR converts the RealMe module to work with SS4.

Currently it all works, there's some cleanup to do. It has been converted to work with the Authentication changes - removing the SecurityExtension class and adding in an Authenticator and LoginHandler.

Todo:

Finish filling in default translations (_t(KEY, 'default') - SS4 warns if you omit 'default')
Update documentation
More smoke testing
Sort out travis

resolves #21

robbieaverill · 2018-07-03T01:25:13Z

_config.php


+if (Director::isDev()) {
+    error_reporting(E_ALL ^ E_DEPRECATED);
+}


What's the reasoning for adding this?

Ah, I was going to remove that and document it. In development environments deprecation warnings are emitted. Unfortunately the php-saml library still uses mcrypt - deprecated in 7.1. So if you want to use this in 7.1, you'll probably want to add that for development.

I'll have to add <7.2 to composer.json too for the time being.

Right, or add ext-mcrypt as a Composer requirement and let devs make up their own mind about it on 7.2

robbieaverill

Awesome work! I've left a few comments that identify some patterns I've seen here and also some specific examples of things. Things like using deprecated config APIs can be updated in bulk (note tests like testConfigurationTemplateDir split the line up so may not be obvious when searching).

I haven't commented on the translations yet, but we'll need to do one last sync down from Transifex in the SilverStripe 3 branch, then remove the Transifex configuration from it (freezing it for SS3) and finally run the text collector on the SilverStripe 4 branch once this is merged and push the source file up to Transifex.

Can you please also add a PSR-4 autoloader to composer?

There'll be a few other things that you'll need to add:

phpcs config
phpunit config
updated Travis config
updated Scrutinizer config
new codecov.io config

Noted in your description that the docs need updating too, that's cool.

robbieaverill · 2018-07-03T01:25:40Z

_config/extensions.yml

@@ -1,6 +1,3 @@
-SiteTree:
+SilverStripe\CMS\Model\SiteTree:


Can you please add a name block to this? E.g.

--- Name: realmeextensions ---

robbieaverill · 2018-07-03T01:26:26Z

composer.json

@@ -17,13 +17,23 @@
    ],
    "require": {
        "composer/installers": "*",


We can probably remove this as an explicit dependency now - it's required already by framework and vendor-plugin

robbieaverill · 2018-07-03T01:26:49Z

composer.json

@@ -17,13 +17,23 @@
    ],
    "require": {
        "composer/installers": "*",
-        "silverstripe/framework": "~3.1",
+        "silverstripe/framework": "^4.0",
        "onelogin/php-saml": "~2.10"
    },
    "require-dev": {
        "phpunit/phpunit": "~3.7"


We'll be using ^5.7 for SS4 modules

robbieaverill · 2018-07-03T01:27:35Z

src/Authenticator.php

+ *
+ */
+class Authenticator implements AuthenticatorInterface
+{


It's worth adding the Injectable trait to this class, since you're defining dependencies that require the injector to bootstrap

robbieaverill · 2018-07-03T01:29:55Z

src/Authenticator/LoginForm.php

+     */
+    public function getAuthenticatorName()
+    {
+        return 'RealMe Login';


Does this need to be made translatable?

robbieaverill · 2018-07-03T02:30:18Z

tests/RealMeServiceTest.php

@@ -25,7 +34,7 @@ public function testGetCertificateContents()
        file_put_contents($path, $contents);


There's a couple of parts in this test that won't work in SS4:

Assets aren't always stored in the assets folder any more. You can use TestAssetStore to let you use a path in the assets folder though.

file_get_contents(BASE_PATH . '/realme/tests won't work any more because the module is now a vendor module, and when Travis runs these tests it's installed as the project, not a module. Instead you could use something like dirname(__FILE__) . '/certs/... so it's relative. Note that this would likely tie into how you implement the TestAssetStore as well.

robbieaverill · 2018-07-03T02:30:48Z

tests/RealMeServiceTest.php

@@ -75,21 +84,21 @@ public function testGetAuth()

    public function testGetAuthCustomSPEntityId()
    {
-        Config::inst()->update('RealMeService', 'sp_entity_ids', ['mts' => 'https://example.com/custom-realm/custom-service']);
+        Config::inst()->update(RealMeService::class, 'sp_entity_ids', ['mts' => 'https://example.com/custom-realm/custom-service']);


- Config::inst()->update( + Config::modify()->set(

robbieaverill · 2018-07-03T02:31:13Z

tests/RealMeServiceTest.php

-
-        define('REALME_CERT_DIR', BASE_PATH . '/realme/tests/certs');
-        define('REALME_SIGNING_CERT_FILENAME', 'standard_cert.pem');
+        Environment::putEnv('REALME_CERT_DIR', BASE_PATH . '/realme/tests/certs');


Will need to be updated to use a different file path

robbieaverill · 2018-07-03T02:32:49Z

tests/RealMeServiceTest.php

-        define('REALME_CERT_DIR', BASE_PATH . '/realme/tests/certs');
-        define('REALME_SIGNING_CERT_FILENAME', 'standard_cert.pem');
+        Environment::putEnv('REALME_CERT_DIR', BASE_PATH . '/realme/tests/certs');
+        Environment::putEnv('REALME_SIGNING_CERT_FILENAME', 'standard_cert.pem');
    }

    public function setUp()


setUp() is protected in PHPUnit 5.x/SilverStripe 4.x SapphireTest

robbieaverill · 2018-07-03T02:33:23Z

tests/RealMeServiceTest.php

-        Config::inst()->update('RealMeService', 'authn_contexts', ['mts' => 'urn:nzl:govt:ict:stds:authn:deployment:GLS:SAML:2.0:ac:classes:LowStrength']);
+        Config::inst()->update(RealMeService::class, 'sp_entity_ids', ['mts' => 'https://example.com/realm/service']);
+        Config::inst()->update(RealMeService::class, 'metadata_assertion_service_domains', ['mts' => 'https://example.com']);
+        Config::inst()->update(RealMeService::class, 'authn_contexts', ['mts' => 'urn:nzl:govt:ict:stds:authn:deployment:GLS:SAML:2.0:ac:classes:LowStrength']);


- Config::inst()->update( + Config::modify()->set(

ScopeyNZ · 2018-07-04T01:13:01Z

It's green! Yay! 🎉

robbieaverill

Down to nit picking =) nice

robbieaverill · 2018-07-04T01:50:17Z

composer.json

+    "autoload-dev": {
+        "psr-4": {
+            "SilverStripe\\RealMe\\Tests\\": "tests/"
+        }


You need to include the tests autoloader under the autoload definition

robbieaverill · 2018-07-04T01:56:45Z

src/RealMeService.php

-    {
-        return self::current_realme_user();
-    }
-


What's the justification for removing this? I assume this is one of the template helper methods

I considered refactoring out storing the current real me user in the service at one point, probably forgot to revert this method.

robbieaverill · 2018-07-04T01:57:01Z

src/RealMeService.php

@@ -359,88 +389,93 @@ public static function currentRealMeUser()
     * other unexpected authentication error. You can use {@link getLastError()} to see if a human-readable error
     * message exists for display to the user.
     *
+     * @param HTTPRequest $request


Also has a $backUrl

robbieaverill · 2018-07-04T02:02:10Z

src/Task/RealMeSetupTask.php

-            $this->message(PHP_EOL . _t('RealMeSetupTask.BUILD_FINISH', '', '', array('env' => $forEnv)));
+            $this->message(PHP_EOL . _t(
+                self::class . '.BUILD_FINISH',
+                'RealMe setup complete. Please copy the XML into a file for upload to the %s environment or DIA ' .


You could do this instead - following standard SS4 practice:

_t(self::class . '.BUILD_FINISH', 'RealMe setup complete. Please copy the XML into a file for upload to the {type} environment or DIA to complete the integration', ['type' => $forEnv]);

We're replacing the use of sprintf style translations wherever we find them: ref silverstripe/silverstripe-framework#6582

robbieaverill · 2018-07-04T02:02:49Z

src/Task/RealMeSetupTask.php

        } catch (Exception $e) {
            $this->message($e->getMessage() . PHP_EOL);
        }
    }

+    /**
+     * @param RealMeService $service


@return $this

robbieaverill · 2018-07-04T02:04:54Z

src/Task/RealMeSetupTask.php

-                'RealMeSetupTask.ERR_VALIDATION',
-                '',
+                self::class . '.ERR_VALIDATION',
+                'There were {numissues} issue(s) found during validation that must be fixed prior to setup: {issues}',


I think you need to remove the empty string argument here. Should be _t('ClassName.KEY', 'Default value {placeholder}', ['placeholder' => $dynamicValue])

Yep. So many of those I was bound to miss one 😖

robbieaverill · 2018-07-04T02:09:35Z

src/Task/RealMeSetupTask.php

-                . "can pass it on to RealMe Operations staff" . PHP_EOL . PHP_EOL,
+        $this->message(_t(
+            self::class . '.OUPUT_PREFIX',
+            'Metadata XML is listed below for the \'%s\' RealMe environment, this should be sent to the agency so they '


Please use a {type} placeholder or similar and pass it in the third argument as a keyed array

robbieaverill · 2018-07-04T02:10:02Z

src/Task/RealMeSetupTask.php

+                self::class . '.ERR_CONFIG_ENTITYID',
+                'The Entity ID (\'{entityId}\') must be https, not be \'localhost\', and must contain a valid ' .
+                'service name and privacy realm e.g. https://my-realme-integration.govt.nz/p-realm/s-name',
+                '',


This empty arg isn't needed. There are a few other occurrences of this but I won't keep listing them =)

robbieaverill · 2018-07-04T02:10:44Z

src/Task/RealMeSetupTask.php

-                '',
+                self::class . '.ERR_ENV_NOT_SPECIFIED',
+                'The RealMe environment was not specified on the cli It must be one of: {allowedEnvs} ' .
+                    'e.g. sake dev/tasks/RealMeSetupTask forEnv=mts',


- e.g. sake dev/tasks + e.g. vendor/bin/sake dev/tasks

robbieaverill · 2018-07-04T02:11:13Z

src/Task/RealMeSetupTask.php

        } elseif (!$this->isReadable($this->service->getCertDir())) {
            $this->errors[] = _t(
-                'RealMeSetupTask.ERR_CERT_DIR_NOT_READABLE',
+                self::class . '.ERR_CERT_DIR_NOT_READABLE',
                '',


This has an empty default value - probably needs to be defined as something

ScopeyNZ · 2018-07-04T03:40:33Z

Ok feedback addressed.

@madmatt I was talking to you about the removal of TRUSTED_PROXY. Previously this constant was used to flag the onelogin library to look at different server super-globals for determining the host, protocol and port. SS4 has replaced this constant with the TrustedProxyMiddleware which validates the proxy and then updates the request with the appropriate host and protocol. With that in mind I've just changed it so we set the host, protocol and port manually with the onelogin library: https://github.com/creative-commoners/silverstripe-realme/blob/pulls/3.0/upgrade-to-ss4/src/RealMeService.php#L864 . Works fine in my testing. Hopefully you can point out anything I might have missed here?

ScopeyNZ · 2018-07-05T21:51:37Z

The login endpoint has been playing up (on MTSs end) a little but I'm happy with the assertion endpoint.

ScopeyNZ · 2018-07-10T00:43:26Z

I have raised an issue at silverstripe/silverstripe-framework#8252 regarding the logic I've added to resolve the forwarded port in RealMeService.php. If framework is updated that code can be removed.

Points addressed

robbieaverill added affects/v4 type/api-break labels Jul 3, 2018

robbieaverill reviewed Jul 3, 2018

View reviewed changes

robbieaverill suggested changes Jul 3, 2018

View reviewed changes

ScopeyNZ force-pushed the pulls/3.0/upgrade-to-ss4 branch 4 times, most recently from 3876fc5 to fec8c10 Compare July 4, 2018 01:09

robbieaverill previously requested changes Jul 4, 2018

View reviewed changes

ScopeyNZ force-pushed the pulls/3.0/upgrade-to-ss4 branch from fec8c10 to 2b88762 Compare July 4, 2018 02:44

ScopeyNZ added 8 commits July 4, 2018 14:46

Updating SilverStripe framework version in composer.json

dcbbdc4

First pass SS4 compatibility

a8f8c85

Addressing feedback & updating documentation

9764073

Adding/updating various configs

0b14d51

Linting commit

33a6b4f

Fixing tests

3cd5d1a

Fixing tests pt.2

f134ca3

Updating translation calls to provide default translations

8f0ac9c

ScopeyNZ force-pushed the pulls/3.0/upgrade-to-ss4 branch from 2b88762 to 8f0ac9c Compare July 4, 2018 02:51

ScopeyNZ added 2 commits July 4, 2018 15:10

Updating translation sources/files

14caa64

Continued updates addressing feedback

b13b974

ScopeyNZ added 2 commits July 6, 2018 10:34

Minor touchups

004ef79

Fixing some inconsitencies with SS3 -> SS4 conversion

b604d38

ScopeyNZ self-assigned this Jul 6, 2018

ScopeyNZ changed the title ~~WIP: SS4 Compatibility~~ SS4 Compatibility Jul 9, 2018

NightJar mentioned this pull request Jul 10, 2018

Check and update module for SS4 compatibility where necessary #21

Closed

NightJar approved these changes Jul 10, 2018

View reviewed changes

robbieaverill merged commit e781c71 into silverstripe:master Jul 10, 2018

robbieaverill deleted the pulls/3.0/upgrade-to-ss4 branch July 10, 2018 02:31

ScopeyNZ removed their assignment Jul 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SS4 Compatibility #23

SS4 Compatibility #23

ScopeyNZ commented Jul 2, 2018 •

edited by NightJar

Loading

robbieaverill Jul 3, 2018

ScopeyNZ Jul 3, 2018 •

edited

Loading

ScopeyNZ Jul 3, 2018

robbieaverill Jul 3, 2018

robbieaverill Jul 3, 2018

robbieaverill left a comment •

edited

Loading

robbieaverill Jul 3, 2018

robbieaverill Jul 3, 2018

robbieaverill Jul 3, 2018

robbieaverill Jul 3, 2018

robbieaverill Jul 3, 2018

robbieaverill Jul 3, 2018

ScopeyNZ Jul 10, 2018

robbieaverill Jul 3, 2018

robbieaverill Jul 3, 2018

robbieaverill Jul 3, 2018

robbieaverill Jul 3, 2018

ScopeyNZ commented Jul 4, 2018

robbieaverill left a comment

robbieaverill Jul 4, 2018

robbieaverill Jul 4, 2018

ScopeyNZ Jul 4, 2018

robbieaverill Jul 4, 2018

robbieaverill Jul 4, 2018

robbieaverill Jul 4, 2018

robbieaverill Jul 4, 2018

ScopeyNZ Jul 4, 2018

robbieaverill Jul 4, 2018

robbieaverill Jul 4, 2018

robbieaverill Jul 4, 2018

robbieaverill Jul 4, 2018

ScopeyNZ commented Jul 4, 2018

ScopeyNZ commented Jul 5, 2018

ScopeyNZ commented Jul 10, 2018

		@@ -25,7 +34,7 @@ public function testGetCertificateContents()
		file_put_contents($path, $contents);

		@@ -1,6 +1,3 @@
		SiteTree:
		SilverStripe\CMS\Model\SiteTree:

SS4 Compatibility #23

SS4 Compatibility #23

Conversation

ScopeyNZ commented Jul 2, 2018 • edited by NightJar Loading

Choose a reason for hiding this comment

ScopeyNZ Jul 3, 2018 • edited Loading

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

robbieaverill left a comment • edited Loading

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

ScopeyNZ commented Jul 4, 2018

robbieaverill left a 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

ScopeyNZ commented Jul 4, 2018

ScopeyNZ commented Jul 5, 2018

ScopeyNZ commented Jul 10, 2018

ScopeyNZ commented Jul 2, 2018 •

edited by NightJar

Loading

ScopeyNZ Jul 3, 2018 •

edited

Loading

robbieaverill left a comment •

edited

Loading