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.

Sign up for GitHub

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

Jump to bottom

[WIP] Introduce Role-Based Authentication for Repository Management #1060

Open

minwoox wants to merge 9 commits into line:main from minwoox:add_repo_admin

Member

minwoox commented Nov 14, 2024 •

edited

Loading

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


          [WIP] Use the highest authority permission

559ce4d

minwoox added the breaking change label

minwoox added this to the 0.72.0 milestone

minwoox requested review from ikhoon, jrhee17 and trustin as code owners

November 14, 2024 04:36

minwoox added a commit to minwoox/centraldogma that referenced this pull request


          Prepare PerPermission Updates for line#1060 Compatibility

09a6593

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 mentioned this pull request

Prepare RepositoryMetadata role Updates for #1060 Compatibility #1061

Open

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

minwoox added deprecation and removed breaking change labels

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

minwoox added 3 commits

November 21, 2024 16:06


          Add missing tests

8df1aa1


          Fix UI

f638fd4


          Fix comment

85cca89

minwoox commented

View reviewed changes

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) {

Member Author

minwoox Nov 22, 2024

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

minwoox added breaking change new feature labels

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

ikhoon reviewed

View reviewed changes

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

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

Contributor

ikhoon Dec 2, 2024

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

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

Comment on lines +1125 to +1128

+                      //         }
+                      //         "tokens": {
+                      //           "my-token": "WRITE"
+                      //         }

Contributor

ikhoon Dec 2, 2024

Suggested change

      
                    //         }
          
                    //         "tokens": {
          
                    //           "my-token": "WRITE"
          
                    //         }
          
                    //         },
          
                    //         "tokens": {
          
                    //           "my-token": "WRITE"
          
                    //         },

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

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

Contributor

ikhoon Dec 2, 2024

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();

Member Author

minwoox Dec 2, 2024

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?

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) {

Contributor

ikhoon Dec 2, 2024

projectRole seems not null always in the current usage.

Suggested change

      
                                                             @Nullable ProjectRole projectRole) {
          
                                                             ProjectRole projectRole) {

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

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

Contributor

ikhoon Dec 2, 2024

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",
          ...
       }
      }
    }
  }
}

Member Author

minwoox Dec 2, 2024

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.

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

+                  @Override
+                  public String toString() {
+                      return MoreObjects.toStringHelper(this).omitNullValues()

Contributor

ikhoon Dec 2, 2024

nit:

Suggested change

      
                    return MoreObjects.toStringHelper(this).omitNullValues()
          
                    return MoreObjects.toStringHelper(this)

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

minwoox modified the milestones: 0.72.0, 0.73.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change deprecation new feature