🇺🇸United States @jproctor

@srutheesh, was this successful?

This is a great improvement but it changes the API for an existing public method.

Can we make it backwards compatible and add a deprecation warning? I don’t see an easy way to do that other than changing the function name (remove class? spell out authentication or references?) but you may have another idea.

I like this without the extra configuration: otherwise we’re saying that some people who have _maintenance_access wouldn’t actually have access.

Thanks for your contribution!

You’re using an old version of the module. Please install version 4.2.1 without your proposed modification and try again.

Please show your configuration for both Account settings (https://yoursite.com/admin/config/people/accounts) and SAML SP Login (https://yoursite.com/admin/config/people/saml_sp/login).

That’s already true.

Look at the conditional on current line 166 (or your MR line 195): the SAML SP Login config option account_request_create_account is sufficient to create an account without administrator approval.

How is this different from ✨ Create drupal account for New SAML Authenticated users Fixed , which was released in version 4.2.1 → several months ago?

I can’t replicate this error using version 4.2.1 on Drupal 9.5.9.

In order for the login to succeed I did need to add a different cert for SAMLtest.id; I copied the IdP cert from https://samltest.id/download/#SAMLtest%E2%80%99s_IdP (the one that ends with i1iHTA==) and added it to the IdP config in addition to the one present in the metadata file, but it doesn’t seem like you got that far.

The two obvious questions are whether you provided your metadata to SAMLtest.id (I would expect something more like “not configured for this service” if you hadn’t), and whether it had expired by the time you did the test. Even if the certificate is valid for years, the OneLogin library sets the SP’s metadata to expire in 2 days; if you saved it to a file and uploaded that (instead of providing a live URL) to SAMLtest.id, it would likely fail. There’s a new configuration option (as of version 4.2.1) which lets you specify a longer duration (or have it match the certificate).

jproctor → created an issue.

Thank you; I have removed the whitespace from the file in 4.x-dev; the fix will be included in the next release.

Also this issue never got closed so I’m doing that now.

The typical attribute names for an IdP to send an email address are mail and urn:oid:0.9.2342.19200300.100.1.3, but email makes enough sense that I think we can add it.

jproctor → made their first commit to this issue’s fork.

The PHP-SAML 4.x versions remove support for PHP 7.3, which is still allowed by Drupal 9 (until it reaches end of life in a few months). We cannot require 4.x until we stop supporting Drupal 9, which will be next year at the earliest.

I’m running PHP-SAML 3.6.1 in PHP 8.1, so I don’t think there’s a serious compatibility issue, despite the release notes suggesting that isn’t added until 4.0.1. My guess, without chasing the code changes, is that this module does not touch any of the PHP-SAML code that needed to be updated. Do you have a specific need for the new $spValidationOnly feature introduced in PHP-SAML 4.1.0?

I agree we should work on allowing 4.x, which will mostly amount to editing composer.json and testing to make sure it doesn’t break anything.

We try to be very careful when we do something that overrides a core behavior. We considered the options and decided it was most consistent if the config option labeled “request an account” does not create an account when the core setting says that only administrators can create an account.

Version 4.2.1 → adds an option to automatically create an account for an authenticated user.

Thanks!

jproctor → made their first commit to this issue’s fork.

jproctor → created an issue.

I totally agree there should be a check, I was just wondering whether it needed to be a service that fired on every request. I was thinking we could do it inline in the IdP form and that would be good enough.

On consideration, that wouldn’t catch the case that SecKit is installed/configured after the IdP is already set up. Without looking I’m willing to bet there’s not a hook or event when SecKit’s config gets modified, so a service might actually be the only way to catch the problem.

You want to merge that last round of code cleanup 3335453 and tag the release? I can do it later this week but I’m swamped for the next couple days.

jproctor → created an issue.

Right, but we care about that because it generates the ACS URL in the <AuthnRequest>. Is there a post-login validation step that fails if OneLogin has the wrong URL (http) even if the right one (https) goes out to the IdP?

I do have one other question: do we need a service for this, or could we do it in the controllers where we initiate and consume the exchange with the IdP?

Here’s a screen shot showing the first pass at rearranging the form, with account options in a fieldset.

jproctor → created an issue.

Update: I guess I needed to stop staring at it for a little while.

Yes, Træfik was in the way, but I was able to get around it (and also specify the header correctly because ugh PHP), and now it seems to work regardless of whether I have the patch:

curl -H "X-FORWARDED-PROTO: https" http://localhost:57961/saml/drupal_login/example_idp

[assertionConsumerService] => Array
    (
        [url] => https://localhost:57961/saml/consume
        [binding] => urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST
     )

This looks like it should work, but I’m having a hard time testing it and I want to be sure I understand:

Given:

1. Reverse proxy server “www.example.com”
   • Has (for this example) IP address 10.1.2.3
   • Forwards all requests to http://drupal.example.com
   • Correctly sets HTTP_X_FORWARDED_PROTO to https when handing a secure request
2. Drupal server “drupal.example.com”
   • $settings['reverse_proxy'] = TRUE;
   • $settings['reverse_proxy_addresses'] = ['10.1.2.3']

When a request comes in to https://www.example.com/saml/drupal_login/example_idp, it forwards the request to http://drupal.example.com/saml/drupal_login/example_idp.

Without this patch, the <AuthnRequest> goes out with http://... in the useful URLs, but with this patch, it should be https://..., yes?

To test this, I enabled debugging on /admin/config/people/saml_sp and tried

curl -H "HTTP_X_FORWARDED_PROTO: https" http://drupal.example.com

I get the same output regardless of whether I have the patch in place:

[assertionConsumerService] => Array
    (
        [url] => http://d9.lndo.site/saml/consume
        [binding] => urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST
     )

It’s possible that my test environment — I’m doing everything on one machine and Drupal is running in a container — is not a sufficient way to test this because Docker or Træfik or something is getting in the way.

Okay, I rerolled the patch without the form validation and made an issue fork and merge request.

I have two questions:

1. Should it log something when the user logs out? It never has, but it feels like this might be useful.

2. The end of the function looks like this:

  // Force redirect if saml_request is not empty.
  if(!empty($saml_request)) {
    $redirect = new RedirectResponse($url);
    $redirect->send();
  }

Is it possible for $saml_request to be empty? What would happen?

My IdP isn’t set up for single logout, so I can’t easily test that this actually works.

jproctor → created an issue.

Reopening this in case the bot finds more things that need to be updated.