You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "csotiriou (via GitHub)" <gi...@apache.org> on 2023/11/15 08:05:41 UTC
[PR] feat: add `required scopes` configuration property to `openid-connect` plugin [apisix]
csotiriou opened a new pull request, #10493:
URL: https://github.com/apache/apisix/pull/10493
Fixes #10352
This adds the optional `required_scopes` configuration property in the `openid-connect` plugin configuration. In cases where the introspection endpoint of the OIDC server is called, the plugin will check if all required scopes are present in the scopes returned by the introspection endpoint.
I have also updated the documentation to reflect this, in case anyone finds this PR valuable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add `required scopes` configuration property to `openid-connect` plugin [apisix]
Posted by "soulbird (via GitHub)" <gi...@apache.org>.
soulbird commented on code in PR #10493:
URL: https://github.com/apache/apisix/pull/10493#discussion_r1396674016
##########
apisix/plugins/openid-connect.lua:
##########
@@ -315,6 +324,24 @@ local function add_access_token_header(ctx, conf, token)
end
end
+-- Function to split the scope string into a table
+local function split_scopes_by_space(scope_string)
+ local scopes = {}
+ for scope in string.gmatch(scope_string, "%S+") do
+ scopes[scope] = true
+ end
+ return scopes
+end
+
+-- Function to check if all required scopes are present
+local function required_scopes_present(required_scopes, http_scopes)
Review Comment:
you can use `core.table.array_find` directly: https://github.com/apache/apisix/blob/master/apisix/core/table.lua#L94
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add `required scopes` configuration property to `openid-connect` plugin [apisix]
Posted by "moonming (via GitHub)" <gi...@apache.org>.
moonming commented on PR #10493:
URL: https://github.com/apache/apisix/pull/10493#issuecomment-1812466331
@csotiriou thanks for this PR. It will be great if you can add test cases for it :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add `required scopes` configuration property to `openid-connect` plugin [apisix]
Posted by "soulbird (via GitHub)" <gi...@apache.org>.
soulbird commented on PR #10493:
URL: https://github.com/apache/apisix/pull/10493#issuecomment-1818185201
> > I think you need to add a test case for scope validation
>
> @soulbird Do you mean the case where the correct scope is passed and the request succeeds?
yes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add `required scopes` configuration property to `openid-connect` plugin [apisix]
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #10493:
URL: https://github.com/apache/apisix/pull/10493#discussion_r1399985810
##########
apisix/plugins/openid-connect.lua:
##########
@@ -319,6 +328,24 @@ local function add_access_token_header(ctx, conf, token)
end
end
+-- Function to split the scope string into a table
+local function split_scopes_by_space(scope_string)
+ local scopes = {}
+ for scope in string.gmatch(scope_string, "%S+") do
Review Comment:
Why not use [ngx.re.split](https://github.com/openresty/lua-resty-core/blob/master/lib/ngx/re.md) ?
##########
apisix/plugins/openid-connect.lua:
##########
@@ -319,6 +328,24 @@ local function add_access_token_header(ctx, conf, token)
end
end
+-- Function to split the scope string into a table
+local function split_scopes_by_space(scope_string)
+ local scopes = {}
+ for scope in string.gmatch(scope_string, "%S+") do
+ scopes[scope] = true
+ end
+ return scopes
+end
+
+-- Function to check if all required scopes are present
Review Comment:
Two blanks are needed between functions
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add `required scopes` configuration property to `openid-connect` plugin [apisix]
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #10493:
URL: https://github.com/apache/apisix/pull/10493#discussion_r1399985810
##########
apisix/plugins/openid-connect.lua:
##########
@@ -319,6 +328,24 @@ local function add_access_token_header(ctx, conf, token)
end
end
+-- Function to split the scope string into a table
+local function split_scopes_by_space(scope_string)
+ local scopes = {}
+ for scope in string.gmatch(scope_string, "%S+") do
Review Comment:
Why not use [ngx.re.split](https://github.com/openresty/lua-resty-core/blob/master/lib/ngx/re.md) ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add `required scopes` configuration property to `openid-connect` plugin [apisix]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #10493:
URL: https://github.com/apache/apisix/pull/10493#discussion_r1396840328
##########
apisix/plugins/openid-connect.lua:
##########
@@ -315,6 +324,24 @@ local function add_access_token_header(ctx, conf, token)
end
end
+-- Function to split the scope string into a table
+local function split_scopes_by_space(scope_string)
+ local scopes = {}
+ for scope in string.gmatch(scope_string, "%S+") do
+ scopes[scope] = true
+ end
+ return scopes
+end
+
+-- Function to check if all required scopes are present
+local function required_scopes_present(required_scopes, http_scopes)
Review Comment:
array_find(arr,ele) checks if `ele` is present in `arr`.
This function checks if all the elements in first table are present in second table.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add `required scopes` configuration property to `openid-connect` plugin [apisix]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on PR #10493:
URL: https://github.com/apache/apisix/pull/10493#issuecomment-1815195781
@moonming @csotiriou I have added the test case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add `required scopes` configuration property to `openid-connect` plugin [apisix]
Posted by "soulbird (via GitHub)" <gi...@apache.org>.
soulbird commented on PR #10493:
URL: https://github.com/apache/apisix/pull/10493#issuecomment-1815711227
I think you need to add a test case for scope validation
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add `required scopes` configuration property to `openid-connect` plugin [apisix]
Posted by "moonming (via GitHub)" <gi...@apache.org>.
moonming commented on PR #10493:
URL: https://github.com/apache/apisix/pull/10493#issuecomment-1818078911
> Conflicting files
> docs/en/latest/plugins/openid-connect.md
@csotiriou please fix this when you have time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add `required scopes` configuration property to `openid-connect` plugin [apisix]
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #10493:
URL: https://github.com/apache/apisix/pull/10493#discussion_r1399986036
##########
apisix/plugins/openid-connect.lua:
##########
@@ -319,6 +328,24 @@ local function add_access_token_header(ctx, conf, token)
end
end
+-- Function to split the scope string into a table
+local function split_scopes_by_space(scope_string)
+ local scopes = {}
+ for scope in string.gmatch(scope_string, "%S+") do
+ scopes[scope] = true
+ end
+ return scopes
+end
+
+-- Function to check if all required scopes are present
Review Comment:
Two blanks are needed between functions
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add `required scopes` configuration property to `openid-connect` plugin [apisix]
Posted by "moonming (via GitHub)" <gi...@apache.org>.
moonming commented on PR #10493:
URL: https://github.com/apache/apisix/pull/10493#issuecomment-1813618919
@Revolyssup Please check how to write a more appropriate test case. I am not familiar with this plug-in.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add `required scopes` configuration property to `openid-connect` plugin [apisix]
Posted by "moonming (via GitHub)" <gi...@apache.org>.
moonming commented on PR #10493:
URL: https://github.com/apache/apisix/pull/10493#issuecomment-1815705578
@csotiriou thanks for your great work 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add `required scopes` configuration property to `openid-connect` plugin [apisix]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on PR #10493:
URL: https://github.com/apache/apisix/pull/10493#issuecomment-1815912710
> I think you need to add a test case for scope validation
@soulbird Do you mean the case where the correct scope is passed and the request succeeds?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add `required scopes` configuration property to `openid-connect` plugin [apisix]
Posted by "csotiriou (via GitHub)" <gi...@apache.org>.
csotiriou commented on PR #10493:
URL: https://github.com/apache/apisix/pull/10493#issuecomment-1818311316
> > Conflicting files
> > docs/en/latest/plugins/openid-connect.md
>
> @csotiriou please fix this when you have time.
Thanks for the notification, @moonming , It seems that the merge conflicts have been resolved by @Revolyssup faster, thank you for the help!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add `required scopes` configuration property to `openid-connect` plugin [apisix]
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 merged PR #10493:
URL: https://github.com/apache/apisix/pull/10493
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add `required scopes` configuration property to `openid-connect` plugin [apisix]
Posted by "csotiriou (via GitHub)" <gi...@apache.org>.
csotiriou commented on PR #10493:
URL: https://github.com/apache/apisix/pull/10493#issuecomment-1812790459
Hello @moonming I'm trying to wrap my head around the test implementation, my familiarity with lua is limited. However, I have reached the point where I'm making a sample test.
I noticed that for the openid-connect plugin there are some sample oidc providers with a dummy client-id and client-secret used. Must I use the same and have the same configuration in my keycloak? If not, how are tests going to pass during automation?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org