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 security fields from openapi to malli route data. #14

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions deps.edn
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
; https://opensource.org/licenses/MIT.

{:deps {io.swagger.parser.v3/swagger-parser {:mvn/version "2.1.25"}}
:paths ["resources" "src"]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we remove this and put the test dir? in something like a specs dir maybe. we can just use (slurp "specs/security-users.yml"). I'd rather not have things that aren't part of the final artifact in resources.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it makes sense I just added this to have a repl going.

:aliases {:test {:extra-paths ["test"]
:extra-deps {io.github.cognitect-labs/test-runner {:git/tag "v0.5.1" :git/sha "dfb30dd"}
metosin/malli {:mvn/version "0.17.0"}}
Expand Down
65 changes: 65 additions & 0 deletions resources/security-users.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
openapi: 3.1.0
info:
title: Simple User Listing API
version: "1.0.0"

paths:
/users:
get:
operationId: listUsers
security:
- sessionCookieAuth: ["read:user"]
- test: ["one:two"]
responses:
'200':
description: A list of user objects
'401':
description: Unauthorized
/users-single-scheme:
get:
operationId: listUsersSingle
security:
- sessionCookieAuth: ["read:user"]
responses:
'200':
description: OK
/users-no-scope:
get:
operationId: listUsersNoScope
security:
- sessionCookieAuth: []
responses:
'200':
description: OK
/users-no-security:
get:
operationId: listUsersNoSecurity
responses:
'200':
description: OK

components:
securitySchemes:
sessionCookieAuth:
type: apiKey
in: cookie
name: session
description: >
A session-based authentication scheme. The cookie must contain
a valid session ID that identifies the user.
schemas:
User:
type: object
required:
- id
properties:
id:
type: string
format: uuid
description: The unique identifier for a user
email:
type: string
description: Optional email address of the user
name:
type: string
description: Optional name of the user
17 changes: 15 additions & 2 deletions src/navi/impl.clj
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,17 @@
(cond-> {:content content}
description (assoc :description description))))

(defn security->vec-of-vecs
"Takes a list of SecurityRequirement objects and returns
a vector of [scheme scope-vector] pairs.
e.g. [[\"sessionCookieAuth\" [\"read:user\"]]
[\"test\" [\"one:two\"]]]"
[^java.util.List security-reqs]
(->> security-reqs
(mapcat seq)
(map (juxt key val))
(into [])))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this into can be dropped for a mapv before this?

Copy link
Owner

@lispyclouds lispyclouds Jan 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thinking more, is this conversion even needed or does is just work in reitit as its all Maps and Lists anyways?

Copy link
Contributor Author

@JohanCodinha JohanCodinha Jan 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just creating route data. It won't work in reitit until you add middleware that uses that data.
Although it's opinionated, I chose to represent it as a vector of vectors because it convey the logical OR of having multiple security schemes.
This could have been a map but I think of map a as AND.
And yes, this can also be done with a mapv, feel free to change it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Let's add some docs about it on the README.


(defn operation->data
"Converts a Java Operation to a map of parameters, responses, schemas and handler
that conforms to reitit."
Expand All @@ -121,10 +132,12 @@
(wrap-map :header)
(wrap-map :cookie))
responses (-> (.getResponses op)
(update-kvs handle-response-key response->data))]
(update-kvs handle-response-key response->data))
security (security->vec-of-vecs (.getSecurity op))]
(cond-> {:handler (get handlers (.getOperationId op))}
(seq schemas) (assoc :parameters schemas)
(seq responses) (assoc :responses responses)))
(seq responses) (assoc :responses responses)
(seq security) (assoc :security security)))
(catch Exception e
(throw (ex-info (str "Exception processing operation "
(pr-str (.getOperationId op))
Expand Down
48 changes: 46 additions & 2 deletions test/navi/impl_test.clj
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@
(ns navi.impl-test
(:require
[clojure.test :refer [deftest is testing]]
[navi.impl :as i])
[navi.core :as navi]
[navi.impl :as i]
[clojure.java.io :as io])
(:import
[clojure.lang ExceptionInfo]
[io.swagger.v3.oas.models Operation PathItem]
Expand Down Expand Up @@ -153,4 +155,46 @@
(.setGet operation))]
(is (= {:get {:handler "a handler"
:parameters {:path [:map [:x int?]]}}}
(i/path-item->data path-item handlers))))))
(i/path-item->data path-item handlers))))))

(defn find-route [rts path method]
(some (fn [[p r]]
(when (= p path)
(get r method)))
rts))

(deftest security-requirements-test
(testing "Verifying security requirements from security-users.yml"
;; A dummy map of operationId to handler (the actual function doesn't matter for this test).
(let [handlers {"listUsers" (constantly :ok)
"listUsersSingle" (constantly :ok)
"listUsersNoScope" (constantly :ok)
"listUsersNoSecurity" (constantly :ok)}
api-spec (slurp (io/resource "security-users.yml"))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets use a slurp without the resource like mentioned before

routes (navi/routes-from api-spec handlers)]

(testing "multiple security schemes"
(let [route (find-route routes "/users" :get)]
(is (some? route) "Should have found /users GET route")
(is (= [["sessionCookieAuth" ["read:user"]]
["test" ["one:two"]]]
(:security route)))))

(testing "single security scheme with scopes"
(let [route (find-route routes "/users-single-scheme" :get)]
(is (some? route) "Should have found /users-single-scheme GET route")
(is (= [["sessionCookieAuth" ["read:user"]]]
(:security route)))))

(testing "single security scheme without scopes"
(let [route (find-route routes "/users-no-scope" :get)]
(is (some? route) "Should have found /users-no-scope GET route")
(is (= [["sessionCookieAuth" []]]
(:security route))
"No scopes should yield an empty vector for that scheme")))

(testing "no security block"
(let [route (find-route routes "/users-no-security" :get)]
(is (some? route) "Should have found /users-no-security GET route")
(is (nil? (:security route))
"Route with no security block should not have :security key"))))))