Skip to content

fix issue with satosa and encrypted assertions #419

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

Merged
merged 1 commit into from
Oct 11, 2017

Conversation

leifj
Copy link
Contributor

@leifj leifj commented May 29, 2017

@rohe and @jkakavas pls review!

@leifj
Copy link
Contributor Author

leifj commented May 29, 2017

The issue is that https://github.com/SUNET/SATOSA/blob/master/src/satosa/backends/saml2.py#L161 and https://github.com/SUNET/SATOSA/blob/master/src/satosa/backends/saml2.py#L174 both generate calls to https://github.com/rohe/pysaml2/blob/master/src/saml2/response.py#L900 (parse_assertion). If there is an encrypted assertion the first call will typically result in the decrypted assertion winding up in self.assertion (and self.assertions list). However the second call to parse_assertion has that assert that will fail because after the first call there is no longer an assertion or an encrypted assertion left in the response (since it was dealt with in the first call to parse_assertion). The patch alters the assert to allow the parse_assertion to be called multiple times. Possibly this is an indication that parse_assertion is really doing > 1 "thing" and should be split up. This is however a breaking API change ...

@rohe
Copy link
Contributor

rohe commented May 29, 2017

I'm OK with this change and I agree we could do with some refactoring here.
But not right now!

@leifj
Copy link
Contributor Author

leifj commented May 31, 2017

@rohe are you ok with the PR as is though?

@rohe
Copy link
Contributor

rohe commented Sep 28, 2017

Yes

@rohe rohe merged commit 9e6ddc0 into IdentityPython:master Oct 11, 2017
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