Skip to content

IDP SAML2: AttributeConsumingServiceIndex implementation #649

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
tbenr wants to merge 6 commits into
base: master
Choose a base branch
from
Open

IDP SAML2: AttributeConsumingServiceIndex implementation #649

tbenr wants to merge 6 commits into from
+426 −39

Conversation

@tbenr
Copy link

@tbenr tbenr commented Jul 4, 2017 •
edited
Loading

I propose this PR for implementing IDP compliance to SAML2 service providers using different AttributeConsumingServices in their metadata, sending request using AttributeConsumingServiceIndex.

  1. new AttributeConsumingService parameter in saml20-sp-remote to hold Service specification
  2. SAMLParser.php has been modified to generate the new configuration param accordingly (metarefresh.php now generates this new param instead of simple attributes array).
  3. IdP/SAML2.php adds saml:AttributeConsumingServiceIndex in state variable.
  4. AttributeLimit.php, if AttributeConsumingService is defined in Destination metadata, uses saml:AttributeConsumingServiceIndex to resolve the attribute set corresponding to the requested Service. If not found, backward compatibility is maintained.
  5. simplesamlphp-reference-sp-remote.md has been updated.
  6. SAMLParserTest.php and AttributeLimitTest.php have been updated to cover code changes.

@johnmaguire

This comment has been minimized.

Sign in to view
@tbenr

This comment has been minimized.

Sign in to view
@johnmaguire

This comment has been minimized.

Sign in to view
fix for saml state null check
4fc6484 
added check if saml:AttributeConsumingServiceIndex is present and is null
@tbenr
Copy link
Author

tbenr commented Aug 1, 2017

Hi all, any comment on this PR? just to know if i'm in the right direction.. I'm working on a project that needs this functionality and i'm happy to contribute but i'd like to do it effectively :)

@thijskh
Copy link
Member

thijskh commented May 29, 2018

It's not quite clear to me what this adds over what's currently possible to specify for AttributeConsumingService in metadata (note that isDefault and Index support have been added recently to master).

Can you make more explicit what this achieves and how it adds to what's already possible?

Would be nice if you can resolve conflics with current master since as said that code has been changed in the meantime.

@tbenr
Copy link
Author

tbenr commented May 31, 2018 •
edited
Loading

Hi Thijs,
this PR covers the usecase where you are implementing a SAML2 IDP. It allows you to parse sp metadata loading all AttributeConsumingServices sets defined in metadata. So, when IDP receives AttributeConsumingServiceIndex sent by SP in the AuthnRequest, it is able to pick the corresponding attribute set (implemented by storing the index in the state and make AttributeLimit.php able to get it and select the requested attribute set from sp metadata).
I may be wrong (I didn't follow the latest changes) but I can't see how to do it using the current implementation.
The AttributeConsumingServiceIndex seems to me that has been considered as SP, not as IDP. Moreover, is current implementation it only considering one attribute set for a given SP?

@falco76
Copy link

falco76 commented Jun 11, 2018

hi @tbenr, hi @thijskh
i need to expose multiple AttributeConsumingService(s) for a single SP?
do this patch solve it?
tnx

@tbenr
Copy link
Author

tbenr commented Jun 18, 2018

hi @falco76,
yes it implements this use-case. (btw, are you involved in the SPID initiative in Italy?)

@falco76
Copy link

falco76 commented Jun 20, 2018

@tbenr
certo enrico

@tvdijen tvdijen force-pushed the master branch 3 times, most recently from 1c686ab to eb20457 Compare August 17, 2020 20:43
@tvdijen tvdijen force-pushed the master branch 2 times, most recently from 08ebb9c to 64fca25 Compare July 2, 2021 14:12
@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 2 times, most recently from c53c946 to 2e6ab04 Compare November 23, 2022 18:23
@tvdijen tvdijen force-pushed the master branch 7 times, most recently from 3b5f5ba to 96357ee Compare July 19, 2023 12:37
@tvdijen tvdijen force-pushed the master branch 5 times, most recently from 7587851 to d523b31 Compare August 2, 2023 11:58
@tvdijen tvdijen force-pushed the master branch 3 times, most recently from 8c90121 to d534e3b Compare September 5, 2023 08:09
@tvdijen tvdijen force-pushed the master branch 2 times, most recently from bc1c5c8 to d0a5974 Compare October 17, 2023 21:17
@tvdijen tvdijen force-pushed the master branch 3 times, most recently from ccb9b02 to 120a100 Compare December 1, 2023 14:34
@tvdijen tvdijen force-pushed the master branch 2 times, most recently from 6004a77 to 58bf8db Compare May 4, 2024 23:45
@tvdijen tvdijen force-pushed the master branch 2 times, most recently from 5c9fb2c to 0970efc Compare May 27, 2024 12:27
@tvdijen tvdijen force-pushed the master branch 2 times, most recently from c27831c to 71e49f4 Compare June 19, 2024 17:03
@tvdijen tvdijen force-pushed the master branch 3 times, most recently from c06a17a to a52c98d Compare August 20, 2025 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

needsinfo

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tbenr @johnmaguire @thijskh @falco76