microsoft / OpenAPI.NET

The OpenAPI.NET SDK contains a useful object model for OpenAPI documents in .NET along with common serializers to extract raw OpenAPI JSON and YAML documents from the model.
MIT License
1.4k stars 231 forks source link

No difference between no security requirement and empty security requirement on Operations #1426

Open VisualBean opened 1 year ago

VisualBean commented 1 year ago

Describe the bug There is no way of telling 'empty' from 'not defined' security requirement objects, during deserialization. I.e no way of distinguishing the 2 different security requirement objects

paths:
  /pets:
    post:
      responses:
        '201':
          description: Created
      security: []
    get:
      responses:
        '200':
          description: A paged array of pets

Why is this important?

The specification states the following for Operation Security

To remove a top-level security declaration, an empty array can be used.

i.e security: []

however it is expected that if security is not defined on an operation level

get:
  responses:
    '200':
      description: A paged array of pets

That the global security is applied, defined on the top level document object.

in the following spec

openapi: '3.0.0'
info:
  version: 1.0.0
  title: Swagger Petstore
  license:
    name: MIT
security:
  - Authorization:
    - readwrite
servers:
  - url: http://petstore.swagger.io/v1
paths:
  /pets:
    post:
      responses:
        '201':
          description: Created
      security: []
    get:
      responses:
        '200':
          description: A paged array of pets
components:
  securitySchemes:
    Authorization:
      type: http
      scheme: bearer
      bearerFormat: JWT

post deserialization, it is impossible to determine which security is actually applied between the 2 operations, as both security requirements are deserialized into the same value public IList<OpenApiSecurityRequirement> Security { get; set; } = new List<OpenApiSecurityRequirement>(); which ends up being empty in both cases.

The reason for this is most likely that during reading a new OpenApiSecurityRequirement() and as this is a dictionary, there is no way of specifying a null key, so to speak.

The problem being that the semantics between the 2 operations are different in the spec.

To Reproduce Deserialize the above spec.

Expected behavior To have a way of differentiating the 2 scenarios. (feasibly one would be empty, the other would be null.

Screenshots/Code Snippets

var input = @"openapi: '3.0.0'
info:
  version: 1.0.0
  title: Swagger Petstore
  license:
    name: MIT
security:
  - Authorization:
    - readwrite
servers:
  - url: http://petstore.swagger.io/v1
paths:
  /pets:
    post:
      responses:
        '201':
          description: Created
      security: []
    get:
      responses:
        '200':
          description: A paged array of pets
components:
  securitySchemes:
    Authorization:
      type: http
      scheme: bearer
      bearerFormat: JWT";
var document = new OpenApiStringReader().Read(input, out var diagnostics);
darrelmiller commented 1 year ago

Thank you for bringing this to our attention. We have two options here to solve this problem. We can either add a new AnonymousAccess property to the OpenApiOperation object to allow users to explicitly state they want to override the security with no security. Or the other option is to stop defaulting the Security property to an empty list and treat a non-null value as an indication that they want to override the security.


// Option 1 - Breaking change
var operation = new OpenApiOperation();
operation.Security = null;  // Default value

// Set Anonymous access
operation.Security = new List<OpenApiSecurityRequirement>() [];  

// Option 2 - Non breaking change
var operation = new OpenApiOperation();
operation.Security = new List<OpenApiSecurityRequirement>() [];  // Default value
operation.AnonymousAccess = false;  // Anonymous access default

// Set Anonymous access
operation.AnonymousAccess = true;  

We can do Option 2 in the current major version of OpenAPI.NET because it is not breaking, however, if we go with Option 1 we will need to wait until the v2 release.

VisualBean commented 1 year ago

Personally, I think nulling it, is more in line with what is expected from a dotnet point of view.

Edited: to state the desired option without option number.

~~Option 1 does have the added effect of it, then being the ONLY list, that isn't newed up automatically. also it 'might' technically be a breaking change for everyone relying on the existing behaviour. So waiting for v2 might be the safest bet, in any case.~~

crudbee commented 1 year ago

I think option 1 and 2 have been flipped around, from the description to the examples: Option 1 adding AnonymousAccess, Option 2 defaulting to null.

Defaulting to null is the breaking change, so you both seem in agreement to wait for a v2 with it.

Null vs. default is similar to how it would be treated in F# (to my understanding) with option, where Some [] is the explicit override, and None is the lack of the property. The None tends to be mapped from null. I'd feel more comfortable with this option 2, explicit null, rather than a new property whose name I can't see mapping to the OpenAPI spec itself.

corentinvds commented 1 week ago

Hello,

Any update on this issue ? Is there a known workaround other than not using global security specifications ?

Thanks