openapi-backend icon indicating copy to clipboard operation
openapi-backend copied to clipboard

SecurityHandlers aren't fed the required scopes, and don't properly handlemultiple requirements with the same scheme

Open henhal opened this issue 4 years ago • 0 comments

An operation may have multiple security requirements, where each requirement may require multiple security schemes. This means that the same scheme could in theory be present in multiple requirements, but openapi-backend treats each scheme as unique. The effect is that if an operation looks like this:

security:
  - Foo: [scope1, scope2]
    Bar: []
  - Foo: [scope3]
    Baz: []

... it means that this operation is authorized if any of the following is true:

  • The Foo scheme is authorized and is granted at least scope1 or scope2, and the Bar scope is authorized

or

  • The Foo scheme is authorized and is granted at least scope3, and the Baz scope is authorized

However, openapi-backend will find that there are four schemes to authorize: Foo, Bar, Foo (again!) and Baz. And since they are performed in parallel using Promise.all, the results for Foo will be calculated twice and is subject to concurrency issues. Furthermore, the implementation of the Foo security handler has no way of knowing which set of scopes is required in each of the two calls.

I see two possible improvements here: Either the security handlers are be invoked as today (Foo invoked twice) but with the required scopes array as a parameter; or Foo is only invoked once and security handlers are required to return the granted scopes as their result, and then some additional logic is implemented to handle scope checking outside of the security handlers. But that limits the freedom of what security handlers may do - perhaps they need to return something else alongside just the granted scopes. Currently, security handlers are very general; they return any and if the return value is truthy it's considered to "pass". With such a change, a security handler would be required to return an array of granted scopes, which limits flexibility.

Actually, even without multiple colliding schemes, a security handler needs to manually iterate context.operation.security to get the relevant scopes to check for, so also in a more normal scenario it would be beneficial if the required scopes would be passed to the security handler. This would be a bit difficult to implement in a backwards compatible way though, since all handlers are basically (context, ...args). But the other approach has even more side-effects. Thoughts?

henhal avatar Mar 09 '21 15:03 henhal