Use the `Forwarded-For-*` headers for proxy

As remarked in #2887207-3: Improve support for ADFS, I'm currently still of the opinion that we should probably add OneLogin options to the admin screen, rather than make that generally extensible by modules. (Unless someone comes up with a great way of 'altering' the form and the settings.)

So thanks for doing this.

I think this 'global' setting inside the constructor is indeed the way to go, because of how the OneLogin SAML toolkit has coded things. However I'll wait with committing this, for the opinion of nathandentzau who's working on testability.

I'm getting errors when trying to apply this patch

I'm using 8.x-2.0-alpha1

Git apply was skipping:

Andres-MBP:samlauth andreperazzi$ git apply -v patch 
Skipped patch 'src/Form/SamlauthConfigureForm.php'.
Skipped patch 'src/SamlService.php'.

This is what I was getting with patch:

Andres-MBP:samlauth andreperazzi$ patch -p1 < p.patch 
patching file src/Form/SamlauthConfigureForm.php
Hunk #1 succeeded at 281 with fuzz 2 (offset -136 lines).
Hunk #2 succeeded at 361 with fuzz 2 (offset -162 lines).
patching file src/SamlService.php
Hunk #1 succeeded at 14 (offset -1 lines).
Hunk #2 FAILED at 96.
1 out of 2 hunks FAILED -- saving rejects to file src/SamlService.php.rej
Andres-MBP:samlauth andreperazzi$ mamp src/SamlService.php.rej 

So I went there and manually added the lines to SamlService.php

I found that the line $this->privateTempStore = $temp_store_factory->get('samlauth'); was not there, so I added the code after
$this->eventDispatcher = $event_dispatcher;

So I guess 8.x-2.0-alpha1 is conflicting with this patch.

This code should be merged to the code. It's really helpful and solved my problem when running this SAML module in IBM Bluemix, since it proxies the app.

I was having the same issue and thought of writing this same exact patch. https://github.com/onelogin/php-saml/issues/290#issuecomment-367619732. Then I tried to configure drupal revers proxy which usually takes care of the headers forwarding. So I did the following:

//condition to tell me that I am on a certain env with

if ( some_env_variable) {
  $settings['reverse_proxy'] = TRUE;
  $settings['reverse_proxy_addresses'] =  array($_SERVER['REMOTE_ADDR']);
}

and if you're proxy is doing the ssl termination you may add

if (PHP_SAPI !== 'cli') {
    // Fix HTTPS since we're behind load balancer.
    $_SERVER['HTTPS'] = 'on';
    // This is hardcoded because there is no header specifying the original port.
    $_SERVER['SERVER_PORT'] = 443;
  }

Doing this made it all work for me without adding the \OneLogin_Saml2_Utils::setProxyVars(TRUE);

However, I would like to know is there a way to actually extend the module to add \OneLogin_Saml2_Utils::setProxyVars(TRUE); or other options as needed? Perhaps this can be re-written to allow developers to modify and add extra properties?

Sorry for leaving this hanging for so long. The "working on testability" that I mentioned in October 2017, still has not materialized and this will likely not influence it. (I doubt we're going to be making automated tests for this specific feature, so we won't care about this global call.)

Now committed, although with a different setting name.

@awm

I would like to know is there a way to actually extend the module to add \OneLogin_Saml2_Utils::setProxyVars(TRUE); or other options as needed? Perhaps this can be re-written to allow developers to modify and add extra properties?

Your question is broad ranging / can be interpreted in multiple ways.

Broadly speaking there is definitely a way to extend the module yourself do to this. Services in Drupal can be overridden, so you can implement a child class and extend the constructor.

But if you mean rewriting this module for a generic approach to make it easier to add options to the configuration UI:
1) I doubt that, because there is no generic way in which the SAML Toolkit library handles 'options'. It has a settings object, which is a multi-dimensional array; then there are alterable parameters to the login() / processSLO() / etc methods, and then there are other global methods to call like \OneLogin_Saml2_Utils::setProxyVars(TRUE). This is hard to generalize.
2) I also have doubts about the usefulness of this approach; in my mind having a well thought out settings UI / layout / descriptions, rather than making it generic, can really help people. And this is possible because the amount of configurable options/calls which the library has, is finite, and isn't evolving that fast.

Obviously that still doesn't help if applying patches takes two and a half years...

I considered adding your proposed text to the README, but then saw on the Drupal docs page that the suggested approach (adding $_SERVER['REMOTE_ADDR']) is explicitly discouraged. I opted for linking to the Drupal docs page from the UI instead.

I hope there's no issue with the fact that I applied the fix to 8.x-3.x only.

(The usage stats reveal that there is still a fairly large amount of 8.x-2.x
users, but it's really easy to switch; there's no upgrade path.)

FYI:

I suspect that the 'samlauth_proxy_vars' (i.e. calling \OneLogin_Saml2_Utils::setProxyVars(TRUE);) and any other settings.php configuration is unnecessary if we add the 'baseurl' setting, as the D7 version has done in https://www.drupal.org/commitlog/commit/79214/3668c3021a0455ae9d2b925006... .

I haven't tested it. If somebody feels like testing that, that would be cool. If/when a 4.x branch gets opened, this option is a candidate for removal.

There is now a checkbox in the configuration UI (of 8.x-3.1) that bypasses the 'samlauth_proxy_vars' option. The aim is that you can test this, before 'samlauth_proxy_vars' gets removed in v4.