Bos auth #81
Bos auth #81
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
| var pathObj = api.paths[path]; | ||
| var routePath = basePath + _convertPathToExpress(path); | ||
|
|
||
| //loop for http method keys, like get an post |
| 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
| 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.
| 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.
| }); | ||
| } | ||
| 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
| 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?
| _.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.
| 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?
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: