...
Number | Line / Reference | Proposed Change or Query | Proposer |
---|---|---|---|
1 | N/A | The PDF appears to lack line numbers, so that may complicate the feedback here. | Fixed Scott Cantor - my bad, sorry (NH). |
2 | L99, End of pg 4 | Nit, suggest you s/SAML 1.0/SAML 1.0 and 1.1 for completeness. | |
3 | L113, Page 5 | Nit, s/intent/intend | |
4 | Footnote 15 | Nit, s/subjected/subject | |
5 | Line 169 | I think the "spirit" of pairwise IDs in SAML would make it improper to just forward them into the Internet as an OIDC claim. They were always intended by the original Liberty work as "secret" in some sense and not to be shared gratuitiously, so I think turning a pairwise ID into a non-pairwise ID through a proxy is not really appropriate unless the proxy is "inward" facing. I don't think the document is presuming that though. If the intent here is rather that the proxy be stateful and do a mapping from the inbound SAML value to an outbound pub claim, that isn't coming across as clearly as perhaps intended. It may be a similar question in the opposite direction but I don't claim to speak for the "intent" behind the pairwise nature of the sub claim in OIDC, whereas I can speak for what the intent was in SAML. | |
6 | Line 211 | Nit, s/taking/taken | |
7 | Line 337 | Those are not in any sense "SAML Attribute names" as used by our community so I would suggest the mapping be limited to eduPerson/etc. and OIDC and leave the SAML part out of it. Essentially if you can deduce that a SAML Attribute corresponds to a given LDAP/X.500 Attribute Type, then your mapping can transit that hop and leave the SAML part implied. I disagree with string-based attribute names in a pretty deep way but if you're going to do that, I would just leave the non-string naming in SAML off to the side. | |
9 | footnote 15, P7 | Nit, s/the the/the | |
10 | L34, P1 | Define Research and Education - s/R&E/Research and Education (R&E)/ | |
11 | L35, P2 | s/R&S/Research and Scholarship (R&S) (then remove eventual definition bracket mention from L435, P21) | |
12 | L103, P5 | s/. Reasoning is that/ because/ | |
13 | Section 8 | Please also include voPerson attributes, which I believe can be mapped just like eduPerson attributes. | James Alan Basney |