Skip to content

IDP SAML2: AttributeConsumingServiceIndex implementation#649

Open
tbenr wants to merge 6 commits intosimplesamlphp:masterfrom
tbenr:for_PR_attrcsi
Open

IDP SAML2: AttributeConsumingServiceIndex implementation#649
tbenr wants to merge 6 commits intosimplesamlphp:masterfrom
tbenr:for_PR_attrcsi

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 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 2 times, most recently from d7f25d2 to 41d9254 Compare July 30, 2023 08:37
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