Introduce Role-Based Authentication for Repository Management #1060

minwoox · 2024-11-14T04:36:05Z

Motivation:
To address the migration of mirroring configurations from projects to repositories and to simplify access management,
we decided to:

Hide the meta repository.
Replace permission-based authentication with a role-based system using RepositoryRole (READ, WRITE, ADMIN). The ADMIN role has access to the configurations.

This change resolves the issue where users with WRITE permission for the meta repository were only allowed to creating mirroring configurations.

With RepositoryRole, access becomes more structured and extensible for future enhancements (e.g., introducing custom roles). While permission-based authentication is replaced here, this does not preclude the future coexistence of role-based and permission-based systems.

Modifications:

Added RepositoryRole with hierarchical roles (READ, WRITE, ADMIN).
Replaced RequiresPermission annotations with RequiresRepositoryRole.
Renamed RequiresRole to RequiresProjectRole.
- Updated it to accept a single ProjectRole, utilizing the hierarchical model to avoid redundant role specifications.
Removed APIs for managing permissions, replacing them with role management APIs.

Result:

Role-based access simplifies management and aligns with repository-specific mirroring configurations.
(Breaking) APIs for managing permissions are removed.

To-do:

Update the documentation to reflect the new systme, including updated screenshots

Migration plan:

Deploy PR, which supports deserialization of both legacy and new metadata format.
Deploy intermediate commit supporting both permission and role APIs, ensuring metadata is stored in the new format.
- The commit also migrate the legacy format to new format.
Deploy this commit, which exclusively supports role-based APIs.

Motivation To ensure smooth compatibility with line#1060, the `PerRolePermissions` and `RepositoryMetadata` structure need an update to support changes. Modifications: - Enhanced the `PerRolePermissions` and `RepositoryMetadata` deserializers to accept both an array or permissions and a permission. - Added `REPO_ADMIN` `Permission`. - Removed `MetadataApiService.updateSpecificUserPermission` and `updateSpecificTokenPermission`. - They are not used in the UI and we don't even have test cases for that. - Will add those APIs later when we need it. Result: - The `PerRolePermissions` and `RepositoryMetadata` structures now support the upcoming changes in line#1060.

minwoox · 2024-11-22T03:20:42Z

server/src/main/java/com/linecorp/centraldogma/server/internal/api/ProjectServiceV1.java

-    @RequiresRole(roles = { ProjectRole.OWNER, ProjectRole.MEMBER })
-    public CompletableFuture<ProjectMetadata> getProjectMetadata(
-            @Param String projectName,
-            @Param("checkPermissionOnly") @Default("false") boolean isCheckPermissionOnly) {


This parameter was introduced here but has not been used anywhere. Let me just remove that.

ikhoon · 2024-12-02T02:23:57Z

server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java

+        //     }
+        //   }
+        // }
+        final JsonPointer path = JsonPointer.compile("/repos" + encodeSegment(repoName) + "/roles/projects");


I understand that this code will be applied after the migration is complete.

server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java

ikhoon · 2024-12-02T05:10:37Z

server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java

+        //     }
+        //   }
+        // }
+        final JsonPointer path = JsonPointer.compile("/repos" + encodeSegment(repoName) + "/roles/projects");


Note: It would be nice to have a DSL/builder to build a JSON pointer type-safely. Let's consider it later.

JsonPointers .meta() .repos(repoName) .roles() .projects() .compile(); JsonPointers .meta() .members(repoName) .compile();

That's a good suggestion. By the way, this PR is in WIP and I forgot to tell you about it.
Would you mind reviewing #1064, beforehand?

ikhoon · 2024-12-02T06:22:03Z

server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java

-                return repositoryMetadata.perRolePermissions().guest();
+    @Nullable
+    private static RepositoryRole repositoryRole(Roles roles, @Nullable RepositoryRole repositoryRole,
+                                                 @Nullable ProjectRole projectRole) {


projectRole seems not null always in the current usage.

Suggested change

@Nullable ProjectRole projectRole) {

ProjectRole projectRole) {

ikhoon · 2024-12-02T06:41:20Z

server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java

+        if (memberOrGuestRole == null) {
+            return repositoryRole;
+        }
+        if (memberOrGuestRole == RepositoryRole.WRITE || repositoryRole == RepositoryRole.WRITE) {


Let's say a-member-user is granted to READ role and projects.member is granted to WRITE. Is WRITE given for a-member-user?

{ "repos": { "my-repo": { "name": "my-repo", "roles": { "users": { "a-member-user": "READ" } "projects": { "member": "WRITE", ... } } } } }

Is WRITE given for a-member-user?

That is correct, as similar to how GitHub handles it.
The role with the highest authority will take precedence.

That's a little surprising - I was expecting the repository role to override the project role.
If that's how Github is doing it, then I guess the current approach is fine.

server/src/main/java/com/linecorp/centraldogma/server/metadata/Roles.java

Motivation To ensure smooth compatibility with #1060, the `RepositoryMetadata` structure needs an update to support changes. AS-IS: ```json { "name": "minu-test", "perRolePermissions": { "owner": [ "READ", "WRITE" ], "member": ["READ"], "guest": [] }, "perUserPermissions": { "[email protected]": [ "READ" ], "[email protected]": [ "READ", "WRITE" ] }, "perTokenPermissions": { "goodman": [ "READ" ] }, "creation": { "user": "[email protected]", "timestamp": "2024-08-19T02:47:23.370762417Z" } } ``` TO-BE: ```json { "name": "minu-test", "roles": { "projects": { "member": "READ", "guest": null } "users": { "[email protected]": "READ", "[email protected]": "WRITE" }, "tokens": { "goodman": "READ" } }, "creation": { "user": "[email protected]", "timestamp": "2024-08-19T02:47:23.370762417Z" } } ``` Modifications: - Enhanced the `RepositoryMetadata` deserializers to accept both legacy and new formats. - Added `RepositoryRole`. - It will be used for repository instead of `Permission`. - Removed `MetadataApiService.updateSpecificUserPermission` and `updateSpecificTokenPermission`. - They are not used in the UI and we don't even have test cases for that. - Will add those APIs later with better API design when we need it. Result: - The `RepositoryMetadata` structure now supports the upcoming changes in #1060.

minwoox · 2024-12-17T05:09:10Z

This PR is ready. PTAL. 🙇

jrhee17

Apart from MetadataService#repositoryRole, I understood that the rest of changes are simple renaming/refactoring.

jrhee17 · 2024-12-20T02:49:22Z

server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java

-                return repositoryMetadata.perRolePermissions().member();
-            default:
-                return repositoryMetadata.perRolePermissions().guest();
+    @Nullable


Optional) What do you think of adding a RepositoryRole.NONE? It doesn't seem intuitive that a method returning an enum is nullable.

Suggested change

@Nullable

jrhee17 · 2024-12-20T02:50:19Z

server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java

+        if (memberOrGuestRole == null) {
+            return repositoryRole;
+        }
+        if (memberOrGuestRole == RepositoryRole.WRITE || repositoryRole == RepositoryRole.WRITE) {


That's a little surprising - I was expecting the repository role to override the project role.
If that's how Github is doing it, then I guess the current approach is fine.

jrhee17 · 2024-12-20T02:55:22Z

server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java

-            default:
-                return repositoryMetadata.perRolePermissions().guest();
+    @Nullable
+    private static RepositoryRole repositoryRole(Roles roles, @Nullable RepositoryRole repositoryRole,


optional) the logic in this class was not easy to follow at first glance. To me, the following flow makes more sense:

@Nullable private static RepositoryRole repositoryRole(Roles roles, @Nullable RepositoryRole repositoryRole, ProjectRole projectRole) { final RepositoryRole projectRepositoryRole; if (projectRole == ProjectRole.MEMBER) { projectRepositoryRole = roles.projectRoles().member(); } else { projectRepositoryRole = roles.projectRoles().guest(); } if (repositoryRole == RepositoryRole.ADMIN || projectRepositoryRole == RepositoryRole.ADMIN) { return RepositoryRole.ADMIN; } if (repositoryRole == RepositoryRole.WRITE || projectRepositoryRole == RepositoryRole.WRITE) { return RepositoryRole.ADMIN; } if (repositoryRole == RepositoryRole.READ || projectRepositoryRole == RepositoryRole.READ) { return RepositoryRole.READ; } return null; }

[WIP] Use the highest authority permission

559ce4d

minwoox added the breaking change label Nov 14, 2024

minwoox added this to the 0.72.0 milestone Nov 14, 2024

minwoox requested review from ikhoon, jrhee17 and trustin as code owners November 14, 2024 04:36

minwoox mentioned this pull request Nov 14, 2024

Prepare RepositoryMetadata role Updates for #1060 Compatibility #1061

Merged

minwoox added 3 commits November 14, 2024 14:30

lint

0ad7b35

WIP

e8249bc

Fix UI

1ffab05

minwoox changed the title ~~[WIP] Use the highest authority permission~~ Simplify Permission Configuration by Using Highest Authority Nov 18, 2024

minwoox added deprecation and removed breaking change labels Nov 18, 2024

minwoox added 2 commits November 18, 2024 11:38

Add test

35459a1

WIP

c06e107

minwoox changed the title ~~Simplify Permission Configuration by Using Highest Authority~~ [WIP] Simplify Permission Configuration by Using Highest Authority Nov 21, 2024

minwoox added 3 commits November 21, 2024 16:06

Add missing tests

8df1aa1

Fix UI

f638fd4

Fix comment

85cca89

minwoox commented Nov 22, 2024

View reviewed changes

minwoox added breaking change new feature labels Nov 22, 2024

minwoox changed the title ~~[WIP] Simplify Permission Configuration by Using Highest Authority~~ Introduce Role-Based Authentication for Repository Management Nov 22, 2024

ikhoon reviewed Dec 2, 2024

View reviewed changes

minwoox changed the title ~~Introduce Role-Based Authentication for Repository Management~~ [WIP] Introduce Role-Based Authentication for Repository Management Dec 2, 2024

minwoox modified the milestones: 0.72.0, 0.73.0 Dec 4, 2024

Merge branch 'main' into add_repo_admin

ef403a9

Address comments from @ikhoon

75da118

minwoox changed the title ~~[WIP] Introduce Role-Based Authentication for Repository Management~~ Introduce Role-Based Authentication for Repository Management Dec 17, 2024

jrhee17 approved these changes Dec 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Role-Based Authentication for Repository Management #1060

Introduce Role-Based Authentication for Repository Management #1060

minwoox commented Nov 14, 2024 •

edited

Loading

minwoox Nov 22, 2024

ikhoon Dec 2, 2024

ikhoon Dec 2, 2024

minwoox Dec 2, 2024

ikhoon Dec 2, 2024

ikhoon Dec 2, 2024

minwoox Dec 2, 2024

jrhee17 Dec 20, 2024

minwoox commented Dec 17, 2024

jrhee17 left a comment

jrhee17 Dec 20, 2024

jrhee17 Dec 20, 2024

jrhee17 Dec 20, 2024

	@Nullable ProjectRole projectRole) {
	ProjectRole projectRole) {

Introduce Role-Based Authentication for Repository Management #1060

Are you sure you want to change the base?

Introduce Role-Based Authentication for Repository Management #1060

Conversation

minwoox commented Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox commented Dec 17, 2024

jrhee17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox commented Nov 14, 2024 •

edited

Loading