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

Support Okta's response format #12

Merged
merged 4 commits into from
Aug 24, 2022
Merged

Conversation

fumieval
Copy link
Contributor

@fumieval fumieval commented Aug 1, 2022

The current version didn't work as-is because EncryptedKey is in a different location.

I changed the parser so that it accepts both KeyCloak's and Okta's responses, and added a test suite.

@fumieval
Copy link
Contributor Author

@mbg ping

Copy link
Owner

@mbg mbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @fumieval, thanks for opening this PR and sorry for the delay, I was travelling/on holiday.

I have reviewed your proposed changes and there seems to be an issue with the parsing for EncryptionMethod which also causes the tests you have added to fail (thank you very much for adding those btw!). Can you have a look at that?

It might also be good to change the CI so that the tests are run there, but I am happy to make that change myself later after this PR is merged.

package.yaml Outdated Show resolved Hide resolved
src/Network/Wai/SAML2/XML/Encrypted.hs Outdated Show resolved Hide resolved
package.yaml Outdated Show resolved Hide resolved
@fumieval
Copy link
Contributor Author

I added a GitHub workflow step to run stack test

@mbg
Copy link
Owner

mbg commented Aug 23, 2022

@fumieval Thanks for adding the new step to the CI and making the other changes.

In 9863e72, you remove a line for a case that is indeed currently redundant, but that is more a result of https://github.com/mbg/wai-saml2/blob/6d81ea83bd7a3c5ece0faee2db13d0be09ba8035/src/Network/Wai/SAML2/StatusCode.hs being incomplete, rather than the case actually being redundant. I would prefer if you drop 9863e72 from this branch since it is unrelated to this PR and would have to be restored in the future.

@fumieval
Copy link
Contributor Author

OK, I was just wondering why CI is failing and suspected that the redundant case is the cause; removed that commit for now

@mbg
Copy link
Owner

mbg commented Aug 23, 2022

I commented on the CI failure in the review conversation. The file with the tests is still called parser.hs, even though you have updated package.yaml to refer to Parser.hs.

Copy link
Owner

@mbg mbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that seems to all work fine now! Thank you for contributing this!

@mbg mbg merged commit 3377629 into mbg:master Aug 24, 2022
@mbg
Copy link
Owner

mbg commented Aug 24, 2022

This is now released as https://hackage.haskell.org/package/wai-saml2-0.3.0.0, thank you again!

@fumieval
Copy link
Contributor Author

Great, thanks!

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.

None yet

2 participants