-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
feat: add auth plugin for casdoor #6382
Conversation
6bfe556
to
753238b
Compare
753238b
to
2f138c8
Compare
465685d
to
6656e06
Compare
apisix/plugins/auth-casdoor.lua
Outdated
local schema = { | ||
type = "object", | ||
properties = { | ||
casdoor_endpoint = { type = "string" }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove the casdoor_ prefix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ComradeProgrammer
Please request me to review it once you are ready. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
apisix/plugins/auth-casdoor.lua
Outdated
local schema = { | ||
type = "object", | ||
properties = { | ||
casdoor_endpoint = { type = "string" }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove the casdoor_ prefix
apisix/plugins/auth-casdoor.lua
Outdated
local core = require("apisix.core") | ||
local log = core.log | ||
local ngx = ngx | ||
local session = require("resty.session") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's sort the imports like
apisix/apisix/plugins/authz-casbin.lua
Line 20 in 4624135
local plugin = require("apisix.plugin") |
conf/config-default.yaml
Outdated
@@ -340,6 +340,7 @@ plugins: # plugin list (sorted by priority) | |||
- request-validation # priority: 2800 | |||
- openid-connect # priority: 2599 | |||
- authz-casbin # priority: 2560 | |||
- auth-casdoor # priority: 2561 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please sort according to the priority and fix the indent
apisix/plugins/auth-casdoor.lua
Outdated
|
||
local _M = { | ||
version = 0.1, | ||
priority = 12, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
priority mismatch
apisix/plugins/auth-casdoor.lua
Outdated
|
||
if session_obj_read.data.access_token then | ||
-- session exists | ||
log.warn("session exists") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto. Warn log should not be used in per-req level
apisix/plugins/auth-casdoor.lua
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return nil | |
return nil, err |
and log the err outside would be better
apisix/plugins/auth-casdoor.lua
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's check if the endpoint contains ?
in the schema instead of split it at runtime
apisix/plugins/auth-casdoor.lua
Outdated
log.err("failed when accessing token. ", err) | ||
return nil | ||
end | ||
local data = cjson.decode(res.body) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
apisix/plugins/auth-casdoor.lua
Outdated
client_id = { type = "string" }, | ||
client_secret = { type = "string" }, | ||
callback_url = { type = "string" }, | ||
test = { type= "boolean", default = false } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test
is not in Attributes
of plugin docs
apisix/plugins/auth-casdoor.lua
Outdated
end | ||
|
||
function _M.access(conf, ctx) | ||
log.info("hit auth-casdoor access") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line looks like it's only for debugging?
apisix/plugins/auth-casdoor.lua
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The err
doesn't have parameters in it
apisix/plugins/auth-casdoor.lua
Outdated
end | ||
|
||
local function fetch_access_token(conf) | ||
local args = core.request.get_uri_args() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
local args = core.request.get_uri_args() | |
local args = core.request.get_uri_args(ctx) |
and should pass ctx
to this function
apisix/plugins/auth-casdoor.lua
Outdated
|
||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's better to determine if args
is empty
apisix/plugins/auth-casdoor.lua
Outdated
end | ||
|
||
function _M.check_schema(conf) | ||
local ok,err=core.schema.check(schema, conf) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
local ok,err=core.schema.check(schema, conf) | |
local ok, err = core.schema.check(schema, conf) |
apisix/plugins/auth-casdoor.lua
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use ngx.re.find
is better?
apisix/plugins/auth-casdoor.lua
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
apisix/plugins/auth-casdoor.lua
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return 503,"no original_url found in session" | |
return 503, "no original_url found in session" |
apisix/plugins/auth-casdoor.lua
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
apisix/plugins/auth-casdoor.lua
Outdated
local schema = { | ||
type = "object", | ||
properties = { | ||
casdoor_endpoint = {type = "string"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
casdoor_endpoint = {type = "string"}, | |
endpoint_addr = {type = "string"}, |
would be better?
apisix/plugins/auth-casdoor.lua
Outdated
} | ||
} | ||
|
||
local _M = {version = 0.1, priority = 2559, name = plugin_name, schema = schema} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please put each field per line, like other plugins
apisix/plugins/auth-casdoor.lua
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The err
doesn't have parameters in it
apisix/plugins/auth-casdoor.lua
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we require the endpoint not to have ?
so we don't need to use get_path
?
apisix/plugins/auth-casdoor.lua
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do the check in the schema via "pattern":
Line 34 in acf7a0c
pattern = [[^[a-zA-Z0-9-_.]+$]] |
|
||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing this one?
|
||
#### 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
apisix/plugins/auth-casdoor.lua
Outdated
|
||
local _M = {version = 0.1, priority = 2559, name = plugin_name, schema = schema} | ||
|
||
local function get_path(uri) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use
apisix/apisix/plugins/http-logger.lua
Line 88 in acf7a0c
local url_decoded = url.parse(conf.uri) |
to get the path instead of writing own solution.
cc75f06
to
19e132b
Compare
@spacewander revisions have been made |
apisix/plugins/auth-casdoor.lua
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we check if session exist? ref: bungle/lua-resty-session#12 (comment)
apisix/plugins/auth-casdoor.lua
Outdated
-- step 1: check whether hits the callback | ||
|
||
if current_uri == conf.callback_url:match(".-//[^/]+(/.*)") then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`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. |
t/plugin/auth-casdoor.t
Outdated
local cookie = res1.headers["Set-Cookie"] | ||
|
||
|
||
local res2,err2=httpc:request_uri(callback_url, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
apisix/plugins/authz-casdoor.lua
Outdated
end | ||
local access_token, lifetime, err = | ||
fetch_access_token(args.code, conf) | ||
if err then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if err then | |
if not access_token then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revised
apisix/plugins/authz-casdoor.lua
Outdated
core.log.error(err) | ||
return 503 | ||
end | ||
if access_token then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This branch can be saved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revised
apisix/plugins/authz-casdoor.lua
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
local redirect_url=conf.endpoint_addr .. "/login/oauth/authorize?" .. ngx.encode_args({ | |
local redirect_url = conf.endpoint_addr .. "/login/oauth/authorize?" .. ngx.encode_args({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revised
revisions have been made |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 :) [1] https://github.com/apache/apisix/blob/master/CONTRIBUTING.md#contributor-t-shirt |
What this PR does / why we need it:
auth-casdoor
is an authorization plugin based on Casdoor. Casdoor is a centralized authentication / Single-Sign-On (SSO) platformPre-submission checklist: