Skip to content

Now allowing Authsources to set validUntil and cacheDuration for Metadata.#1481

Open
tommy2d wants to merge 6 commits into
simplesamlphp:masterfrom
tommy2d:metadataMaxCacheMaxDuration
Open

Now allowing Authsources to set validUntil and cacheDuration for Metadata.#1481
tommy2d wants to merge 6 commits into
simplesamlphp:masterfrom
tommy2d:metadataMaxCacheMaxDuration

Conversation

@tommy2d
Copy link
Copy Markdown

@tommy2d tommy2d commented Jul 12, 2021

Now allowing generation of SP metadata XML that includes @validuntil or @cacheDuration at EntityDescriptor.

Comment thread modules/saml/docs/sp.md Outdated
@tvdijen tvdijen changed the title Now allowing Authsources to set maxCache and maxDuration for Metadata. Now allowing Authsources to set validUntil and cacheDuration for Metadata. Aug 24, 2021
Copy link
Copy Markdown
Member

@thijskh thijskh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to be changed according to the (later) changes to the SP metadata refactoring + unit test update so we keep coverage of the new code up to date.

Comment thread lib/SimpleSAML/Metadata/SAMLBuilder.php Outdated
if ($metadata['expire'] - time() < $this->maxDuration) {
$this->maxDuration = $metadata['expire'] - time();
if ($metadata['expire'] - time() < $this->cacheDuration) {
$this->cacheDuration = $metadata['expire'] - time();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what this code wants to achieve. There is an expiration in the metadata. So we whould likely set validUntil to that value I think? Not cacheDuration? In any case the docs should probably say how the option interacts with expire.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.. I think it's also the wrong setting being set here.. Our metadata-converter converts validUntil to expire.
I think it makes sense to drop the expire-option in favour of validUntil.

Comment thread lib/SimpleSAML/Metadata/SAMLBuilder.php Outdated
Comment thread modules/saml/www/sp/metadata.php Outdated
@tvdijen tvdijen force-pushed the metadataMaxCacheMaxDuration branch from a96e0bf to 575b81d Compare August 27, 2021 18:51
@tvdijen tvdijen changed the base branch from simplesamlphp-1.19 to master August 27, 2021 18:53
@tvdijen tvdijen force-pushed the metadataMaxCacheMaxDuration branch from 575b81d to ec3e5ec Compare August 27, 2021 19:19
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 27, 2021

Codecov Report

❌ Patch coverage is 60.00000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 40.05%. Comparing base (31a49a8) to head (3defd30).
⚠️ Report is 1743 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##             master    #1481   +/-   ##
=========================================
  Coverage     40.05%   40.05%           
- Complexity     3531     3532    +1     
=========================================
  Files           141      141           
  Lines         10596    10598    +2     
=========================================
+ Hits           4244     4245    +1     
- Misses         6352     6353    +1     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tvdijen
Copy link
Copy Markdown
Member

tvdijen commented Aug 27, 2021

I've rebased against master because the PR targeted the 1.19-branch and this is not something we can merge there. I've made a backup of the branch before I did.
@thijskh : Do you agree that the expire-setting has now become obsolete and should go?

@thijskh
Copy link
Copy Markdown
Member

thijskh commented Sep 4, 2021

There's still quite a lot of code that refers to expire when talking about remote entities I think. Are you referring to that or to the setting for hosted entities (does that exist?).

@tvdijen
Copy link
Copy Markdown
Member

tvdijen commented Sep 4, 2021

It does not exist for hosted entities I think, so for remote entities... validUntil is translated to the expire setting by our metadata converter and I think it makes sense to rename it to validUntil.. We have to update the metarefresh-module as well if we do so

@tvdijen tvdijen force-pushed the master branch 2 times, most recently from 7a53fc8 to d73ae47 Compare September 26, 2021 13:03
@tvdijen tvdijen force-pushed the master branch 2 times, most recently from e5c0e21 to d5616df Compare January 9, 2022 11:00
@tvdijen tvdijen force-pushed the master branch 3 times, most recently from 2e6ab04 to 32f9acc Compare November 23, 2022 18:24
@tvdijen tvdijen force-pushed the master branch 3 times, most recently from c743c9a to fc71570 Compare January 5, 2023 16:01
@tvdijen tvdijen force-pushed the master branch 5 times, most recently from 7b173cf to 3326beb Compare March 20, 2023 22:59
@tvdijen tvdijen force-pushed the master branch 2 times, most recently from 8ac729b to a16cf6e Compare April 25, 2023 08:33
@tvdijen tvdijen force-pushed the master branch 2 times, most recently from fc454de to 7ac76ae Compare May 3, 2023 08:31
@tvdijen tvdijen force-pushed the master branch 6 times, most recently from 29f7b69 to 1a911ce Compare May 12, 2023 16:07
@tvdijen tvdijen force-pushed the master branch 3 times, most recently from c7c8357 to fdbe001 Compare June 12, 2023 14:28
@tvdijen tvdijen force-pushed the master branch 8 times, most recently from 3b5f5ba to 96357ee Compare July 19, 2023 12:37
@tvdijen tvdijen force-pushed the master branch 4 times, most recently from 332c2bb to a1681a6 Compare August 2, 2023 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants