Metadata cached beyond expiry

having the same issue here. The xml metadata we get at /saml/metadata route is generated by the required package onelogin/php-saml and can't be disabled here.

Here I have a patch that extracts the validUntil attribute from the returned XML and add an Expires header to the CachableResponse.

1. +++ b/src/Controller/SamlController.php
   @@ -160,7 +160,12 @@ class SamlController extends ControllerBase {
   +    $response->SetExpires($this->getValidUntil($metadata));
   

   this should be setExpires not SetExpires

2. +++ b/src/Controller/SamlController.php
   @@ -160,7 +160,12 @@ class SamlController extends ControllerBase {
   +    ¶
   
   @@ -435,4 +440,34 @@ class SamlController extends ControllerBase {
   +      ¶
   

   There's some whitespace here that can be removed

3. +++ b/src/Controller/SamlController.php
   @@ -435,4 +440,34 @@ class SamlController extends ControllerBase {
   +   * @return bool|\DateTime
   

   This needs a comment and a spare line between @return and @params - also, technically it doesn't return bool, there is no code-path that doesn't return a \DateTime

4. +++ b/src/Controller/SamlController.php
   @@ -435,4 +440,34 @@ class SamlController extends ControllerBase {
   +      $expires = \DateTime::createFromFormat("Y-m-d\TH:i:s\Z",$valid_until, new \DateTimeZone('UTC'));
   ...
   +    } catch(\Exeption $e) {
   

   There's some phpcs spacing issues here, need a space after the comman, and after the catch

Thanks @larowlan for the review. uploaded the patch with your review changes applied.

I totally agree with this issue summary, being able to disable it would be awesome, our IDP just stripped it out of the metadata on our behalf on Friday so I'm looking for this:) It's too bad but yeah onelogin doesn't have that option to disable it.

I'll apply this patch soon, one question, what is with the +6 hours line from now, is that arbitrary?

+1 looks good but would want to put something as a comment with the "+6 hours" before I commit this. Even if that is "arbitrary".

(Not officially RTBC'ing because not tried, only viewed this issue.)

Thanks for reviewing and applying the patch. The +6 hours is a totally arbitrary number less than the default +2 days validUntil.

First of all, thanks to all of you for raising and implementing this. I am learning more about SAML as I go along, through issues that are raised here.

Having said that... I am now reviewing the code in more detail, and...

The xml metadata we get at /saml/metadata route is generated by the required package onelogin/php-saml and can't be disabled here.

While this is true, we do perform the call to Settings::getSPMetadata() that generates the metadata, and it has a $validUntil parameter, so we are / can easily be the source of that expiration value. I'd rather we know the value by explicitly passing it, than know it by parsing the XML document that we just generated ourselves. Meaning: I'm hesitant to commit the current getValidUntil() method.

If we go that other way, we need a setting for the expiry time. This expands the scope of this issue, but it's useful. (It can solve the @todo at the very top of the patch - and the suggested "best" way in this issue's description, as much as possible, by setting expiry to e.g. 1 second.)

This means we'd have some work to do:

• Come up with good placement and description in the settings screen.
• Ideally: include some conversion from "seconds" to "more easily human readable time-period value" and back
• Do good default behavior. (I think we probably want to set the value really low for new installations with a hint to set it higher when you're done experimenting with SP configuration - but keep it at the default for existing installations that don't have the setting yet.)
• Think about (or guess) the practical application of the separate $cacheDuration parameter. (At this moment I don't understand why OneLogin would define its default Metadata::TIME_CACHED to be larger than Metadata::TIME_VALID. But regardless: if we let Drupal cache the response with a "Valid until" date, I think it follows that the $cacheDuration should be smaller than it is now.)

I'll first go take a look at some other things (attribute mapping, finally), but will circle back to this... after X-mas ish.

Leaving the patch here for review, for a while: I think it makes sense to have two configuration settings:

• Time period for the XML metadata 'validUntil'
• Cache response (checkbox; if yes, take 'validUntil' minus 10 seconds).

Meanwhile, I'll think about whether this should have tests. (Well, it should; the config screen and metadata route are not crippled by spaghettification or the need for a working IdP.) Or whether I'm going to just prioritize tagging a stable version, finally - and include this in it.

Ahem.

-      $metadata = $this->saml->getMetadata($metadata_valid);
+      $metadata = $this->saml->getMetadata(time() + $metadata_valid);

Well, the good news is, the metadata is one thing in this module which has we do already have tests for. So I don't need to feel guilty about not making them.

For the rest... after I looked it again, I

• changed value metadata_cache_response to metadata_cache_http
• added a default value for metadata_cache_http to the config/install YML
• changed the config form layout a bit

So the included patch is what will be committed.

...by which I mean, this included patch.

For our purpose, being able to disable the validUntil attribute entirely would be the best, then there are no synchronization issues at all.

Note that as of May 2024, per https://github.com/SAML-Toolkits/php-saml/pull/569 , php-saml now includes a parameter to exclude validUntil.