apigee-127 / swagger-node-runner

The heart of Swagger-Node
MIT License
102 stars 123 forks source link

security handlers #70

Open osher opened 7 years ago

osher commented 7 years ago

EDITED: clarifications added here and there...

I was trying to implement security handlers.

I found the example in the test:

swagger:
    bagpipes:
        _swagger_security:
           name: swagger_security
           securityHandlersModule: api/helpers/securityHandlers

Ok. that's a start. Imature for my flavor - but still a start. 2 problems are:

  1. What if I want to initiate plummings on server start-up, and fail fast unless I have all I need? In the current setup, the server may start accept requests while async initiation is still on progress. If the initiation is incomplete - I don't want the server to even join the load-balancer. If there is a problem - I want it to fail fast. I want to leave the developer the decision to lazy-load his dependency, but also to let him pull the breaks.
  2. What if I want to wrap handler of each method in it's own package, and let my developers cherry-pick them to their micro-services?

So I thought to create a PR that will accept multiple modules and consolidate them together to the securityHandlers collection.

Something Like that:

swagger:
    bagpipes:
        _swagger_security:
            name: swagger_security
            securityHandlers: 
                basic: openapi-seciruty-myorg-basic
                oauth: openapi-seciruty-myorg-oauth
                req-ip: openapi-seciruty-myorg-req-ip

Keys are types matching api.securityDefinitions like the openapi spec demonstrates. About values - I thought to let them be

These are were the early thoughts.

So I thought to have a look inside and see where it takes me. I opened fitting/swagger_security, which calls yet again for double-checking with you...

It seems like it wants to support:

swagger:
    bagpipes:
        ...
    securityHandlers: 
       <but what's here? I have no idea> 

After reading the code it looks to me that if config.swagger.securityHandlers is present - no security handlers are loaded...

  1. What am I missing? really... šŸ˜²
  2. What are your thoughts about the ideas above?
osher commented 7 years ago

P.S. The current situation will force me to handle the cherry-picking and consolidating to a single module in code of the securityHandlersModule, and pull the breaks abruptly with log.fatal(..) && process.exit(1) - which isn't nice šŸ˜¢

We'll soon see how I progress with that

theganyo commented 7 years ago

I'll have to look into it when I get time. Hopefully it's not as dire as you expect.

Regarding security handler configs, have you had a look at this? https://github.com/swagger-api/swagger-node/issues/228

It covers a lot, so maybe that could help answer some of your questions?

osher commented 7 years ago

Well, that helped a bit. After reading that - I found that runner.create(options) can be injected with security hanlders:

That's great.

I also believe I figured the OR and AND logic between elements in operation.security section: The following section will accept basic auth only from the given IP ranges, OR requests with valid api-key in header OR in qs param.

  security:
     - ipRange: [ "10.138.0.*" ] 
       basicAuth: true
     - apiKeyHeader: []
     - apiKeyQS: []

So that's good to put that explicitly too šŸ˜‰ (pls correct me if I'm wrong about it šŸ˜®).

That's also great. .

But that still doesn't help to clarify what the following line means:

if (fittingDef.securityHandlersModule && !runner.config.securityHandlers) {

If it was

if (fittingDef.securityHandlersModule && !runner.securityHandlers) {

Then it would make sense to me like:

.

Also I cannot understand whats runner.config.securityHandlers - i.e where it comes from, and if it's provided what values should be in it.

Reverse engineering the way it's put now - then I understand it as:

The fact that there are no specs/tests for this fitting also makes it harder to guess the original intent... šŸ˜æ

osher commented 7 years ago

Sorry for the nit-picking, I'm MUCH more interested about your thoughts regarding cherry-picking authentication handlers in configuration. Now, even if the implementations come from reusable cherry-picked packages - I still have to compose them together and I have to do that in code... šŸ˜›

osher commented 7 years ago

Mmm. Looks like my security example is not correct. I see that operation.security.{key} may only have Array<string> as value, although the array may be empty.

Plus - the vaildator in swagger-ui will not accept custom types for custom handlers (like my ipRange checker). I now see it permits only basic, apiKey or oauth2, so I cannot implement custom handlers (for application-level ip checks, for example).

This leaves me puzzled regarding why we need such a complex AND/OR logic, but that's another story...

Perhaps I can cheat the system, chose one of the supported types, but implement whatever checks in my securityHandlersModule. meh. šŸ˜›

osher commented 7 years ago

I think I got a setup, but it feels scattered and abusive. Anyway:

in swagger.yaml

paths:
  <path>:
    <operation>:
      security:
        - basic: []
          internalNetwork: []
        - my-oauth2: ["user-read"]

securityDefinitions:
  basic:
    type: basic
  internalNetwork:
    type: basic
  my-oauth2:
     type: oauth2
     flow: implicit
     tokenUrl: "http://foo.bar/token"
     authorizationUrl: "http://foo.bar/auth"
     scopes: 
       user-read: "read permissions"

securityHandlersModule:

const allowedIpRanges = getAllowedIpRanges()
module.exports =  {
  internalNetwork: function(req,res,next) {
      const ip = ipOf(req);
      if ( isIpInRange( ip, allowedIpRanges ) ) return next();
      const e = new Error('Request IP is not in permitted range');
      e.status = 401;
      e.ip = ip;
      next(e);
  },
  basic: function(req,res,next) { ... },
  'my-oauth2': function( req, secDef, scopes, next) { ... }
}
function getAllowedIpRanges() { ... } //pull from some other config :P
function ipOf(req) { ... } //pull from XFF http header or remoteAddress
function isIpInRange(ip, ranges) { ... } //returns true | false