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 all types of ACS endpoint specifications #757

Merged
merged 1 commit into from Mar 7, 2021

Conversation

peppelinux
Copy link
Member

@peppelinux peppelinux commented Dec 26, 2020

With these changes a SP ACS service configured with a 3-tuple (URL, binding, index) works correctly.

Fixes #599

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?

@peppelinux peppelinux changed the title Fixes https://github.com/IdentityPython/pysaml2/issues/599 fixed AuthnReq with a 3-tuple (URL+binding+index) ACS service Dec 26, 2020
src/saml2/client_base.py Outdated Show resolved Hide resolved
The SP authnReq now works with a 3-tuple (URL+binding+index) ACS service conf
@peppelinux
Copy link
Member Author

TODO:
a unit test

@c00kiemon5ter
Copy link
Member

I think this is correct.
We do handle indexes in https://github.com/IdentityPython/pysaml2/blob/2641019/src/saml2/metadata.py#L405-L410
In Config.endpoint we do not care about the index, so we should ignore that part of the tuple.

@c00kiemon5ter c00kiemon5ter changed the title fixed AuthnReq with a 3-tuple (URL+binding+index) ACS service Fix AuthnReq with a 3-tuple (URL+binding+index) ACS service configuration Dec 29, 2020
@c00kiemon5ter c00kiemon5ter self-assigned this Dec 29, 2020
@peppelinux
Copy link
Member Author

I think this is correct.
We do handle indexes in https://github.com/IdentityPython/pysaml2/blob/2641019/src/saml2/metadata.py#L405-L410
In Config.endpoint we do not care about the index, so we should ignore that part of the tuple.

I agree, and looking at that code ... Hundreds of nested if statements in a generic try/except... In a for loop. We'll die crazy...

That function could be entirely refactored.
We could use .get method, no more try with all those "haduken" codes!

@peppelinux
Copy link
Member Author

And not at last, we shouldn't return sometimes a string and sometimes a tuple. Let's return a tuple and anything else ...

Let's refactor that function, it's too noisy

@peppelinux
Copy link
Member Author

@c00kiemon5ter you approved this, is it time to merge or do we have to put in also the unit test, before?

@c00kiemon5ter c00kiemon5ter changed the title Fix AuthnReq with a 3-tuple (URL+binding+index) ACS service configuration Handle all types of ACS endpoint specifications Mar 7, 2021
@c00kiemon5ter c00kiemon5ter merged commit 745c592 into IdentityPython:master Mar 7, 2021
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.

[Metadata] Configurable AssertionConsumerService isDefault and index values (IndexedEndpointType)
2 participants