Make getPostLoginDestination / getPostLogoutDestination configurable

I think this is valid for the 2.x branch too.

We probably want a settings form exposed somewhere

Here is a new patch adding form fields to override the default settings. I also added a field for the logout URL (it was marked as a todo).

Please review :)

Looks great!
One question only: why are you using getUrlIfValidWithoutAccessCheck instead of getUrlIfValid? Maybe is worth commenting the reason in the code.

Not sure if you need access to the urls to enter the link. The access check is done when visiting the page anyway.

Looks great to me

There might be a external URL in de loginDestination. In that case we need to do a TrustedRedirectResponse().
Could you please check this updated patch?

I think you need to do this instead:

$safe_response = TrustedRedirectResponse::createFromRedirectResponse($response);

I actually didn't know that. Thanks!
New patch is attached..

In https://www.drupal.org/node/2794589 it was decided to merge in the external work from https://github.com/droath/samlauth/tree/8.x-2.x.
A new patch is attached to work with that version.

After some more testing I found that #11 doesn't work with internal paths, only external. So I guess #9 should be the one.
Some more discussion resulted in the github branch is not going to be used after all.
Hiding the patches..

Ok, there still was an error. "The controller result claims to be providing relevant cache metadata, but leaked metadata was detected."
Somehow the metadata was an issue when using a TrustedRedirectResponse.

I found a stackoverflow issue describing it: http://drupal.stackexchange.com/questions/187086/trustedresponseredirect...

I implemented the suggestion and it worked (altough I'm not sure what is really causing the error). If someone has any other suggestion, please let me know!

New patch is attached.

I think this feature could use an implementation where other modules may specify the destinations programmatically. For instance, I would like to achieve an redirect where user is taken back to the page it came from (assuming that login is available in many/all pages).

EDIT: I just realised that this can be done of course by overriding the original SamlService, but give this a thought anyway :)

The way we implemented this was by creating a controller in a custom module. The SAML Authentication module redirects to this URL after loggin in. After this, your custom controller can do whatever it wants.

Besides that I think a new event to alter this makes sense. Overriding the SAML Service or implementing a custom controller to influance the redirect URL shouldn't be neccesary.

Agreed. Also, the post login/logout destinations are not intrinsically connected to the SAML authentication process so having them in there is not necessary / it should be possible to 'override' this.

Only one piece of (standard or custom) code should govern the redirection. So as far as I know this is better suited for a plugin mechanism than an event. I haven't written a D8 plugin (provider) yet, maybe this could be the first time...

There are two things about this redirection issue.

1) About programmatically setting it:

I am wrong about this needing a plugin architecture. There is already a mechanism for specifying a destination: the Relay State which is supported by the SAML toolkit itself.

The $return_to parameter in SamlService::login() is a URL which is carried through to the acs() function (that is: on the next HTTP request after the user was logged in). But the existing code which set a useless default $return_to value, made this unclear.
In the latest commit, I have made the module pick up this url from the 'destination' parameter on the login() side, and on the acs() side this is used for redirection

I think this should be enough for doing things like taking the user back to the page it came from. Right?

2) About a configurable parameter (i.e. the patch above):

This could probably still be useful but it would be the default in case no '/saml/login?destination=' parameter was set.

So my questions are:

• is such a default still useful? (Or can we require the user to just provide a 'destination' parameter whenever they don't want to end up at the user profile?)
• do you think it would be useful to have a URL as a relative path with token replacement? Could that be useful for e.g. multilingual stuff or special user pages? Do you think that would cover everything (that is not covered by giving the 'destination' parameter)?

+1 to leverage relay state (maybe document a bit on how to use it? http://saml.xml.org/wiki/sp-initiated-single-sign-on-postartifact-bindings), but as far as I can understand it, there's such thing as an IDP-initiated SSO (http://saml.xml.org/wiki/idp-initiated-single-sign-on-post-binding) and maybe is useful to be able to override this?

Maybe instead of token replacement just providing a way to programmaticaly override this url would be a better option for really custom integrations.

Also +1 on the relay state. Although I don't think other developers using the module should be bothered with it.

About settings / changing the destination post login/logout. Yes I definitely think there should be a default. A destination parameter should override this, but should be optional. Besides that, having a event to alter the destination is a must have, since there could be complex use cases where users should go to a special page based on permissions/roles or other config. This is what events are for.

So in the end I think the following should be implemented:

• Fetch a default destination from the settings form, this could be with or without tokens, as long as we can also add a external URL
• Override the default destination with the value from the destination parameter
• Dispatch a event to alter the destination.

Here is a reroll of #15 adding form fields for login/logout redirects. I think we now only have to add the following:

• Allow token in the URLs
• Add a event to alter the redirect URLs

Maybe we could just add this for now and add followups for the remaining tasks?

Well... I think it's important for me to understand this, in order to get things right - because authentication is no toy.

Unfortunately making me understand this is hard ;-) I'm close-ish to the Stackoverflow answer about URL generation and metadata caching and the UrlGenerator but not 100% there.

I'm going all the way back to comment #9 here:

1. +++ b/src/Controller/SamlController.php
   @@ -130,10 +143,13 @@ class SamlController extends ControllerBase {
   -    return new RedirectResponse($url);
   +    $generated_url = $url->toString(TRUE);
   

   Sooo... instead of a string we are generating a GeneratedUrl and then we are setting that as a cacheable dependency. OK. Fine.
   Ehm..... We want to make sure to never cache this redirect response because we want to re-process every single incoming SAML assertion. Right?

   So shouldn't we be doing en explicit $generated_url->setCacheMaxAge(0); ? Or is that already done somewhere internally?

2. +++ b/src/Controller/SamlController.php
   @@ -130,10 +143,13 @@ class SamlController extends ControllerBase {
   -    return new RedirectResponse($url);
   +    $generated_url = $url->toString(TRUE);
   +    $response = new TrustedRedirectResponse($generated_url->getGeneratedUrl());
   +    $response->addCacheableDependency($generated_url);
   +    return $response;
   

   Question. Should we really allow redirecting to an external URL after successful authentication?

   I know that a SAML SP (ACS) in principle can authenticate/authorize you for doing lots of things, also on other sites - but this Drupal module's ACS does one very specific thing: it logs you in to the current Drupal site. It seems to me like the redirect URL should be restricted to within this Drupal site.

   (The logout URL is a different story. Also my gut feeling is that is less 'exploitable'.)

3. +++ b/src/SamlService.php
   @@ -348,10 +348,10 @@ class SamlService {
   -          'url' => Url::fromRoute('samlauth.saml_controller_acs', array(), array('absolute' => TRUE))->toString(),
   +          'url' => Url::fromRoute('samlauth.saml_controller_acs', array(), array('absolute' => TRUE))->toString(TRUE)->getGeneratedUrl(),
   

   This is a brain teaser for any code reader. I want a comment describing why this construct was chosen. Does the following comment accurately describe this?

   "toString() can influence cache metadata for the page that will be rendered in this HTTP request. toString(TRUE) does not influence anything internally, but returns any cache metadata to us in a GeneratedUrl object - which we can then ignore; we use just the URL string."

   It's longish, but I don't know how to further shorten it.

In the meantime, implemented simple token support (for tokens which are always present, like [site:*] and [current-user:*].

#24.1 To prevent caching we can use no_cache: TRUE on the routes.
#24.2 Redirecting to a external URL seems like a valid use case to me (I use this already). The module configuration is for admins only, so I don't think this is a big issue. In the end you can create a custom callback and redirect that as well, but we shouldn't have to do that.
#24.3 I think this is simple enough. The documentation for Url::toString() explains the rest. We can add a @see comment? "We need to stop the Url object from collecting bubbleable metadata. If we don't this causes metadata to leak when returning the redirect."

Nice work on the tokens!

#24.1 ah thanks! I wonder if the $response->addCacheableDependency() is still necessary now, though.

#24.2 You're right about this not being a big issue - I will talk to you to understand your use case, I guess.

#24.3 Thanks for thinking along. IMHO Url::toString() documentation doesn't tell me much useful, however. (More specifically: it does not tell me that some internal caching-metadata-related state is -apparently- affected by this call, if I _do not_ pass TRUE.)

By now we are only talking about the redirect, though. I would like to decouple this from the original issue, and work more on it. So I'm going to split that part out into #2863340: Fix/unify usage of redirects - to have a dedicated place for people to discuss things, if later someone discovers that something non-ideal has happened to the URL handling.

I use version of module: 8.x-2.0-alpha1 released 2 March 2017 and faced the problem described in this ticket https://www.drupal.org/project/samlauth/issues/2863975
I need patches are applied using a composer without conflicts,

    "patches": {
      "drupal/samlauth": {
        "Make destination configurable": "https://www.drupal.org/files/issues/samlauth-destination_configurable-2670118-26.patch",
        "Fix/unify usage of redirects": "https://www.drupal.org/files/issues/samlauth-redirects-2863340-3.patch",
        "Fix SLS": "https://www.drupal.org/files/issues/2863975-2.x.patch",
        "Error: Call to a member function processSLO() on null": "https://www.drupal.org/files/issues/error_call_to_a_member_function_on_null-2910257-3.patch"
      }
    },
    "composer-exit-on-patch-failure": true,
    "enable-patching": true,

therefore I created a patch samlauth-destination_configurable-2670118-26.patch from commit:: 2974232 2017-03-22 | Issue #2670118 by seanB, roderik, eric.duran7@gmail.com: Make getPostLoginDestination / getPostLogoutDestination configurable [seanpenn079]