Conversation
…quest object for all three auth schemes
…r basic and apiKey
# Conflicts: # examples/swagger/config/default.json # examples/swagger/swagger/api-v1.yaml
# Conflicts: # examples/swagger/swagger/api-v1.yaml
seanpk
left a comment
seanpk
left a comment
There was a problem hiding this comment.
partial review, but I think a good start to look through
middleware/bos-authentication.js
Outdated
| var pathObj = api.paths[path]; | ||
| var routePath = basePath + _convertPathToExpress(path); | ||
|
|
||
| //loop for http method keys, like get an post |
middleware/bos-authentication.js
Outdated
| var routePath = basePath + _convertPathToExpress(path); | ||
|
|
||
| //loop for http method keys, like get an post | ||
| _.keys(pathObj).forEach(function (method) { |
There was a problem hiding this comment.
isn't it a path that comes out of the pathObj?
There was a problem hiding this comment.
no it isn't, it is a key that corresponds to an http method
| //loop for http method keys, like get an post | ||
| _.keys(pathObj).forEach(function (method) { | ||
| if (_.contains(httpMethods, method)) { | ||
| var operation = pathObj[method]; |
There was a problem hiding this comment.
isn't this the actual HTTP method? so the var should be method (with the previous naming change done)
There was a problem hiding this comment.
in swagger the object is referred to as an 'operation' object
middleware/bos-authentication.js
Outdated
| if (_.contains(httpMethods, method)) { | ||
| var operation = pathObj[method]; | ||
| if (operation['security']) { | ||
| operation['security'].forEach(function (securityReq) { |
There was a problem hiding this comment.
given the other uses in this file of _.forEach (and since I believe it safely passes over the case where the input is null or undefined), replace the if and the Array.forEach with _.forEach.
middleware/bos-authentication.js
Outdated
| operation['security'].forEach(function (securityReq) { | ||
| _.forOwn(securityReq, function (scopes, securityDefn) { | ||
| _applySecurityRequirement(app, method, routePath, securityDefn, | ||
| api.securityDefinitions[securityDefn], |
There was a problem hiding this comment.
what is the length that this is wrapping at? does it need to be wrapped again before the scopes param?
There was a problem hiding this comment.
If I understand you correctly, no because all scopes should be passed into the applySecurityRequirement function. Honestly since the scopes are only used for oauth, it makes no difference at the moment.
middleware/bos-authentication.js
Outdated
| }); | ||
| } | ||
| if (operation['x-bos-security']) { | ||
| operation['x-bos-security'].forEach(function (securityReq) { |
There was a problem hiding this comment.
similar comment as above about using _.forEach
middleware/bos-authentication.js
Outdated
| operation['x-bos-security'].forEach(function (securityReq) { | ||
| _.forOwn(securityReq, function (scopes, securityDefn) { | ||
| _applyCustomSecurityRequirement(app, method, routePath, securityDefn, | ||
| api['x-bos-securityDefinitions'][securityDefn], |
There was a problem hiding this comment.
wrapping really required before scopes?
middleware/bos-authentication.js
Outdated
| _.forOwn(securityReq, function (scopes, securityDefn) { | ||
| _applyCustomSecurityRequirement(app, method, routePath, securityDefn, | ||
| api['x-bos-securityDefinitions'][securityDefn], | ||
| /*operation['x-bos-permissions'][securityReq],*/ |
There was a problem hiding this comment.
why is this comment-removed parameter still here?
| if (securityDefn['x-bos-middleware']) { | ||
| var customAuthMiddleware = loader.getConsumer('middleware', securityDefn['x-bos-middleware']); | ||
| if (!customAuthMiddleware) { | ||
| loader.loadConsumerModules('middleware', |
There was a problem hiding this comment.
what happens if we always call this first? can we remove the if (!customAuthMiddleware)?
There was a problem hiding this comment.
FYI: I haven't really reviewed the code below this point
There was a problem hiding this comment.
it won't do a reload from disk since it is using node's require cache behind the scenes, but still the code is more efficient if you don't attempt to reload the middleware.
middleware/bos-authentication.js
Outdated
| securityDefn.type, securityReq); | ||
| } | ||
| } | ||
| /*//wire up path with user defined authentication method for this req |
There was a problem hiding this comment.
what's the point of keeping all this comment-removed code?
3 participants
This may not be a finished product, but opening a PR to facilitate more conversation around this.
bos-passport was pulled into it's own module (not yet published to npm): https://github.com/BlueOakJS/bos-passport
Some discussion points: