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 a parser for IDPSSODescriptor a.k.a. IDP metadata #20

Merged
merged 2 commits into from
Nov 6, 2022

Conversation

fumieval
Copy link
Contributor

@fumieval fumieval commented Oct 3, 2022

I implemented a parser for IdP metadata. There are a few remaining design questions; I'm open to suggestions.

  • Should it parse the contents of x509Certificate?
  • Should there be a datatype for NameIDFormat? If it should, It would affect the definition of NameID too

@mbg mbg self-requested a review October 3, 2022 13:01
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.

Thank you for the continuing contributions to this project!

Should it parse the contents of x509Certificate?

I understand that, in your use case, you store the data in a database and so having it as Text is preferable, but I think that as much validation should be done while parsing the document as possible. Otherwise, you delay performing those checks and might have to implement them in a bunch of places / remember to carry them out in each of them, which is more error-prone.

Should there be a datatype for NameIDFormat? If it should, It would affect the definition of NameID too

There probably should be, yes. I was thinking about this when reviewing #13, but wasn't sure how soon a need for this would arise. I'll leave it up to you whether you want to implement this as part of this PR or not though.


Some other comments:

  • As previously noted in Add a function that generates an SP initiated SSO request #19, it would be good if you could make sure that new source files align with the contribution guidelines with respect to documentation and style.

  • To make it easier for me to review these PRs, I would appreciate it if you could link to the relevant parts of the specification that you are implementing in the PR description or as part of the Haddock documentation going forward.

@fumieval
Copy link
Contributor Author

fumieval commented Oct 5, 2022

I updated the type of x509Certificate to SignedExact Certificate and added some documentation. Unfortunately I wasn't able to find specification for EntityDescriptor in HTML, so I linked to the PDF version instead

@fumieval
Copy link
Contributor Author

fumieval commented Oct 5, 2022

I'll implement a datatype for NameIDFormat in a separate PR

@fumieval
Copy link
Contributor Author

I updated the code style a bit. It would be nice if you could document non-standard code style requirements (such as -- | haddock syntax for record fields) in the guideline

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 for continuing to work on this and working on the formatting a bit. I have made some more suggestions about the formatting/documentation, as well as some questions to help me better understand the implementation with respect to the use of cryptostore, which should probably be documented somewhere.

src/Network/Wai/SAML2/EntityDescriptor.hs Outdated Show resolved Hide resolved
src/Network/Wai/SAML2/EntityDescriptor.hs Outdated Show resolved Hide resolved
package.yaml Outdated Show resolved Hide resolved
src/Network/Wai/SAML2/EntityDescriptor.hs Outdated Show resolved Hide resolved
src/Network/Wai/SAML2/EntityDescriptor.hs Outdated Show resolved Hide resolved
src/Network/Wai/SAML2/EntityDescriptor.hs Outdated Show resolved Hide resolved
src/Network/Wai/SAML2/EntityDescriptor.hs Show resolved Hide resolved
Comment on lines 60 to 66
$ X509.readSignedObjectFromMemory
$ T.encodeUtf8
$ T.unlines
[ "-----BEGIN CERTIFICATE-----"
, rawCertificate
, "-----END CERTIFICATE-----"
]
Copy link
Owner

Choose a reason for hiding this comment

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

There is a readSignedObjectFromMemory function in Data.X509.Memory fromx509-store as well. Why is the one from cryptostore needed? Both seem to be wrappers around decodeSignedObject from x509. I am wondering if it might be better to use one of those functions directly to avoid adding the BEGIN CERTIFICATE / dependency on cryptostore here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this was an oversight; updated to use decodeSignedObject instead

@mbg
Copy link
Owner

mbg commented Oct 24, 2022

It would be nice if you could document non-standard code style requirements

No problem, if this is helpful I can add it explicitly to the contribution guidelines.

@mbg
Copy link
Owner

mbg commented Oct 24, 2022

@fumieval I have just opened #25 with some improvements (hopefully!) to the contribution guidelines. I would be grateful if you could have a look over them to see if that addresses everything you'd find useful or if anything is missing?

@fumieval fumieval force-pushed the idp-metadata branch 2 times, most recently from 1ecdfe2 to f675110 Compare October 28, 2022 01:51
@fumieval
Copy link
Contributor Author

Rebased to use the NameIDFormat datatype

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 for making all those changes! This looks good to me to merge now.

@mbg mbg merged commit 51bee12 into mbg:master Nov 6, 2022
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