feat: add support for password grant in authz-keycloak plugin by azilentech · Pull Request #6586 · apache/apisix

azilentech · 2022-03-11T14:26:18Z

Support of password grant type for token generation (#6574)

Committing changes after adding new feature for generating token based on user id/password and also taking support of grant type password

feat: #6574

Pre-submission checklist:

Did you explain what problem does this PR solve? Or what new features have been added?
Have you added corresponding test cases?
Have you modified the corresponding document?
Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

Squashed commit: [99ff9f4] refactor: Removing unnecessary properties (apache#6574) Removing unnecessary properties and test codes. And also improve document for Token generation feature. feat: apache#6574 [22c8d8e] feat: Support of password grant type for token generation (apache#6574) Committing changes after adding new feature for generating token based on `user id/password` and also taking support of grant type `password` feat: apache#6574

soulbird · 2022-03-12T00:53:45Z

you should sing the CLA first

soulbird · 2022-03-12T01:16:49Z

Missing unit tests

Better variable names. Documentation updated as per guideline.

azilentech · 2022-03-12T18:17:45Z

Hi,

I will need little guidance for the CLA signing process from your side.

In PR, I have made the following changes.

Added test cases. As these test cases require keycloak installation, I tested with my local environment by adjusting parameters and configuration settings. Please let me know If I need to perform further steps.
I updated code by using better configuration setting "password_grant_token_generation_incoming_uri" instead of using "token_generation_endpoint" which can be confusing with "token_endpoint".
I updated the documentation by following contribution guidelines.
I also ran "make lint" and followed all the suggestions.

Please let me know the next actions for me.

…using password_grant_token_generation_incoming_uri. I have removed some more errors reported by git workflow build process.

spacewander · 2022-03-13T12:15:32Z

I will need little guidance for the CLA signing process from your side.

There is no CLA file for APISIX :)

Added test cases. As these test cases require keycloak installation, I tested with my local environment by adjusting parameters and configuration settings. Please let me know If I need to perform further steps.

As the keycloak environment is set up by @sshniro,
@sshniro
Could you provide some help for @azilentech to test this feature? Thanks!

niketrk · 2022-03-13T16:58:37Z

@sshniro

I checked the logs of above test runs.
error is: "status=401, body={"error":"invalid_grant","error_description":"Invalid user credentials"}"

I have used following settings in my test scenario:
"client_id": "course_management"
"client_secret": "d1ec69e9-55d2-4109-a3ea-befa071579d5"
username = "[email protected]"
password = "123456"
grant_type = "password"

I took the above settings from other Keycloak test cases.
I am doubting that these settings are not proper for the test Keycloak instance. In my local environment, I put my Keycloak settings and then run test scenario via
TEST_NGINX_BINARY=/usr/local/bin/openresty prove -Itest-nginx/lib -r t/plugin/authz-keycloak.t

And it worked properly.

Another issue is reported as: "ERROR: apisix/plugins/authz-keycloak.lua: line 719: getting the Lua global "string""

For which I made fix "local str_find = string.find", so it should be fixed now.

starsz · 2022-03-14T01:20:09Z


+-- To get new access token by calling get token api
+local function generate_token_using_password_grant(conf,ctx)
+    log.warn("generate_token_using_password_grant Function Called")


Why should we use warn level ?

Agree... it should be debug level log. I changed it.

starsz · 2022-03-14T01:20:37Z

+    --Read Body
+    ngx.req.read_body()
+    --Get Body Data
+    local request_body=ngx.req.get_body_data()


You can use core.request.get_body() instead.

Done. I changed it.

starsz · 2022-03-14T01:22:52Z

+        return 401, {message = err}
+    end
+
+    return  res.status, res.body


Suggested change

return res.status, res.body

return res.status, res.body

starsz · 2022-03-14T01:23:36Z

+
+    if conf.password_grant_token_generation_incoming_uri then
+        if ngx.var.request_uri:upper()
+                == conf.password_grant_token_generation_incoming_uri:upper() then


Use and would be better.

Good Suggestion and changed it to use and for all three conditions.

starsz · 2022-03-14T01:30:39Z

+    local username = nil
+    local password = nil
+    --split by &
+    local parameters_array = util.split(request_body, "&")


Why the username, password in the request body and use & to concat.

As url encoded parameters were coming, they were automatically appended using '&'. Having said that, your comment helped me to utilize "ngx.decode_args" which makes code much cleaner. Thanks!

Used ngx.decode_args for input arguments.

sshniro · 2022-03-15T11:18:02Z

scenario

Hi, are you getting the same error when you run the docker image instance (sshniro/keycloak-apisix:latest) locally?

azilentech · 2022-03-15T11:54:42Z

@sshniro
I didn't test that way earlier, but just now, I did that and it is working. Also when I see the above checks, that test case is starting to pass now.

I have got the idea what things might cause that. Earlier I was using '&' to split the incoming parameters. but now I am using "ngx.decode_args" ( #6586 (comment) ).

Because of that earlier, username (which is email) wouldn't parse properly.
That should be the root cause of the same.

starsz · 2022-03-15T14:16:12Z

+    if conf.password_grant_token_generation_incoming_uri and
+        ngx.var.request_uri:upper() ==
+        conf.password_grant_token_generation_incoming_uri:upper() and
+        ctx.curr_req_matched["_method"]:upper() == "POST" then


ctx.curr_req_matched["_method"]:upper() looks strange.
I think you can use core.req.get_method instead.

starsz · 2022-03-15T14:16:39Z

+end

 function _M.access(conf, ctx)
+


redundant line ?

starsz · 2022-03-15T14:17:25Z


+### Password Grant Token Generation Incoming URI
+
+If you want to generate a token using `password` grant, you can set value of `password_grant_token_generation_incoming_uri`.


Suggested change

If you want to generate a token using `password` grant, you can set value of `password_grant_token_generation_incoming_uri`.

If you want to generate a token using `password` grant, you can set the value of `password_grant_token_generation_incoming_uri`.

starsz · 2022-03-15T14:18:08Z

+
+If you want to generate a token using `password` grant, you can set value of `password_grant_token_generation_incoming_uri`.
+
+Incoming request URI will be matched with this value and if matched, it will generate token using `Token Endpoint`.


Suggested change

Incoming request URI will be matched with this value and if matched, it will generate token using `Token Endpoint`.

Incoming request URI will be matched with this value and if matched, it will generate a token using `Token Endpoint`.

starsz · 2022-03-15T14:18:34Z

+If you want to generate a token using `password` grant, you can set value of `password_grant_token_generation_incoming_uri`.
+
+Incoming request URI will be matched with this value and if matched, it will generate token using `Token Endpoint`.
+It will also check, if REST method is `POST`.


Suggested change

It will also check, if REST method is `POST`.

It will also check if the request method is `POST`.

spacewander · 2022-03-17T02:26:30Z

+
+    local params = {
+        method = "POST",
+        body =  ngx.encode_args({


Suggested change

body = ngx.encode_args({

body = ngx.encode_args({

done and also incorporated this in the same file at other places.

spacewander · 2022-03-17T02:29:03Z

+    if not token_endpoint then
+        local err = "Unable to determine token endpoint."
+        log.error(err)
+        return 500, err


Suggested change

return 500, err

return 503, err

New feature is encouraged to avoid 500

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.

spacewander · 2022-03-17T02:29:43Z

+    if conf.password_grant_token_generation_incoming_uri and
+        ngx.var.request_uri:upper() ==
+        conf.password_grant_token_generation_incoming_uri:upper() and
+        core.request.get_method() == "POST" then


We should also check the "Content-Type"?

Yes. it was missed. I introduced that now.

spacewander · 2022-03-17T02:31:52Z

 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


Is it required to compare case-insensitive?
Also, the var.request_uri will contain ?argument=part, is it desirable?

URL should be case sensitive so removed that "upper" from code.
For the part of request_uri, I believe, as we are checking uri, method, and header, all three parts, this should be fine.
Basically for the operation of token generation, exact uri matching will be helpful.
Your thoughts?

The request_uri will contain the query string part of the uri. For example,
With request /path?query=string
The ngx.var.request_uri will return /path?query=string, and ctx.var.uri will return /path. By looking at the doc of password_grant_token_generation_incoming_uri, I guess we can use ctx.var.uri so both /path?query=string and /path can match.

@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.

spacewander · 2022-03-17T02:33:16Z

+                        },
+                        "uri": "/api/token"
+                }]],
+                [[{


We need to remove the echo response data as #6545

Alright. I removed it from this new test case.

spacewander

Approved except #6586 (comment)

…bility

spacewander · 2022-03-20T06:20:46Z

 function _M.access(conf, ctx)
+    local headers = core.request.headers(ctx)
+    local need_grant_token = conf.password_grant_token_generation_incoming_uri and
+        ngx.var.request_uri == conf.password_grant_token_generation_incoming_uri and


Suggested change

ngx.var.request_uri == conf.password_grant_token_generation_incoming_uri and

ctx.var.request_uri == conf.password_grant_token_generation_incoming_uri and

would be better, which provides a cache of the var.

Done. Made the change.

lijing-21 · 2022-04-18T06:37:29Z

Hi @azilentech , 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

azilentech changed the title ~~feat-6574-keycloak-plugin-password-grant-support~~ feat:6574 - keycloak plugin password grant support Mar 11, 2022

azilentech changed the title ~~feat:6574 - keycloak plugin password grant support~~ feat - keycloak plugin password grant support Mar 11, 2022

azilentech changed the title ~~feat - keycloak plugin password grant support~~ feat: keycloak plugin password grant support Mar 11, 2022

azilentech changed the title ~~feat: keycloak plugin password grant support~~ feat: add support for password grant in keycloak plugin Mar 11, 2022

azilentech mentioned this pull request Mar 11, 2022

feat: add support for password grant in keycloak plugin and in dashboard apache/apisix-dashboard#2376

Closed

7 tasks

azilentech force-pushed the feat-6574-keycloak-plugin-password-grant-support branch from e555a00 to 3d1e27b Compare March 12, 2022 05:43

Refactored code.

fc96102

Better variable names. Documentation updated as per guideline.

azilentech force-pushed the feat-6574-keycloak-plugin-password-grant-support branch from 3d1e27b to fc96102 Compare March 12, 2022 12:37

azilentech added 2 commits March 12, 2022 22:56

Test cases added.

07ae576

Used apisix util split method.

a2b0d40

I have combined test cases where route is set and token is generated …

fbdf244

…using password_grant_token_generation_incoming_uri. I have removed some more errors reported by git workflow build process.

spacewander requested a review from sshniro March 13, 2022 12:15

used local str_find

250040b

starsz reviewed Mar 14, 2022

View reviewed changes

Review comments incorporated.

eb60718

Used ngx.decode_args for input arguments.

azilentech requested a review from starsz March 15, 2022 04:45

starsz reviewed Mar 15, 2022

View reviewed changes

Review comments incorporated.

7cea982

starsz previously approved these changes Mar 16, 2022

View reviewed changes

spacewander reviewed Mar 17, 2022

View reviewed changes

Incorporated review comments:

8f34241

azilentech dismissed starsz’s stale review via 8f34241 March 17, 2022 05:00

azilentech requested a review from spacewander March 17, 2022 05:05

spacewander previously approved these changes Mar 18, 2022

View reviewed changes

tzssangglass reviewed Mar 18, 2022

View reviewed changes

Comment thread apisix/plugins/authz-keycloak.lua Outdated

azilentech dismissed spacewander’s stale review via 6f04d1a March 18, 2022 12:36

introduced need_grant_token for composite conditions for better reada…

c1d1c8d

…bility

azilentech force-pushed the feat-6574-keycloak-plugin-password-grant-support branch from 6f04d1a to c1d1c8d Compare March 18, 2022 12:58

spacewander previously approved these changes Mar 20, 2022

View reviewed changes

used ctx.var instead of ngx.var

bd91138

azilentech dismissed spacewander’s stale review via bd91138 March 20, 2022 16:13

azilentech requested review from spacewander, starsz and tzssangglass March 20, 2022 16:15

spacewander approved these changes Mar 21, 2022

View reviewed changes

tzssangglass approved these changes Mar 21, 2022

View reviewed changes

spacewander merged commit 66e944c into apache:master Mar 21, 2022

Rushabh-Sukhadia mentioned this pull request Mar 23, 2022

feat: keycloak password grant token generation uri property apache/apisix-dashboard#2390

Closed

4 tasks

kayx23 changed the title ~~feat: add support for password grant in keycloak plugin~~ feat: add support for password grant in authz-keycloak plugin Dec 28, 2023


		### Password Grant Token Generation Incoming URI

		If you want to generate a token using `password` grant, you can set value of `password_grant_token_generation_incoming_uri`.


		If you want to generate a token using `password` grant, you can set value of `password_grant_token_generation_incoming_uri`.

		Incoming request URI will be matched with this value and if matched, it will generate token using `Token Endpoint`.

	It will also check, if REST method is `POST`.
	It will also check if the request method is `POST`.

	ngx.var.request_uri == conf.password_grant_token_generation_incoming_uri and
	ctx.var.request_uri == conf.password_grant_token_generation_incoming_uri and

Uh oh!

Conversation

azilentech commented Mar 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-submission checklist:

Uh oh!

soulbird commented Mar 12, 2022

Uh oh!

soulbird commented Mar 12, 2022

Uh oh!

azilentech commented Mar 12, 2022

Uh oh!

spacewander commented Mar 13, 2022

Uh oh!

niketrk commented Mar 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sshniro commented Mar 15, 2022

Uh oh!

azilentech commented Mar 15, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azilentech Mar 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacewander left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

azilentech commented Mar 11, 2022 •

edited

Loading

azilentech Mar 17, 2022 •

edited

Loading