Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement and/or logic in security validator #393

Merged
merged 7 commits into from Oct 24, 2020

Conversation

balazssoltesz
Copy link
Contributor

No description provided.

@cdimascio
Copy link
Owner

@balazssoltesz Thanks a lot for the PR!

There appears to be one test that needs attention
you can run them locally via npm t

1) security.handlers
       should return 200 if api_key or anonymous and no api key is supplied:
     Error: expected 200 "OK", got 401 "Unauthorized"
      at Test._assertStatus (node_modules/supertest/lib/test.js:268:12)
      at Test._assertFunction (node_modules/supertest/lib/test.js:283:11)
      at Test.assert (node_modules/supertest/lib/test.js:173:18)
      at Server.localAssert (node_modules/supertest/lib/test.js:131:12)
      at emitCloseNT (net.js:1658:8)
      at processTicksAndRejections (internal/process/task_queues.js:79:21)

in addition to investigating this error, it will be good to add additional tests to exercise the new and/or capabilities

thanks so much!

@balazssoltesz
Copy link
Contributor Author

balazssoltesz commented Oct 11, 2020

@cdimascio i not rly understand this. why should return 200 if not api_key supplied? That is the point, we should return 401 when any of the security check is failed (when AND logic is used).

@cdimascio
Copy link
Owner

cdimascio commented Oct 11, 2020

@balazssoltesz openapi allows one to declare anonymous access explicitly. this particular test checks that if ApiKey and anonymous access are declared for an api endpoint using OR semantics, then it should return 200 when either the request contains the apikey OR the request is anonymous

Here is the spec for the test that sets this up. Note that {} indicates anonymous access is allowed:

https://github.com/cdimascio/express-openapi-validator/blob/master/test/resources/security.yaml#L27-L37

  /api_key_or_anonymous:
    get:
      security:
        # {} means anonyous or no security - see https://github.com/OAI/OpenAPI-Specification/issues/14
        - {}
        - ApiKeyAuth: []
      responses:
        "200":
          description: OK
        "401":
          description: unauthorized

all in all, its possible the new code is not considering the {} (explicit anonymous) case

@cdimascio
Copy link
Owner

cdimascio commented Oct 17, 2020

note that in my example above (which the test exercises), it states A OR B where A is {} (anonymous) and B is ApiKeyAuth. In this case, if no authentication is provided, it should return success as it satisfies the security conditions

@balazssoltesz
Copy link
Contributor Author

note that in my example above (which the test exercises), it states A OR B where A is {} (anonymous) and B is ApiKeyAuth. In this case, if no authentication is provided, it should return success as it satisfies the security conditions

ok, i modified my fork. now all test is passed

@cdimascio
Copy link
Owner

@balazssoltesz Would u mind adding a test or two.
This is fantastic! Thanks so much for your work on this.

@cdimascio cdimascio merged commit 683c54b into cdimascio:master Oct 24, 2020
@cdimascio
Copy link
Owner

@all-contributors add @balazssoltesz for code and test

@allcontributors
Copy link
Contributor

@cdimascio

I've put up a pull request to add @balazssoltesz! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants