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

Add routing & dependency injection support for easier coding and testing #966

Merged
merged 33 commits into from Nov 20, 2018

Conversation

Projects
None yet
3 participants
@jaimeperez
Member

jaimeperez commented Oct 17, 2018

This PR adds support for routing (automatically dispatching requests to controllers based on the URL) and dependency injection, so that coding and testing are easier and self-contained. This includes (roughly, and so far), the following changes:

  • Bump the minimum PHP version supported to 5.5.
  • Add some symfony components as a dependency.
  • Add support for Twig 2 (coexisting with Twig 1).
  • Make the new user interface and the old one completely independent. Enable the new one by setting the usenewui configuration option to true.
  • Twig templates are not used unless usenewui is set to true or the template belongs to a theme. This allows us to have the two user interfaces coexist, while avoiding breaking themes that have already been migrated to twig.
  • Modules can now add a routes.yaml file to the root of the module, where the different routes are defined and mapped to a method in a class. The class is completely customisable, and the URLs are paths relative to the module's root. E.g.: /bar in modules/foo/routes.yaml is translated to module.php/foo/bar.
  • The SimpleSAML\XHTML\Template class is now a Response object (PSR-7 compatible), meaning you can now use it as a response that you can send().
  • Fully independent new user interface. When usenewui is set to true, visiting the root of the SimpleSAMLphp installation gets you redirected to module.php/core/login. If there is more than one authentication source defined (excluding admin), the list of them is shown, so that the user can choose. If there is only one (or the user chooses one), authentication is started with that auth source. After authentication succeeds, the user is redirected to module.php/core/account/<auth_source_id>, where the user can see his own attributes, session length, etc. We might still want to change this page a bit, and it should allow us to include links to other pages relevant to the user (e.g. consent management). Finally, the module.php/core/logout/<auth_source_id> allows us to log out from the given auth source.
  • The controller classes are defined as autowired services in Symfony. This means they can have certain (type-hinted) arguments in their constructor, so that they get their dependencies automagically injected. So far, SimpleSAML\Configuration and SimpleSAML\Session are the only two supported types of arguments. This way, controller classes no longer need to manually get the instances, so that we can use the configuration and session objects we want while we are testing.

jaimeperez added some commits Oct 5, 2018

Initial version of routing, requests and responses.
This introduces the following:

- The use of Request objects to handle request data to controllers.
- The use of Response objects to model responses that should be sent to the browser.
- The use of "controllers" that are responsible for translating a request into a response.
- The possibility to define your own URLs on each module by specifying them, together with their controllers, in a "routes.yaml" file in the root of a module.
- The new UI is completely separated from the old, so "usenewui" must be set to "true" in the configuration.
- Twigified templates are not used unless we're using the new UI, or the twig template is part of a theme.
Stop using the deprecated TemplateLoader::parseName() method.
It was deprecated in Twig 1.x and removed in 2.0.
'svg' => 'image/svg+xml',
'svgz' => 'image/svg+xml',
'swf' => 'application/x-shockwave-flash',
'swfl' => 'application/x-shockwave-flash',

This comment has been minimized.

@tvdijen

tvdijen Oct 17, 2018

Member

Do we need to include the flash MIME-types?

This comment has been minimized.

@jaimeperez

jaimeperez Oct 17, 2018

Member

I just moved the code from the original module.php file, but yes, I guess we could probably get rid of those.

jaimeperez added some commits Oct 17, 2018

Make SimpleSAML\Session and SimpleSAML\Store implement the ClearableS…
…tate interface.

This way we can clear their state as well between tests, not only the configuration. This allows for richer functional testing.
Add a method to get the (normalized) template name from a XHTML\Templ…
…ate object.

This is useful when testing.
Add a type of Response that allows us to run a callback.
This allows us to use the router even with old code that won't return an actual Response object, but perform a redirection or print directly to the standard output.
Start using the RunnableResponse.
Now we can test properly the controllers executing old code.
Pass an instance of SimpleSAML\Configuration and SimpleSAML\Session a…
…s optional parameters to SimpleSAML\Auth\Simple.

This allows us to inject both as dependencies of the class.
Create an AuthenticationFactory that allows us to create SimpleSAML\A…
…uth\Simple objects.

This is needed to allow dependency injection in that class. Since the first parameter of the constructor is the ID of the auth source to use, we don't know the value it must take during compilation of the container. We only know what auth source to use when we are in the controller, becase at that point the URL as been parsed and the auth source ID has been passed to the controller as an argument. Therefore, we need to add this factory class as a service to the container, and once we have the auth source ID, we can call the create() method of the factory to obtain a relevant instance of \SimpleSAML\Auth\Simple.

jaimeperez and others added some commits Oct 17, 2018

@jaimeperez

This comment has been minimized.

Member

jaimeperez commented on b4f31fc Oct 18, 2018

Thanks for this fix Tim! Much easier and simpler than the one I was working on 😅

In any case, we'll need to extend DI and routing to all the classes, also utility ones like \SimpleSAML\Utils\HTTP, which is what I've been working on...

This comment has been minimized.

Member

tvdijen replied Oct 18, 2018

Ah ok 😅 Sorry
I didn't know you were going to put all that in this PR

This comment has been minimized.

Member

jaimeperez replied Oct 18, 2018

No worries! I shouldn't, actually, but since everything is related, I ended up doing so...

Shall we merge the PR into master?

This comment has been minimized.

Member

tvdijen replied Oct 18, 2018

Yes please! I think we have all 1.17 milestone activities done then

@thijskh

This comment has been minimized.

Member

thijskh commented Oct 19, 2018

I've committed the PHP version bump separately. So you might want to rebase this PR to current master.

@thijskh

This comment has been minimized.

Member

thijskh commented Nov 19, 2018

Merged with current master & conflicts resolved

@tvdijen

This comment has been minimized.

Member

tvdijen commented Nov 19, 2018

Show resolved Hide resolved lib/SimpleSAML/Module.php Outdated
@@ -41,10 +41,15 @@
"whitehat101/apr1-md5": "~1.0",
"twig/twig": "~1.0 || ~2.0",
"gettext/gettext": "^4.6",
"jaimeperez/twig-configurable-i18n": "^2.0"
"jaimeperez/twig-configurable-i18n": "^2.0",

This comment has been minimized.

@thijskh

thijskh Nov 20, 2018

Member

Should we move this to simplesamlphp namespace or something else with shared responsibility? As to prevent blockers if one person is not available.

This comment has been minimized.

@jaimeperez

jaimeperez Nov 20, 2018

Member

Yep, we should definitely do that.

Originally this was a really crappy solution, expecting the issue to be fixed upstream in twig. However, they don't seem to understand the issue or care enough about it, so I guess we'll depend on this for a while. In that case, it should definitely go away from my namespace and be in the SSP namespace instead.

* Contributed by Travis Hegner.
*/
$script = "/$module/$url";
if (stripos($request->getScriptName(), $script) === false) {

This comment has been minimized.

@thijskh

thijskh Nov 20, 2018

Member

ipos? or better just pos?

This comment has been minimized.

@jaimeperez

jaimeperez Nov 20, 2018

Member

Nice catch! Fixed as well 😄

{
$pathInfo = $request->getPathInfo();
$url = str_replace($pathInfo, rtrim($pathInfo, ' /'), $request->getRequestUri());
return new RedirectResponse($url, 308);

This comment has been minimized.

@thijskh

thijskh Nov 20, 2018

Member

Nice to use the 'correct' http response code!

thijskh and others added some commits Nov 20, 2018

Remove language.i18.backend setting; make each templating system use …
…its own i18n backend.

Twig will always use gettext, legacy will use legacy. Twig does not
work with legacy so new ui would break if you would not also set this
option. Instead, just switch the system in use based on the usenewui
config variable.

@thijskh thijskh merged commit 5bb7565 into master Nov 20, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@thijskh thijskh deleted the experiment/routing branch Nov 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment