Fix to enable changing the permission of internal repositories. #898

minwoox · 2023-11-27T02:57:12Z

Motivation:
Internal repositories are initially created with the internal permission, and I previously restricted the ability to modify this permission. However, users may have valid reasons to adjust the permission settings.

Modifications:

Implemented functionality to change the permission of internal repositories.
- Note: The guest role remains restricted from having any permission on internal repositories.

Result:

You can now have the capability to modify the permission settings for internal repositories, excluding the guest role.

Motivation: Internal repositories are initially created with the [internal](https://github.com/line/centraldogma/blob/0cf273402d70773e0aab1e2b858111a1c1c0f22c/server/src/main/java/com/linecorp/centraldogma/server/metadata/PerRolePermissions.java#L61) permission, and I previously restricted the ability to modify this permission. However, users may have valid reasons to adjust the permission settings. Modifications: - Implemented functionality to change the permission of internal repositories. - Note: The guest role remains restricted from having any permission on internal repositories. Result: - You can now have the capability to modify the permission settings for internal repositories, excluding the guest role.

codecov · 2023-11-27T03:05:08Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (ee7e250) 66.11% compared to head (ae147fa) 66.07%.

Files	Patch %	Lines
.../centraldogma/server/metadata/MetadataService.java	80.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #898      +/-   ##
============================================
- Coverage     66.11%   66.07%   -0.04%     
+ Complexity     3407     3404       -3     
============================================
  Files           363      363              
  Lines         14117    14121       +4     
  Branches       1509     1511       +2     
============================================
- Hits           9333     9330       -3     
- Misses         3921     3925       +4     
- Partials        863      866       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ikhoon · 2023-11-28T02:38:00Z

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

+            if (!guest.isEmpty()) {
+                throw new UnsupportedOperationException(
+                        "can't give a permission to guest for internal repository: " + repoName);


For consistency with the permission of internalPermissions, should we disallow giving permissions to MEMBER for internal repositories?

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

Lines 55 to 56 in 0cf2734

private static final PerRolePermissions internalPermissions =

new PerRolePermissions(READ_WRITE, NO_PERMISSION, NO_PERMISSION);

I believed we had previously discussed this matter and reached a consensus to delegate permission-granting authority to the owner for members. 😉
internalPermissions is assigned to internal repositories upon creation, and the owner has the discretion to change it at will. While I'm not strongly opposed, I see no compelling reason to restrict granting permissions to members.

Understood.
I was confused that the permission could be also granted to dogma repo.

Technically, I believe allowing MEMBER access to dogma can potentially allow MEMBERs to "kick" OWNERs out of projects.
I don't really know the distinction of owner and member though. If the meaning is simply to add an extra layer of authority, I guess the current change is fine.

Ah, got it. The dogma repo is restricted from being accessed by any user, so I initially thought this would suffice:

centraldogma/server/src/main/java/com/linecorp/centraldogma/server/internal/api/auth/RequiresPermissionDecorator.java

Lines 78 to 83 in 0cf2734

if (user.isAdmin()) {

return unwrap().serve(ctx, req);

}

if (Project.REPO_DOGMA.equals(repoName)) {

return throwForbiddenResponse(ctx, projectName, repoName, "administrator");

}

.

However, for clarity, I believe it's beneficial to explicitly allow changes only to the meta repository.

ikhoon

Thanks, @minwoox!

jrhee17

👍 👍 👍

jrhee17 · 2023-11-28T06:02:04Z

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

+            final Set<Permission> guest = perRolePermissions.guest();
+            if (!guest.isEmpty()) {
+                throw new UnsupportedOperationException(
+                        "can't give a permission to guest for internal repository: " + repoName);


Since I believe this exception will be directly exposed to the end user

Suggested change

"can't give a permission to guest for internal repository: " + repoName);

"Can't give a permission to guest for internal repository: " + repoName);

jrhee17 · 2023-11-28T06:12:27Z

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

+            if (!guest.isEmpty()) {
+                throw new UnsupportedOperationException(
+                        "can't give a permission to guest for internal repository: " + repoName);


Technically, I believe allowing MEMBER access to dogma can potentially allow MEMBERs to "kick" OWNERs out of projects.
I don't really know the distinction of owner and member though. If the meaning is simply to add an extra layer of authority, I guess the current change is fine.

minwoox · 2023-11-28T06:23:21Z

Technically, I believe allowing MEMBER access to dogma can potentially allow MEMBERs to "kick" OWNERs out of projects.

dogma can be accessed only by an admin not even by an owner:

centraldogma/server/src/main/java/com/linecorp/centraldogma/server/internal/api/auth/RequiresPermissionDecorator.java

Lines 78 to 83 in 0cf2734

    
           if (user.isAdmin()) { 
        
               return unwrap().serve(ctx, req); 
        
           } 
        
           if (Project.REPO_DOGMA.equals(repoName)) { 
        
               return throwForbiddenResponse(ctx, projectName, repoName, "administrator"); 
        
           }

.

minwoox · 2023-11-29T02:09:17Z

Thanks for reviewing. 😉

Motivation: Despite having the necessary permissions after line#898, members were unable to access the meta repository. Modifications: - Implemented a fix to ensure that members can access the meta repository as intended. Result: - Members with appropriate permissions can now successfully access the meta repository.

Motivation: Despite having the necessary permissions after #898, members were unable to access the meta repository. Modifications: - Implemented a fix to ensure that members can access the meta repository as intended. Result: - Members with appropriate permissions can now successfully access the meta repository.

minwoox added the defect label Nov 27, 2023

minwoox added this to the 0.63.3 milestone Nov 27, 2023

minwoox requested review from ikhoon, jrhee17 and trustin as code owners November 27, 2023 02:57

ikhoon reviewed Nov 28, 2023

View reviewed changes

ikhoon approved these changes Nov 28, 2023

View reviewed changes

jrhee17 approved these changes Nov 28, 2023

View reviewed changes

Address comments from @ikhoon and @jrhee17

ae147fa

ikhoon mentioned this pull request Nov 28, 2023

Introduce new mirroring and credential settings format and REST API #880

Merged

minwoox merged commit 0afa010 into line:main Nov 29, 2023

minwoox deleted the internal_repo_permission branch November 29, 2023 02:09

minwoox mentioned this pull request Nov 30, 2023

Fix to access for a member to the meta repository. #901

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix to enable changing the permission of internal repositories. #898

Fix to enable changing the permission of internal repositories. #898

minwoox commented Nov 27, 2023

codecov bot commented Nov 27, 2023 •

edited

Loading

ikhoon Nov 28, 2023

minwoox Nov 28, 2023

ikhoon Nov 28, 2023

jrhee17 Nov 28, 2023

minwoox Nov 28, 2023

ikhoon left a comment

jrhee17 left a comment

jrhee17 Nov 28, 2023

jrhee17 Nov 28, 2023

minwoox commented Nov 28, 2023

minwoox commented Nov 29, 2023

	private static final PerRolePermissions internalPermissions =
	new PerRolePermissions(READ_WRITE, NO_PERMISSION, NO_PERMISSION);

	if (user.isAdmin()) {
	return unwrap().serve(ctx, req);
	}
	if (Project.REPO_DOGMA.equals(repoName)) {
	return throwForbiddenResponse(ctx, projectName, repoName, "administrator");
	}

	"can't give a permission to guest for internal repository: " + repoName);
	"Can't give a permission to guest for internal repository: " + repoName);

Fix to enable changing the permission of internal repositories. #898

Fix to enable changing the permission of internal repositories. #898

Conversation

minwoox commented Nov 27, 2023

codecov bot commented Nov 27, 2023 • edited Loading

Codecov Report

ikhoon Nov 28, 2023

Choose a reason for hiding this comment

minwoox Nov 28, 2023

Choose a reason for hiding this comment

ikhoon Nov 28, 2023

Choose a reason for hiding this comment

jrhee17 Nov 28, 2023

Choose a reason for hiding this comment

minwoox Nov 28, 2023

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

jrhee17 Nov 28, 2023

Choose a reason for hiding this comment

jrhee17 Nov 28, 2023

Choose a reason for hiding this comment

minwoox commented Nov 28, 2023

minwoox commented Nov 29, 2023

codecov bot commented Nov 27, 2023 •

edited

Loading