-
Notifications
You must be signed in to change notification settings - Fork 19
fix(nameIdentifier): handle the case of not found nameIdentifier #20
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,3 +3,5 @@ node_js: | |
| - 4 | ||
| - 6 | ||
| - 8 | ||
| - 10 | ||
| - 12 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| module.exports = { extends: ['@commitlint/config-conventional'] }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,8 @@ | |
| "description": "WSFed server middleware", | ||
| "main": "lib/index.js", | ||
| "scripts": { | ||
| "test": "mocha" | ||
| "test": "mocha", | ||
| "release": "standard-version" | ||
| }, | ||
| "repository": { | ||
| "type": "git", | ||
|
|
@@ -24,15 +25,25 @@ | |
| "saml": "^0.12.1" | ||
| }, | ||
| "devDependencies": { | ||
| "@commitlint/cli": "^9.1.2", | ||
| "@commitlint/config-conventional": "^9.1.2", | ||
| "chai": "~1.5.0", | ||
| "cheerio": "0.22.0", | ||
| "debug": "~3.2.6", | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was this a missing dependency or perhaps when testing locally? I can't seem to find where it's used
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, sorry, forgot to mention this. This dev dependency is used by |
||
| "express": "~3.1.0", | ||
| "husky": "^4.3.0", | ||
| "mocha": "~1.8.1", | ||
| "request": "~2.14.0", | ||
| "xmldom": "=0.1.15", | ||
| "cheerio": "0.22.0", | ||
| "standard-version": "^9.0.0", | ||
| "xml-crypto": "~0.10.1", | ||
| "xml-encryption": "0.11.0", | ||
| "xmldom": "=0.1.15", | ||
| "xpath": "0.0.5", | ||
| "xtend": "~2.0.3", | ||
| "xml-encryption": "0.11.0" | ||
| "xtend": "~2.0.3" | ||
| }, | ||
| "husky": { | ||
| "hooks": { | ||
| "commit-msg": "commitlint -E HUSKY_GIT_PARAMS" | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,10 +26,13 @@ var credentials = { | |
| pkcs7: fs.readFileSync(path.join(__dirname, 'wsfed.test-cert.pb7')) | ||
| }; | ||
|
|
||
| module.exports.options = {}; | ||
|
|
||
| module.exports.start = function(options, callback){ | ||
| module.exports.options = options; | ||
| if (typeof options === 'function') { | ||
| callback = options; | ||
| options = {}; | ||
| module.exports.options = {}; | ||
| } | ||
|
|
||
| var app = express(); | ||
|
|
@@ -64,13 +67,19 @@ module.exports.start = function(options, callback){ | |
| } | ||
|
|
||
| //configure wsfed middleware | ||
| app.get('/wsfed', | ||
| wsfed.auth(xtend({}, { | ||
| issuer: 'fixture-test', | ||
| getPostURL: getPostURL, | ||
| cert: credentials.cert, | ||
| key: credentials.key | ||
| }, options))); | ||
| app.get('/wsfed', function(req, res, next) { | ||
| wsfed.auth(xtend({}, { | ||
| issuer: 'fixture-test', | ||
| getPostURL: getPostURL, | ||
| cert: credentials.cert, | ||
| key: credentials.key | ||
| }, module.exports.options))(req, res, function(err){ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of doing this why not pass in a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With the current implementation, if we pass stubbed profile mapper, we wouldn't be able to test the behavior of picking default profile mapper 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current approach is fine. 👍 |
||
| if (err) { | ||
| return res.send(400, err.message); | ||
| } | ||
| next(); | ||
| }) | ||
| }); | ||
|
|
||
| var server = http.createServer(app).listen(5050, callback); | ||
| module.exports.close = server.close.bind(server); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In
saml11.createboth thenameIdentifierandnameIdentifierFormatare optional.https://github.com/auth0/node-saml/blob/a08d250d02938acde2c53d72858a78d33f4dd684/lib/saml11.js#L41-L42
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
nameIdentifierwithout alert to the client and the client has to deal with it somehow. While emptynameIdentifierbe 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
nameIdentifierand 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, it's a good question. So it might be a breaking change for those consumers, that:
nameIdentifierclaim insideand 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 supportingrequireNameIdentifierconfig option) for now and to go with this breaking change, releasing a major version.This new config option
requireNameIdentifiermight be added in the future if the corresponding feedback will be received.