Skip to content
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.

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

Handle multiple back/front-ends #449

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

theseal
Copy link
Contributor

@theseal theseal commented Dec 19, 2023

Without this fix only the last back/front-end will be written to file if split is not involved.

Add new method create_entities_descriptor as a counterpart to create_signed_entity_descriptor to also apply valid option to EntititesDescriptor but avoiding signing.

All Submissions:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you added an explanation of what problem you are trying to solve with this PR?
  • Have you added information on what your changes do and why you chose this as your solution?
  • Have you written new tests for your changes?
  • Does your submission pass tests?
  • This project follows PEP8 style guide. Have you run your code against the 'flake8' linter?

Without this fix only the last back/front-end will be written to
file if split is not involved.

Add new method create_entities_descriptor as a counterpart to
create_signed_entity_descriptor to also apply `valid` option to
EntititesDescriptor but avoiding signing.
@vladimir-mencl-eresearch
Copy link
Contributor

Hi,

Looking at this PR, I understand this would introduce a breaking change: even for deployments with just a entity entity descriptor, the EntityDescriptor would now be wrapped in an EntitiesDescriptor. This might take many deployments by surprise - break the unwritten assumption that the file would contain just a single EntityDescriptor element.

Though, I'm not sure what the best way forward would be:

  • doing this conditional on number of length(entity_descriptors) > 1 would be more likely to cause issues
  • maybe a new option to wrap multiple descriptors into an EntitiesDescriptor?
  • or accept this PR and flag it as a breaking change

Note that I'm not a maintainer, so these are just my thoughts, not a maintainer review.

Hope its helpful.

Cheers,
Vlad

@theseal
Copy link
Contributor Author

theseal commented Dec 20, 2023

You are right and thought about it when I started to write the change but apparently forgot about it.

We have to wonder what the intent was when the code was written from the begging I guess. Stacking multiple entityDescriptor's without embracing in an entitiesDescriptor I think is incorrect.

Not sure how common it is to have multiple front/back-ends (we have at-least) so to mitigate the break I could add your suggested check for the amount of entities and only wrap multiple entities in an entitiesDescriptor.

@vladimir-mencl-eresearch
Copy link
Contributor

I have a use case where I can have an arbitrary number of SAML backends (each service bridged has a standalone SAML identity), but I use the --split-backend to get each EntityDescriptor in a separate metadata file.

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.

2 participants