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

Add artifact binding for ACS #237

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

flupzor
Copy link
Contributor

@flupzor flupzor commented Jan 28, 2021

We've used the wonderful python3-saml code to create an implementation for
two dutch government services (DigiD en eHerkenning). This meant that we had to make various changes
to python3-saml code. The biggest being the Artifact Binding for the ACS.

This PR adds support for the Artifact Binding for the ACS for python3-saml. I've tried as much as I could to keep
the coding style which was used in this project.

Alexander Schrijver added 25 commits January 14, 2021 16:37
I used 1 artifactResolutionService (the first) and it was parsed from
the metadata file. However, in the metadata file I am using, the same
URL is used, with two different indexes. Since the wrong index was
returned, this caused errors.

Another implementation would be to just ignore the index/url returned
in the SAML artifact.
@pitbulk
Copy link
Contributor

pitbulk commented Jan 28, 2021

I will need time to review this.

I have not many requirements related to Artifact binding, but I will consider adding this.

In a fast review, I see you made some improvements in parts of the code, so maybe a start is to add them to make your code compatible... later let's see if add this piece of code or see if we can make this available via a plugin or something.

My first goal is to keep the toolkit simple so I always have a battle between functionality and UX.

@flupzor
Copy link
Contributor Author

flupzor commented Jan 29, 2021

@pitbulk That direction make sense to me. I'm hoping to have some time next week to split out these changes.

@sergei-maertens
Copy link

@pitbulk I've been following up some of our forks to check if they are still needed and if the changes can be/are contributed back to upstream. While doing some, I stumbled on python3-saml and three pull requests by flupzor from our fork. Unfortunately flupzor no longer works with us, so I can't ask directly about the state of those PRs, but I was wondering if you could answer these high level questions:

  1. Is there a chance these PRs will get merged or not? I sense some hesitation because it would potentially complicate the library too much.
  2. If there's interest in these features, do you need anything else to be done to progress (apart from resolving the merge conflicts)?
  3. If not, is there a roadmap or idea to make the python API more pluggable so we could instead implement this as a plugin rather than having to fork?

Thanks in advance for any replies!

@VdeJong
Copy link

VdeJong commented Mar 16, 2022

We'd love to see the Artifact Binding being added to this repo! For the same reason @flupzor made this merge request. Hopefully you'll be able to successfully merge this. @sergei-maertens

@sergei-maertens
Copy link

@maintainers - now that this library is transferred, can you give us an indication on how viable the PRs authored by @flupzor are to be merged? Or let us know what is needed to merge these - we'd really like to replace our fork again with the upstream version :)

@sergei-maertens
Copy link

@pitbulk sorry to ping you - is the wontfix label a final conclusion? My above comment was never addressed it seems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants