fix(nameIdentifier): handle the case of not found nameIdentifier#20
Conversation
| var profileMap = options.profileMapper(user); | ||
| var claims = profileMap.getClaims(options); | ||
| var ni = profileMap.getNameIdentifier(options); | ||
| if (!ni || !ni.nameIdentifier) { |
There was a problem hiding this comment.
In saml11.create both the nameIdentifier and nameIdentifierFormat are optional.
I realise this is the same approach adopted in auth0/node-samlp#102 but is there a reason why it's preferable to fail here rather than not pass the options?
There was a problem hiding this comment.
Good question @luuuis! I feel like the responsibility of the middleware should be to guarantee that meaningful response is returned. Otherwise, it's going to return the empty nameIdentifier without alert to the client and the client has to deal with it somehow. While empty nameIdentifier be a clear indication of the problem within used profile mapper.
On the other hand, somebody might say that it's the job of the profile mapper to guarantee the existence of nameIdentifier and fail otherwise, but it's not the obvious assumption. And I feel like failing fast is more robust and predictable option. WDYT?
There was a problem hiding this comment.
That makes sense. Even though the <NameIdentifier> is optional, for our purposes we always want to include one in the SAML assertions so it is appropriate to fail fast.
There was a problem hiding this comment.
Agreed, for our purposes this would work just fine. I'm thinking: could this potentially be a breaking change for consumers of this lib? In other words, do you know if the name identifier is strictly required in the assertion and hence not finding one should result in us throwing?
There was a problem hiding this comment.
Hm, it's a good question. So it might be a breaking change for those consumers, that:
- use custom profile mapper that returns name identifier object without
nameIdentifierclaim inside - used profile mapper (default or custom) maps empty name identifier
and expect an assertion to be successfully formed.
Reading SAML v1.1 spec, I don't see that <NameIdentifier> has to be a non-empty string. So it might be a valid case for some flow?
On the other hand, since a similar change has been done in samlp it seems nobody explicitly requested a possibility to support this scenario. What if this change will be released as MINOR or MAJOR change?
There was a problem hiding this comment.
After talking with @fadymak, given that we don't know if any consumer of the library really needs to explicitly pass empty nameIdentifier, it was decided not to introduce an extra complexity (of supporting requireNameIdentifier config option) for now and to go with this breaking change, releasing a major version.
This new config option requireNameIdentifier might be added in the future if the corresponding feedback will be received.
| getPostURL: getPostURL, | ||
| cert: credentials.cert, | ||
| key: credentials.key | ||
| }, module.exports.options))(req, res, function(err){ |
There was a problem hiding this comment.
Instead of doing this why not pass in a profileMapper stub to start and have that behave differently in each test?
There was a problem hiding this comment.
With the current implementation, if we pass stubbed profile mapper, we wouldn't be able to test the behavior of picking default profile mapper PassportProfileMapper (existing tests basically), since the corresponding evaluation of which mapper to chose happens on server start (at the very beginning).
Another viable option is to leave the current test suite as it is, testing the default profile mapper, and have a separate test suite where we start a server with a stubbed profile mapper.
There are different options in the end, but honestly, I find the proposed approach of runtime building wsfed auth for testing the most flexible and easier to use. I saw it basically in node-samlp https://github.com/auth0/node-samlp/blob/master/test/fixture/server.js#L58-L70
| }, | ||
| "devDependencies": { | ||
| "chai": "~1.5.0", | ||
| "debug": "~3.2.6", |
There was a problem hiding this comment.
Was this a missing dependency or perhaps when testing locally? I can't seem to find where it's used
There was a problem hiding this comment.
Yes, sorry, forgot to mention this. This dev dependency is used by mocha. Automatically mocha picks the latest debug lib verison 4.x.x which uses features from ES2015 (like let keywords) and this is not compatible with node v4 (supported by this library). By pinpointing the debug version to 3.x.x. (3.2.6 is the latest) we can still successfully run tests for nodejs v4. Otherwise, tests are failing because of the usage of let by debug package for node v4 basically.
| var profileMap = options.profileMapper(user); | ||
| var claims = profileMap.getClaims(options); | ||
| var ni = profileMap.getNameIdentifier(options); | ||
| if (!ni || !ni.nameIdentifier) { |
There was a problem hiding this comment.
Agreed, for our purposes this would work just fine. I'm thinking: could this potentially be a breaking change for consumers of this lib? In other words, do you know if the name identifier is strictly required in the assertion and hence not finding one should result in us throwing?
BREAKING CHANGE: an error will be returned in case no `nameIdentifier` is returned from the profile mapper
forrest-ua
force-pushed
the
handle-case-of-not-found-name-identifier
branch
from
279aa1f to
3bde2f7
Compare
|
I've forced pushed in to purely update the commit message to reflect the breaking change. And also a small non-functional commit is added 3bde2f7 to enable commitlint and add standard-version. |
2 participants
Description
This PR adds handling of the case when
nameIdentifiercould not be found within the mapped profile for some reason.Testing
Checklist
master