Implement_Access_Control_in_auth-service#3 #18

2025-05-14T23:35:42Z

abed.alrahman commented

2025-05-14 23:35:42 +00:00

This PR should be after PR #16, It contains the code for Access control using Keycloak groups and roles.

hurui200320 was assigned by abed.alrahman

2025-05-14 23:35:42 +00:00

stanislav.hejny was assigned by abed.alrahman

2025-05-14 23:35:42 +00:00

abed.alrahman added 3 commits 2025-05-14 23:35:43 +00:00

Group added to Verfication response, 9889789fcc

add auth token to warn message 33182d5a8e

1. Implement Access Control in auth-service via Traefik Forward Auth 66767d9e1e

2. Add keycloak admin client, to fetch as admin from keycloak
3. Add /auth endpoint, that will parse the request, and decide for auth

stanislav.hejny was unassigned by abed.alrahman

2025-05-14 23:36:25 +00:00

hurui200320 was unassigned by abed.alrahman

2025-05-14 23:36:26 +00:00

abed.alrahman self-assigned this 2025-05-14 23:36:30 +00:00

abed.alrahman requested review from stanislav.hejny 2025-05-14 23:36:41 +00:00

abed.alrahman requested review from hurui200320 2025-05-14 23:36:42 +00:00

abed.alrahman added this to the V.01 milestone 2025-05-14 23:36:55 +00:00

abed.alrahman added a new dependency 2025-05-14 23:42:09 +00:00

infrastructure/infrastructure#38 - re-enable email for services we disabled it for SES debugging

abed.alrahman removed a dependency 2025-05-14 23:42:21 +00:00

infrastructure/infrastructure#38 - re-enable email for services we disabled it for SES debugging

abed.alrahman added a new dependency 2025-05-20 01:18:28 +00:00

#3 Implement Access Control in auth-service via Traefik Forward Auth

abed.alrahman added 1 commit 2025-05-20 01:27:24 +00:00

Add more tests cases to test class, to test auth endpoint as well ade56ec23d

abed.alrahman added a new dependency 2025-05-20 01:38:49 +00:00

#15 Unit test: Implement Access Control in auth-service

hurui200320 requested changes 2025-05-20 06:07:09 +00:00

Dismissed

src/main/java/com/cleverthis/authservice/controller/AuthController.java

						
				@@ -89,0 +316,4 @@

				     * Constructs the required Keycloak Client Role name based on the HTTP method and relative path.

				     * Example: GET /api/jobs/{id} -> GET_api_jobs_BY_ID Example: POST /api/users -> POST_api_users

				     */

				    private String constructRequiredRoleName(String httpMethod, String relativePath) {

hurui200320 commented

2025-05-20 05:51:58 +00:00

If the path contains underline, this solution may not work well.

And according to this doc https://docs.cleverthis.com/en/architecture/microservices/feature-discussion/user-privilege-design, we need a lookup table to figure out which roles the user has and which role gives the access for the certain path, instead of finding roles based on url itself.

If the path contains underline, this solution may not work well. And according to this doc https://docs.cleverthis.com/en/architecture/microservices/feature-discussion/user-privilege-design, we need a lookup table to figure out which roles the user has and which role gives the access for the certain path, instead of finding roles based on url itself.

abed.alrahman commented

2025-05-21 00:33:46 +00:00

Path will never contain an underscore(underline), this: Example: GET /api/jobs/{id} -> GET_api_jobs_BY_ID
means how we automatically parse the request method and URI, here request: method=GET URI=/api/jobs/{id}
will be parsed as the role name (GET_api_jobs_BY_ID), then this role name should exist in the Keycloak roles, I showed this in the last demo, but I will show it again in the next spring review

Path will never contain an underscore(underline), this: Example: GET /api/jobs/{id} -> GET_api_jobs_BY_ID means how we automatically parse the request method and URI, here request: method=GET URI=/api/jobs/{id} will be parsed as the role name (GET_api_jobs_BY_ID), then this role name should exist in the Keycloak roles, I showed this in the last demo, but I will show it again in the next spring review

hurui200320 commented

2025-05-21 08:56:03 +00:00

According to the document I mentioned earlier, the role name itself shouldn't indicate the permission itself. The role name is simply a name for humans to distinguish one role from another. The permission itself is described by the role's content, as documented in the design:

role:
  id: "role_789"
  name: "job_editor"
  description: "Can modify job statuses"
  privileges: ["priv_456", "priv_789"]  # List of privilege IDs

The privileges grant actual access permission. And a privilege is defined here:

privilege:
  id: "priv_456"
  name: "job_edit"
  description: "Allows editing job status"
  resource_id: "resource_123"  # Links to a resource
  action: "read" | "write" | "delete" | "execute"  # CRUD-like

The resource field describes the scope of this privilege:

resource:
  id: "job_list_1"  # Unique identifier
  type: "API" | "DATA" | "MANAGEMENT"  # Resource category
  path: "/api/jobs/{id}"  # Exact path or regex
  service: "cleverswarm"  # Which service owns it
  is_regex: false  # Whether path is a regex pattern

This is where the path lives.

Unless I'm reading out-of-date documentation (Stan shares this one in the group chat on May 1st), I didn't find any design document that matches your implementation in this PR. If you have the link to the document you're following with, please do share it. Thanks.

Also, a path can contain an underscore, since it's a valid URL. For example, a rest endpoint to manage digital books, I might give the path /api/digital_books/, although it looks weird and is generally not recommended (should use digital-books), the URL is legal and works with other systems. I couldn't see why we must add such limits to exclude underscores from URL path.

Ref: https://datatracker.ietf.org/doc/html/rfc3986#section-2.3

According to the document I mentioned earlier, the role name itself shouldn't indicate the permission itself. The role name is simply a name for humans to distinguish one role from another. The permission itself is described by the role's content, as documented in the design: ``` role: id: "role_789" name: "job_editor" description: "Can modify job statuses" privileges: ["priv_456", "priv_789"] # List of privilege IDs ``` The privileges grant actual access permission. And a privilege is defined here: ``` privilege: id: "priv_456" name: "job_edit" description: "Allows editing job status" resource_id: "resource_123" # Links to a resource action: "read" | "write" | "delete" | "execute" # CRUD-like ``` The resource field describes the scope of this privilege: ``` resource: id: "job_list_1" # Unique identifier type: "API" | "DATA" | "MANAGEMENT" # Resource category path: "/api/jobs/{id}" # Exact path or regex service: "cleverswarm" # Which service owns it is_regex: false # Whether path is a regex pattern ``` This is where the path lives. Unless I'm reading out-of-date documentation (Stan shares this one in the group chat on May 1st), I didn't find any design document that matches your implementation in this PR. If you have the link to the document you're following with, please do share it. Thanks. ---- Also, a path can contain an underscore, since it's a valid URL. For example, a rest endpoint to manage digital books, I might give the path `/api/digital_books/`, although it looks weird and is generally not recommended (should use `digital-books`), the URL is legal and works with other systems. I couldn't see why we must add such limits to exclude underscores from URL path. Ref: https://datatracker.ietf.org/doc/html/rfc3986#section-2.3

abed.alrahman commented

2025-05-21 22:13:41 +00:00

Here is the design document for this ticket:
#2

We are implementing the ACL over Keycloak ACL capabilities as a first step.
Regarding the Stan blueprint document, we will think about it when it comes to a specific permission system.

Here is the design document for this ticket: https://git.cleverthis.com/clevermicro/user-management/issues/2 We are implementing the ACL over Keycloak ACL capabilities as a first step. Regarding the Stan blueprint document, we will think about it when it comes to a specific permission system.

hurui200320 commented

2025-05-22 10:19:32 +00:00

ok, since Stan gives his approval, I can accept this PR as the first step. Although I still think the current solution is not perfect, we will improve it in the future. After all, the whole permission system is still in the early stage.

hurui200320 marked this conversation as resolved

src/main/java/com/cleverthis/authservice/service/impl/KeycloakAdminClient.java

						
				@@ -0,0 +60,4 @@

				    }

				    private String getUserClientRolesEndpoint(String userId, String clientInternalId) {

				        return String.format("%s/admin/realms/%s/users/%s/role-mappings/clients/%s/composite",

hurui200320 commented

2025-05-20 06:02:26 +00:00

I would be generally concerned about security when I saw string concatenation for things like SQL or URL. I did check the code and right now it's safe. But fundamentally, I can pass something like someone?var=value# to hijack this webclient to send something we don't want to keycloak, maybe trying to fool it to do something else and give us a false result.

Ref: https://stackoverflow.com/questions/51544420/what-is-the-right-way-to-safely-concatenate-an-url-without-forgetting-a-separato/51545278#51545278

I would be generally concerned about security when I saw string concatenation for things like SQL or URL. I did check the code and _**right now**_ it's safe. But fundamentally, I can pass something like `someone?var=value#` to hijack this webclient to send something we don't want to keycloak, maybe trying to fool it to do something else and give us a false result. Ref: https://stackoverflow.com/questions/51544420/what-is-the-right-way-to-safely-concatenate-an-url-without-forgetting-a-separato/51545278#51545278

abed.alrahman commented

2025-05-21 00:38:47 +00:00

You are right. We should take care of SQL or HTML injection, but here, the string concatenation doesn't come from the request. I only know what the client name is, and the value comes from either the configuration file or from Keycloak config, nothing from the request.

hurui200320 marked this conversation as resolved

src/main/java/com/cleverthis/authservice/service/impl/KeycloakIdentityManagerService.java

						
				@@ -48,7 +47,7 @@ public class KeycloakIdentityManagerService implements IdentityManagerService {

				    private String getIntrospectionEndpoint() {

				        return String.format("%s/realms/%s/protocol/openid-connect/token/introspect",

hurui200320 commented

2025-05-20 06:04:28 +00:00

Same url concat issue.

abed.alrahman commented

2025-05-21 00:38:57 +00:00

You are right. We should take care of SQL or HTML injection, but here, the string concatenation doesn't come from the request. I only know what the client name is, and the value comes from either the configuration file or from Keycloak config, nothing from the request.

hurui200320 marked this conversation as resolved

abed.alrahman requested review from hurui200320 2025-05-21 00:39:30 +00:00

stanislav.hejny approved these changes 2025-05-21 10:24:46 +00:00

hurui200320 approved these changes 2025-05-22 10:19:59 +00:00

abed.alrahman commented

2025-05-26 23:40:14 +00:00