url-param auth POC #463
url-param auth POC #463
Conversation
| @@ -0,0 +1,63 @@ | |||
| (ns lrsql.auth.interceptor | |||
There was a problem hiding this comment.
interceptor i am assuming will be renamed?
There was a problem hiding this comment.
I concur - it should be in the /admin/interceptors directory
There was a problem hiding this comment.
can I push back on this? It's not actually an admin functionality. Its function is 100% focused on just making /statements work. Has no bearing on any admin route.
src/main/lrsql/auth/interceptor.clj
Outdated
| (let [lrs (:com.yetanalytics/lrs ctx) | ||
| conn (-> lrs :connection :conn-pool) | ||
| backend (:backend lrs) | ||
| input (auth-input/query-credential-by-id-input cred-id) |
There was a problem hiding this comment.
need handling for doesnt exist?
src/main/lrsql/auth/interceptor.clj
Outdated
| [lrsql.util :as util] | ||
| [next.jdbc :as jdbc])) | ||
|
|
||
| (def holder (atom nil)) |
There was a problem hiding this comment.
I am failing to see the necessity of state here. Even looking at it i can't untangle why we would introduce it. We inject an interceptor, and then it modifies ctx, why are there atoms?
src/main/lrsql/system/webserver.clj
Outdated
| @@ -14,6 +15,8 @@ | |||
| [lrsql.util.cert :as cu] | |||
| [lrsql.util.interceptor :refer [handle-json-parse-exn]])) | |||
|
|
|||
| (def holder (atom nil)) | |||
There was a problem hiding this comment.
Agreed with Cliff - I recall that having an atom would make this not work once load balancers are involved (@cliffcaseyyet correct me if I'm wrong)
There was a problem hiding this comment.
double-dog removed
| :clamav-port 3310 | ||
| :auth-by-cred-id false ;may need to be set to true if lrsql behind proxy |
There was a problem hiding this comment.
When adding config vars, make sure you don't forget to also add them to a) the production config and b) the documentation.
if
auth-by-cred-idflag is true, authorizes by credentialID in query params.