Extend and document the profile object #301
Conversation
cjbarth
force-pushed
the
profile-data
branch
from
7253e98
to
5365e62
Compare
|
@markstos I've squashed these commits. |
|
This may also address #256 |
| ID?: string; | ||
| } | ||
| ``` | ||
|
|
||
| #### Config parameter details: |
There was a problem hiding this comment.
@cjbarth Thanks for this. Generally it looks good.
To check if this was correct, I reviewed the result of grep profile lib/passport-saml/saml.js. The results show that all these fields are present. Some recommended changes:
- briefly document both getAssertionXml() and getAssertion()
- From reading this code, it appears arbitrary name/value pairs of attributes may also be returned: https://github.com/bergie/passport-saml/blob/master/lib/passport-saml/saml.js#L851 this should also be documented.
- The exceptional handling of "mail" and "email" mentioned above is perhaps also worth mentioning. The code shows that "mail" defaults to a particular field identified by OID, and that "email" will default to "mail" if not present. It's not clear to me why both exist, but with slightly different meanings.
- squash commits into one
Thanks!
There was a problem hiding this comment.
@markstos I believe I've addressed your concerns here. Please let me know if there is more to do.
cjbarth
force-pushed
the
profile-data
branch
from
23c618a
to
faccd89
Compare
|
@cjbarth Everything looks great now. Merging. |
|
When might the npm package get updated to include this (specifically, getAssertion)? I installed latest but get 0.35.0 which according to readme is current latest and greatest, and only the getAssertionXml is there. |
|
@ray2k This week, I think. We are packaging some breaking changes in a bundle, so trying go about it carefully. Install from Github if you want, expecting it will be forward compatible. NPM's package.json has a syntax for that. |
This relates to the discussion in #278 .