Bos auth by ryshep111 · Pull Request #81 · BlueOakJS/blueoak-server

ryshep111 · 2016-11-02T19:54:47Z

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:

do we keep the oauth implementation?
do we do anything to help with authorization?
should this be changed at all based on changes being made in open api v3: https://github.com/OAI/OpenAPI-Specification/blob/OpenAPI.next/versions/3.0.md#security-scheme-object

…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

partial review, but I think a good start to look through

seanpk · 2016-11-15T12:05:50Z

+            var pathObj = api.paths[path];
+            var routePath = basePath + _convertPathToExpress(path);
+
+            //loop for http method keys, like get an post


seanpk · 2016-11-15T12:07:29Z

+            var routePath = basePath + _convertPathToExpress(path);
+
+            //loop for http method keys, like get an post
+            _.keys(pathObj).forEach(function (method) {


isn't it a path that comes out of the pathObj?

no it isn't, it is a key that corresponds to an http method

seanpk · 2016-11-15T12:08:28Z

+            //loop for http method keys, like get an post
+            _.keys(pathObj).forEach(function (method) {
+                if (_.contains(httpMethods, method)) {
+                    var operation = pathObj[method];


isn't this the actual HTTP method? so the var should be method (with the previous naming change done)

in swagger the object is referred to as an 'operation' object

seanpk · 2016-11-15T12:10:24Z

+                if (_.contains(httpMethods, method)) {
+                    var operation = pathObj[method];
+                    if (operation['security']) {
+                        operation['security'].forEach(function (securityReq) {


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.

seanpk · 2016-11-15T12:11:34Z

+                        operation['security'].forEach(function (securityReq) {
+                            _.forOwn(securityReq, function (scopes, securityDefn) {
+                                _applySecurityRequirement(app, method, routePath, securityDefn,
+                                    api.securityDefinitions[securityDefn],


what is the length that this is wrapping at? does it need to be wrapped again before the scopes param?

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.

seanpk · 2016-11-15T12:12:25Z

+                        });
+                    }
+                    if (operation['x-bos-security']) {
+                        operation['x-bos-security'].forEach(function (securityReq) {


similar comment as above about using _.forEach

seanpk · 2016-11-15T12:14:51Z

+                        operation['x-bos-security'].forEach(function (securityReq) {
+                            _.forOwn(securityReq, function (scopes, securityDefn) {
+                                _applyCustomSecurityRequirement(app, method, routePath, securityDefn,
+                                    api['x-bos-securityDefinitions'][securityDefn],


wrapping really required before scopes?

see above comment

seanpk · 2016-11-15T12:15:09Z

+                            _.forOwn(securityReq, function (scopes, securityDefn) {
+                                _applyCustomSecurityRequirement(app, method, routePath, securityDefn,
+                                    api['x-bos-securityDefinitions'][securityDefn],
+                                    /*operation['x-bos-permissions'][securityReq],*/


why is this comment-removed parameter still here?

seanpk · 2016-11-15T14:13:07Z

+    if (securityDefn['x-bos-middleware']) {
+        var customAuthMiddleware = loader.getConsumer('middleware', securityDefn['x-bos-middleware']);
+        if (!customAuthMiddleware) {
+            loader.loadConsumerModules('middleware',


what happens if we always call this first? can we remove the if (!customAuthMiddleware)?

FYI: I haven't really reviewed the code below this point

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.

seanpk · 2016-11-15T15:15:00Z

+                securityDefn.type, securityReq);
+        }
+    }
+    /*//wire up path with user defined authentication method for this req


what's the point of keeping all this comment-removed code?

shepp added 19 commits October 4, 2016 16:11

bos auth initial commit

8785593

add extractor functions for basic, apiKey

da82474

hook in passport

b0b2bdb

fun stuff with oauth2

1aeb50a

build apiKey extractor around RFC 7616. set consistent property on re…

f9f393d

…quest object for all three auth schemes

partial: testing auth

4f7bf92

allow custom security definitions and middleware

54fe804

temp commit

a57556f

wire up passport

59f4133

example working for passport-hmac-auth

06214a5

oauth POC

513f15c

get oauth implicit flow example to work with swagger ui

67bdb70

cleanup

df71547

add securityDefn to authData

43488c5

added support for multiple security requirements on a route, tests fo…

e820495

…r basic and apiKey

set authenticate header in implicit flow

32516a1

Merge branch 'master' into bos-auth

3e09008

# Conflicts: # examples/swagger/config/default.json # examples/swagger/swagger/api-v1.yaml

pulled out bos-passport into it's own module

07783d1

Merge branch 'master' into bos-auth

a7adfaf

# Conflicts: # examples/swagger/swagger/api-v1.yaml

seanpk assigned seanpk and dbleakley Nov 7, 2016

shepp added 2 commits November 7, 2016 10:07

ref compiler enabled in example to avoid conflicts with master

042a47b

remove oauth for now

7da021f

seanpk reviewed Nov 15, 2016

View reviewed changes

mostly code cleanup

175717b

Conversation

ryshep111 commented Nov 2, 2016

Uh oh!

seanpk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants