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

Fixes for Keycloak #11

Merged
merged 2 commits into from
Jul 28, 2022
Merged

Fixes for Keycloak #11

merged 2 commits into from
Jul 28, 2022

Conversation

Philonous
Copy link
Contributor

I'm tyring to use your library with keycloak, and made a few changes to accommodate that use case:

  • I removed encryptedKeyData from EncryptedKey. It's not used anywhere and prevented the assertion from being parsed.
  • The parsing code had a subtle bug:

Take for example (from here):

                oneOrFail "CanonicalizationMethod is required"
              $ cursor 
             $/ element (dsName "CanonicalizationMethod") 
            >=> parseXML

This parses as

                oneOrFail "CanonicalizationMethod is required"
              ( cursor 
                 $/ element (dsName "CanonicalizationMethod") 
                 >=> parseXML)

Note how the oneOrFail applies the whole expression including the >=> parseXML.
The problem arises when parseXML throws an error. The correct behaviour is that the error gets thrown according to the MonadFail instance of the outer expression. However, because oneOrFail expects a list, parseXML uses the MonadFail instance of list, which just discards the error and returns an empty list. oneOrFail then throws its own error.
The result is that the error that parseXML threw is replaced, making debugging a lot harder.

I replaced it with the following code, which propagates errors correctly:

        canonicalisationMethod <- 
                oneOrFail "CanonicalizationMethod is required"
              ( cursor
             $/ element (dsName "CanonicalizationMethod") 
              ) >>= parseXML

@mbg
Copy link
Owner

mbg commented Jul 21, 2022

Hey @Philonous, thanks for opening this PR!

I removed encryptedKeyData from EncryptedKey. It's not used anywhere and prevented the assertion from being parsed.

I have had a look over the specification for EncryptedKey which largely extends the specification for EncryptedType. It seems that the KeyInfo key is optional (minOccurs='0'), rather than required. Even though it is not currently used, I would probably prefer correctly implementing it as optional, rather than removing outright! Could you make that change?

The parsing code had a subtle bug: [..]

Well spotted! Thanks for fixing this.

@Philonous
Copy link
Contributor Author

I updated the PR so the encryptedKeyData field is parsed optionally.

Sorry about the white space changes, I've configured my editor to automatically remove trailing white spaces. I've tried to leave them out of the commit, but some seem to have slipped through and I couldn't be bothered to remove them since they are an improvement anyway 😅

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.

This looks good to me; thank you for making the changes! No worries about the changes to the line endings, those spaces shouldn't have been there to begin with!

@mbg mbg merged commit 8cee939 into mbg:master Jul 28, 2022
kdxu pushed a commit to kdxu/wai-saml2 that referenced this pull request Aug 1, 2022
…nal (mbg#11)

* Don't eat errors when `oneOrFail` and `parseXML` are used together.

* Make `encryptedKeyData` field optional.
@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!

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