feat: add auth plugin for casdoor#6382
Conversation
6bfe556 to
753238b
Compare
753238b to
2f138c8
Compare
ComradeProgrammer
changed the title
ComradeProgrammer
changed the title
465685d to
6656e06
Compare
| local schema = { | ||
| type = "object", | ||
| properties = { | ||
| casdoor_endpoint = { type = "string" }, |
There was a problem hiding this comment.
Let's remove the casdoor_ prefix
There was a problem hiding this comment.
@ComradeProgrammer
Please request me to review it once you are ready. Thanks!
There was a problem hiding this comment.
Let's remove the casdoor_ prefix
all other casdoor_ prefixes are removed except casdoor_endpoint, because i think just using "endpoint" may be a little confusing.
| local schema = { | ||
| type = "object", | ||
| properties = { | ||
| casdoor_endpoint = { type = "string" }, |
There was a problem hiding this comment.
Let's remove the casdoor_ prefix
| local core = require("apisix.core") | ||
| local log = core.log | ||
| local ngx = ngx | ||
| local session = require("resty.session") |
There was a problem hiding this comment.
Let's sort the imports like
apisix/apisix/plugins/authz-casbin.lua
Line 20 in 4624135
| - request-validation # priority: 2800 | ||
| - openid-connect # priority: 2599 | ||
| - authz-casbin # priority: 2560 | ||
| - auth-casdoor # priority: 2561 |
There was a problem hiding this comment.
Please sort according to the priority and fix the indent
|
|
||
| local _M = { | ||
| version = 0.1, | ||
| priority = 12, |
|
|
||
| if session_obj_read.data.access_token then | ||
| -- session exists | ||
| log.warn("session exists") |
There was a problem hiding this comment.
Ditto. Warn log should not be used in per-req level
| local args = ngx.req.get_uri_args() | ||
| if not args.code or not args.state then | ||
| log.err("failed when accessing token. Invalid code or state ") | ||
| return nil |
There was a problem hiding this comment.
| return nil | |
| return nil, err |
and log the err outside would be better
| end | ||
| local client = http.new() | ||
| local res, err = client:request_uri( | ||
| get_path(conf.casdoor_endpoint) .. "/api/login/oauth/access_token", |
There was a problem hiding this comment.
Let's check if the endpoint contains ? in the schema instead of split it at runtime
| log.err("failed when accessing token. ", err) | ||
| return nil | ||
| end | ||
| local data = cjson.decode(res.body) |
There was a problem hiding this comment.
Need to check if decode succeed
|
|
||
| ``` | ||
|
|
||
| By doing this all requests toward `/anything/*` will be intercepted and whether this user has logged in via Casdoor will be checked (by checking session). If not, the user will be redirected to login page of casdoor, thus implementing authentication without modifying server's code. |
There was a problem hiding this comment.
Could you provide a step-by-step example?
I still can't fully understand it after reading it twice...
It looks like the callback_url needs to contain part of the request host & path?
|
I am extremely flattered, delighted and grateful for this extremely detailed reply and advice from you. Revisions will be made accordingly soon and tests will be added soon together. @spacewander |
3173296 to
653cfc6
Compare
| client_id = { type = "string" }, | ||
| client_secret = { type = "string" }, | ||
| callback_url = { type = "string" }, | ||
| test = { type= "boolean", default = false } |
There was a problem hiding this comment.
test is not in Attributes of plugin docs
| end | ||
|
|
||
| function _M.access(conf, ctx) | ||
| log.info("hit auth-casdoor access") |
There was a problem hiding this comment.
This line looks like it's only for debugging?
| local function fetch_access_token(conf) | ||
| local args = core.request.get_uri_args() | ||
| if not args.code or not args.state then | ||
| log.err() |
There was a problem hiding this comment.
Do you mean this function? Extract "code" and "state" parameters and use them to launch to casdoor api, in order to fetch "access_token" so that we can confirm whether the user have really logged in
There was a problem hiding this comment.
The err doesn't have parameters in it
| end | ||
|
|
||
| local function fetch_access_token(conf) | ||
| local args = core.request.get_uri_args() |
There was a problem hiding this comment.
| local args = core.request.get_uri_args() | |
| local args = core.request.get_uri_args(ctx) |
and should pass ctx to this function
|
|
||
| local function fetch_access_token(conf) | ||
| local args = core.request.get_uri_args() | ||
| if not args.code or not args.state then |
There was a problem hiding this comment.
it's better to determine if args is empty
| end | ||
|
|
||
| function _M.check_schema(conf) | ||
| local ok,err=core.schema.check(schema, conf) |
There was a problem hiding this comment.
| local ok,err=core.schema.check(schema, conf) | |
| local ok, err = core.schema.check(schema, conf) |
| local ok,err=core.schema.check(schema, conf) | ||
| if ok then | ||
| --check whether contains extra ? or / | ||
| if string.find(conf.callback_url,"?",1) then |
There was a problem hiding this comment.
use ngx.re.find is better?
| if ok then | ||
| --check whether contains extra ? or / | ||
| if string.find(conf.callback_url,"?",1) then | ||
| return false,"callback_url should not contain get parameters or end up with /" | ||
| else | ||
| return true,nil | ||
| end | ||
| else | ||
| return false,err | ||
| end |
There was a problem hiding this comment.
| if ok then | |
| --check whether contains extra ? or / | |
| if string.find(conf.callback_url,"?",1) then | |
| return false,"callback_url should not contain get parameters or end up with /" | |
| else | |
| return true,nil | |
| end | |
| else | |
| return false,err | |
| end | |
| if not ok then | |
| return false, err | |
| end | |
| --check whether contains extra ? or / | |
| local has_extra = string.find(conf.callback_url,"?",1) | |
| if has_extra then | |
| return false, "callback_url should not contain get parameters or end up with /" | |
| end | |
| return true, nil |
is better?
| if access_token then | ||
| local original_url = session_obj_read.data.original_uri | ||
| if not original_url then | ||
| return 503,"no original_url found in session" |
There was a problem hiding this comment.
| return 503,"no original_url found in session" | |
| return 503, "no original_url found in session" |
| session_obj_write.data.access_token = access_token | ||
| session_obj_write:save() | ||
| ngx.redirect(original_url, 302) | ||
| return |
There was a problem hiding this comment.
It seems a bit strange that here return does not reverse anything, but just exits the current function?
653cfc6 to
b5953d7
Compare
|
@spacewander @tzssangglass Most of the revisions in your comments have been implemented, and tests are also added. I think this code is ready to be reviewed now, THX. |
hi @ComradeProgrammer, after the CI run is finished, please fix the failure of the CI. |
52eea7e to
c699468
Compare
I have made changes according to the CI information, and I wonder whether I can have CI run another time , so that I can confirm whether I can pass CI now? |
hi @ComradeProgrammer , there are still some code lint check failed. |
c699468 to
cc75f06
Compare
| local schema = { | ||
| type = "object", | ||
| properties = { | ||
| casdoor_endpoint = {type = "string"}, |
There was a problem hiding this comment.
| casdoor_endpoint = {type = "string"}, | |
| endpoint_addr = {type = "string"}, |
would be better?
| } | ||
| } | ||
|
|
||
| local _M = {version = 0.1, priority = 2559, name = plugin_name, schema = schema} |
There was a problem hiding this comment.
please put each field per line, like other plugins
| local function fetch_access_token(conf) | ||
| local args = core.request.get_uri_args() | ||
| if not args.code or not args.state then | ||
| log.err() |
There was a problem hiding this comment.
The err doesn't have parameters in it
| return nil, "failed when accessing token. Invalid code or state" | ||
| end | ||
| local client = http.new() | ||
| local url = get_path(conf.casdoor_endpoint) .. |
There was a problem hiding this comment.
Could we require the endpoint not to have ? so we don't need to use get_path?
| local ok, err = core.schema.check(schema, conf) | ||
| if not ok then return false, err end | ||
| -- check whether contains extra ? or / | ||
| local has_extra = string.find(conf.callback_url, "?", 1) |
There was a problem hiding this comment.
We can do the check in the schema via "pattern":
Line 34 in acf7a0c
|
|
||
| The first one is "callback_url". This is exactly the callback url in OAuth2. It should be emphasized that this callback url **must belong to the "uri" you specified for the route**, for example, in this example, http://localhost:9080/anything/callback obviously belong to "/anything/*" Only by this way can the visit toward callback_url can be intercepted and utilized by the plugin(so that the plugin can get the code and state in Oauth2). The logic of callback_url is implemented competely by plugin so that there is no need to modify the server implement this callback. | ||
|
|
||
| The second parameter "callback_url" is obviously the url of Casdoor. The third and fourth parameters are "client_id" and "client_secret", which you can acquire from Casdoor when you register an app. |
There was a problem hiding this comment.
| The second parameter "callback_url" is obviously the url of Casdoor. The third and fourth parameters are "client_id" and "client_secret", which you can acquire from Casdoor when you register an app. | |
| The second parameter "endpoint_addr" is obviously the url of Casdoor. The third and fourth parameters are "client_id" and "client_secret", which you can acquire from Casdoor when you register an app. |
|
|
||
| #### How it works? | ||
|
|
||
| Suppose a new user who have never visited this route before is going to visit it (http://localhost:9080/anything/d?param1=foo¶m2=bar), considering that "auth-casdoor" is enabled, this visit would be processed by "auth-casdoor" plugin first. After checking the session and confirming that this use hasn't been authentication, the visit will be intercepted. After remembering the original url user wants to visit, he will be redirected to the login page of Casdoor. |
There was a problem hiding this comment.
| Suppose a new user who have never visited this route before is going to visit it (http://localhost:9080/anything/d?param1=foo¶m2=bar), considering that "auth-casdoor" is enabled, this visit would be processed by "auth-casdoor" plugin first. After checking the session and confirming that this use hasn't been authentication, the visit will be intercepted. After remembering the original url user wants to visit, he will be redirected to the login page of Casdoor. | |
| Suppose a new user who has never visited this route before is going to visit it (http://localhost:9080/anything/d?param1=foo¶m2=bar), considering that "auth-casdoor" is enabled, this visit would be processed by "auth-casdoor" plugin first. After checking the session and confirming that this user hasn't been authenticated, the visit will be intercepted. With the original url user wants to visit kept, he will be redirected to the login page of Casdoor. |
|
|
||
| After successfully logging in with username and password(or whatever method he use), Casdoor will redirect this user to the "callback_url" with GET parameter "code " and "state" specified. Because the "callback_url" is known by the plugin, so when the visit toward the "callback_url" is intercepted this time, the logic of "Authorization code Grant Flow" in Oauth2 will be triggered, which means this plugin will request the access token to confirm whether this user is really logged in. After this confirmation, this plugin will redirect this user to the original url user wants to visit, which was remembered by us previously. The logged in status will also be remembered by session | ||
|
|
||
| Next time this user want to visit url behind this route (for example, http://localhost:9080/anything/d), after discovering that this user have been authentication previously, this plugin won't redirect this user any more so that this user can visit whatever he wants under this route without being interfered |
There was a problem hiding this comment.
| Next time this user want to visit url behind this route (for example, http://localhost:9080/anything/d), after discovering that this user have been authentication previously, this plugin won't redirect this user any more so that this user can visit whatever he wants under this route without being interfered | |
| Next time this user want to visit url behind this route (for example, http://localhost:9080/anything/d), after discovering that this user has been authenticated previously, this plugin won't redirect this user anymore so that this user can visit whatever he wants under this route without being interfered |
|
|
||
| Suppose a new user who have never visited this route before is going to visit it (http://localhost:9080/anything/d?param1=foo¶m2=bar), considering that "auth-casdoor" is enabled, this visit would be processed by "auth-casdoor" plugin first. After checking the session and confirming that this use hasn't been authentication, the visit will be intercepted. After remembering the original url user wants to visit, he will be redirected to the login page of Casdoor. | ||
|
|
||
| After successfully logging in with username and password(or whatever method he use), Casdoor will redirect this user to the "callback_url" with GET parameter "code " and "state" specified. Because the "callback_url" is known by the plugin, so when the visit toward the "callback_url" is intercepted this time, the logic of "Authorization code Grant Flow" in Oauth2 will be triggered, which means this plugin will request the access token to confirm whether this user is really logged in. After this confirmation, this plugin will redirect this user to the original url user wants to visit, which was remembered by us previously. The logged in status will also be remembered by session |
There was a problem hiding this comment.
| After successfully logging in with username and password(or whatever method he use), Casdoor will redirect this user to the "callback_url" with GET parameter "code " and "state" specified. Because the "callback_url" is known by the plugin, so when the visit toward the "callback_url" is intercepted this time, the logic of "Authorization code Grant Flow" in Oauth2 will be triggered, which means this plugin will request the access token to confirm whether this user is really logged in. After this confirmation, this plugin will redirect this user to the original url user wants to visit, which was remembered by us previously. The logged in status will also be remembered by session | |
| After successfully logging in with username and password(or whatever method he uses), Casdoor will redirect this user to the "callback_url" with GET parameter "code " and "state" specified. Because the "callback_url" is known by the plugin, when the visit toward the "callback_url" is intercepted this time, the logic of "Authorization code Grant Flow" in Oauth2 will be triggered, which means this plugin will request the access token to confirm whether this user is really logged in. After this confirmation, this plugin will redirect this user to the original url user wants to visit, which was kept by us previously. The logged-in status will also be kept in the session. |
|
|
||
| local _M = {version = 0.1, priority = 2559, name = plugin_name, schema = schema} | ||
|
|
||
| local function get_path(uri) |
There was a problem hiding this comment.
We can use
apisix/apisix/plugins/http-logger.lua
Line 88 in acf7a0c
to get the path instead of writing own solution.
cc75f06 to
19e132b
Compare
|
@spacewander revisions have been made |
| function _M.access(conf, ctx) | ||
| -- log.info("hit auth-casdoor access") | ||
| local current_uri = ctx.var.uri | ||
| local session_obj_read = session.open() |
There was a problem hiding this comment.
should we check if session exist? ref: bungle/lua-resty-session#12 (comment)
| -- step 1: check whether hits the callback | ||
|
|
||
| if current_uri == conf.callback_url:match(".-//[^/]+(/.*)") then |
There was a problem hiding this comment.
| -- step 1: check whether hits the callback | |
| if current_uri == conf.callback_url:match(".-//[^/]+(/.*)") then | |
| -- step 1: check whether hits the callback | |
| if current_uri == conf.callback_url:match(".-//[^/]+(/.*)") then |
|
|
||
| ## Name | ||
|
|
||
| `auth-casdoor` is an authorization plugin based on [Casdoor](https://casdoor.org/). Casdoor is a centralized authentication / Single-Sign-On (SSO) platform supporting OAuth 2.0, OIDC and SAML, integrated with Casbin RBAC and ABAC permission management |
There was a problem hiding this comment.
| `auth-casdoor` is an authorization plugin based on [Casdoor](https://casdoor.org/). Casdoor is a centralized authentication / Single-Sign-On (SSO) platform supporting OAuth 2.0, OIDC and SAML, integrated with Casbin RBAC and ABAC permission management | |
| `auth-casdoor` is an authorization plugin based on [Casdoor](https://casdoor.org/). Casdoor is a centralized authentication / Single-Sign-On (SSO) platform supporting OAuth 2.0, OIDC and SAML, integrated with Casbin RBAC and ABAC permission management. |
| local cookie = res1.headers["Set-Cookie"] | ||
|
|
||
|
|
||
| local res2,err2=httpc:request_uri(callback_url, { |
There was a problem hiding this comment.
| local res2,err2=httpc:request_uri(callback_url, { | |
| local res2,err2 = httpc:request_uri(callback_url, { |
Please pay more attention to similar formatting issues in this PR.
70a497b to
edd583a
Compare
|
@spacewander @tzssangglass what about this time? (revisions have been made) |
|
@spacewander @tzssangglass what kind of "chaos-test" is this? Besides, comparing with my last previous commit, the only change was format in test file t/plugin/auth-casdoor.t, but I passed all the tests in the previous commit. |
edd583a to
70a497b
Compare
|
what is apisix/utils/batch-processor.lua? This file seemed to lead to the failure of lint but it's irrelevant with me...... |
Please merge the APISIX master branch into your PR branch. |
44e9d82 to
897c7a9
Compare
|
rebase made(force-pushed was caused by rebase) |
| end | ||
| local access_token, lifetime, err = | ||
| fetch_access_token(args.code, conf) | ||
| if err then |
There was a problem hiding this comment.
| if err then | |
| if not access_token then |
| core.log.error(err) | ||
| return 503 | ||
| end | ||
| if access_token then |
| session_obj_write.data.state = state | ||
| session_obj_write:save() | ||
|
|
||
| local redirect_url=conf.endpoint_addr .. "/login/oauth/authorize?" .. ngx.encode_args({ |
There was a problem hiding this comment.
| local redirect_url=conf.endpoint_addr .. "/login/oauth/authorize?" .. ngx.encode_args({ | |
| local redirect_url = conf.endpoint_addr .. "/login/oauth/authorize?" .. ngx.encode_args({ |
|
revisions have been made |
leslie-tsang
left a comment
leslie-tsang
left a comment
There was a problem hiding this comment.
@spacewander @ComradeProgrammer The documentation in the PR still needs to be optimized, let's merge this PR first and deal with the documentation in the next PR ?
|
OK |
|
|
@ComradeProgrammer |
|
Hi @ComradeProgrammer , thank you for your contribution! Here is the Contributor T-shirt form[1], if you're interested, kindly take a look :) |
7 participants
What this PR does / why we need it:
auth-casdooris an authorization plugin based on Casdoor. Casdoor is a centralized authentication / Single-Sign-On (SSO) platformPre-submission checklist: