-
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 support for password grant in authz-keycloak plugin #6586
Changes from 8 commits
f952324
fc96102
07ae576
a2b0d40
fbdf244
250040b
eb60718
7cea982
8f34241
c1d1c8d
bd91138
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -67,7 +67,12 @@ local schema = { | |||||
access_token_expires_leeway = {type = "integer", minimum = 0, default = 0}, | ||||||
refresh_token_expires_in = {type = "integer", minimum = 1, default = 3600}, | ||||||
refresh_token_expires_leeway = {type = "integer", minimum = 0, default = 0}, | ||||||
}, | ||||||
password_grant_token_generation_incoming_uri = { | ||||||
type = "string", | ||||||
minLength = 1, | ||||||
maxLength = 4096 | ||||||
}, | ||||||
}, | ||||||
allOf = { | ||||||
-- Require discovery or token endpoint. | ||||||
{ | ||||||
|
@@ -695,8 +700,87 @@ local function fetch_jwt_token(ctx) | |||||
return token | ||||||
end | ||||||
|
||||||
-- To get new access token by calling get token api | ||||||
local function generate_token_using_password_grant(conf,ctx) | ||||||
log.debug("generate_token_using_password_grant Function Called") | ||||||
|
||||||
local body, err = core.request.get_body() | ||||||
if err or not body then | ||||||
log.error("Failed to get request body: ", err) | ||||||
return 503 | ||||||
end | ||||||
local parameters = ngx.decode_args(body) | ||||||
|
||||||
local username = parameters["username"] | ||||||
local password = parameters["password"] | ||||||
|
||||||
if not username then | ||||||
local err = "username is missing." | ||||||
log.error(err) | ||||||
return 422, err | ||||||
end | ||||||
if not password then | ||||||
local err = "password is missing." | ||||||
log.error(err) | ||||||
return 422, err | ||||||
end | ||||||
|
||||||
local client_id = authz_keycloak_get_client_id(conf) | ||||||
|
||||||
local token_endpoint = authz_keycloak_get_token_endpoint(conf) | ||||||
|
||||||
if not token_endpoint then | ||||||
local err = "Unable to determine token endpoint." | ||||||
log.error(err) | ||||||
return 500, err | ||||||
end | ||||||
local httpc = authz_keycloak_get_http_client(conf) | ||||||
|
||||||
local params = { | ||||||
method = "POST", | ||||||
body = ngx.encode_args({ | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done and also incorporated this in the same file at other places. |
||||||
grant_type = "password", | ||||||
client_id = client_id, | ||||||
client_secret = conf.client_secret, | ||||||
username = username, | ||||||
password = password | ||||||
}), | ||||||
headers = { | ||||||
["Content-Type"] = "application/x-www-form-urlencoded" | ||||||
} | ||||||
} | ||||||
|
||||||
params = authz_keycloak_configure_params(params, conf) | ||||||
|
||||||
local res, err = httpc:request_uri(token_endpoint, params) | ||||||
|
||||||
if not res then | ||||||
err = "Accessing token endpoint URL (" .. token_endpoint | ||||||
.. ") failed: " .. err | ||||||
log.error(err) | ||||||
return 401, {message = err} | ||||||
end | ||||||
|
||||||
log.debug("Response data: " .. res.body) | ||||||
local json, err = authz_keycloak_parse_json_response(res) | ||||||
|
||||||
if not json then | ||||||
err = "Could not decode JSON from response" | ||||||
.. (err and (": " .. err) or '.') | ||||||
log.error(err) | ||||||
return 401, {message = err} | ||||||
end | ||||||
|
||||||
return res.status, res.body | ||||||
end | ||||||
|
||||||
function _M.access(conf, ctx) | ||||||
if conf.password_grant_token_generation_incoming_uri and | ||||||
ngx.var.request_uri:upper() == | ||||||
conf.password_grant_token_generation_incoming_uri:upper() and | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it required to compare case-insensitive? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. URL should be case sensitive so removed that "upper" from code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @spacewander I understood your idea. I am curious, as to why we should treat both strings (/path?query=string or /path) the same way and proceed with token generation. Token generation is a very specific use case, and shouldn't we say if and only if a specific URL will be submitted then we will generate token and submit result back. if the request is made with /path?query=string, it shouldn't be assumed for token generation and it should be treated the same as any other request. (e.g. proceed to forward to upstream for further processing). Please share your view. Accordingly, I can make changes, if needed. |
||||||
core.request.get_method() == "POST" then | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should also check the "Content-Type"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. it was missed. I introduced that now. |
||||||
return generate_token_using_password_grant(conf,ctx) | ||||||
end | ||||||
log.debug("hit keycloak-auth access") | ||||||
local jwt_token, err = fetch_jwt_token(ctx) | ||||||
if not jwt_token then | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -179,6 +179,7 @@ done | |
access_token_expires_leeway = 0, | ||
refresh_token_expires_in = 3600, | ||
refresh_token_expires_leeway = 0, | ||
password_grant_token_generation_incoming_uri = "/api/token", | ||
}) | ||
if not ok then | ||
ngx.say(err) | ||
|
@@ -621,3 +622,104 @@ GET /t | |
--- response_headers | ||
Location: http://127.0.0.1/test | ||
--- error_code: 307 | ||
|
||
|
||
|
||
=== TEST 18: Add https endpoint with password_grant_token_generation_incoming_uri | ||
--- config | ||
location /t { | ||
content_by_lua_block { | ||
local t = require("lib.test_admin").test | ||
local code, body = t('/apisix/admin/routes/1', | ||
ngx.HTTP_PUT, | ||
[[{ | ||
"plugins": { | ||
"authz-keycloak": { | ||
"token_endpoint": "https://127.0.0.1:8443/auth/realms/University/protocol/openid-connect/token", | ||
"permissions": ["course_resource#view"], | ||
"client_id": "course_management", | ||
"client_secret": "d1ec69e9-55d2-4109-a3ea-befa071579d5", | ||
"grant_type": "urn:ietf:params:oauth:grant-type:uma-ticket", | ||
"timeout": 3000, | ||
"ssl_verify": false, | ||
"password_grant_token_generation_incoming_uri": "/api/token" | ||
} | ||
}, | ||
"upstream": { | ||
"nodes": { | ||
"127.0.0.1:1982": 1 | ||
}, | ||
"type": "roundrobin" | ||
}, | ||
"uri": "/api/token" | ||
}]], | ||
[[{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to remove the echo response data as #6545 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alright. I removed it from this new test case. |
||
"node": { | ||
"value": { | ||
"plugins": { | ||
"authz-keycloak": { | ||
"token_endpoint": "https://127.0.0.1:8443/auth/realms/University/protocol/openid-connect/token", | ||
"permissions": ["course_resource#view"], | ||
"client_id": "course_management", | ||
"client_secret": "d1ec69e9-55d2-4109-a3ea-befa071579d5", | ||
"grant_type": "urn:ietf:params:oauth:grant-type:uma-ticket", | ||
"timeout": 3000, | ||
"ssl_verify": false, | ||
"password_grant_token_generation_incoming_uri": "/api/token" | ||
} | ||
}, | ||
"upstream": { | ||
"nodes": { | ||
"127.0.0.1:1982": 1 | ||
}, | ||
"type": "roundrobin" | ||
}, | ||
"uri": "/api/token" | ||
}, | ||
"key": "/apisix/routes/1" | ||
}, | ||
"action": "set" | ||
}]] | ||
) | ||
|
||
if code >= 300 then | ||
ngx.status = code | ||
end | ||
|
||
local json_decode = require("toolkit.json").decode | ||
local http = require "resty.http" | ||
local httpc = http.new() | ||
local uri = "http://127.0.0.1:" .. ngx.var.server_port .. "/api/token" | ||
local res, err = httpc:request_uri(uri, { | ||
method = "POST", | ||
headers = { | ||
["Content-Type"] = "application/x-www-form-urlencoded", | ||
}, | ||
|
||
body = ngx.encode_args({ | ||
username = "[email protected]", | ||
password = "123456", | ||
}), | ||
}) | ||
|
||
if res.status == 200 then | ||
local body = json_decode(res.body) | ||
local accessToken = body["access_token"] | ||
local refreshToken = body["refresh_token"] | ||
|
||
if accessToken and refreshToken then | ||
ngx.say(true) | ||
else | ||
ngx.say(false) | ||
end | ||
else | ||
ngx.say(false) | ||
end | ||
} | ||
} | ||
--- request | ||
GET /t | ||
--- response_body | ||
true | ||
--- no_error_log | ||
[error] |
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.
New feature is encouraged to avoid 500
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.
Valid point. I updated it. I didn't update at other places because there can be backward compatibility issues and there should be separate PR for the same.