Prepare RepositoryMetadata role Updates for #1060 Compatibility #1061

minwoox · 2024-11-14T05:18:02Z

Motivation
To ensure smooth compatibility with #1060, the RepositoryMetadata structure needs an update to support changes.
AS-IS:

{
  "name": "minu-test",
  "perRolePermissions": {
    "owner": [
      "READ",
      "WRITE"
    ],
    "member": ["READ"],
    "guest": []
  },
  "perUserPermissions": {
    "foo@dogma.com": [
      "READ"
    ],
    "bar@dogma.com": [
      "READ",
      "WRITE"
    ]
  },
  "perTokenPermissions": {
    "goodman": [
      "READ"
    ]
  },
  "creation": {
    "user": "minu.song@dogma.com",
    "timestamp": "2024-08-19T02:47:23.370762417Z"
  }
}

TO-BE:

{
  "name": "minu-test",
  "roles": {
    "projects": {
      "member": "READ",
      "guest": null
    }
    "users": {
      "foo@dogma.com": "READ",
      "bar@dogma.com": "WRITE"
    },
    "tokens": {
      "goodman": "READ"
    }
  },
  "creation": {
    "user": "minu.song@dogma.com",
    "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 Introduce Role-Based Authentication for Repository Management #1060.

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.

jrhee17

👍 👍

jrhee17 · 2024-11-19T01:20:48Z

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

+import com.linecorp.centraldogma.internal.Jackson;
+import com.linecorp.centraldogma.server.QuotaConfig;
+
+final class RepositoryMetadataDeserializer extends StdDeserializer<RepositoryMetadata> {


I understood these custom deserializers will be removed once migration is complete

That is correct. 👍 👍

ikhoon

👍👍

jrhee17

Understood the new changes

jrhee17 · 2024-11-25T01:16:11Z

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

+     * Returns the {@link ProjectRoles} of the repository.
+     */
+    @JsonProperty("projects")
+    public ProjectRoles projectRoles() {


I understood this property as: the default RepositoryRole that will be assigned to project members that aren't explicitly set by users and tokens.

the default RepositoryRole that will be assigned to project members

That's correct. 👍

that aren't explicitly set by users and tokens.

That is not correct. A RepositoryRole can still be assigned to a user. The RepositoryRole, which operates at a higher level, will be applied in such situation. https://github.com/line/centraldogma/pull/1060/files#diff-dc8a031ec8b91f16d76b42e152e9c3be8dd07b6f77bea922030074acc78ce78eR800-R823

jrhee17 · 2024-11-25T01:20:58Z

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

+            throws IOException {
+        final JsonNode jsonNode = p.readValueAsTree();
+        final String name = jsonNode.get("name").textValue();
+        final JsonNode perRolePermissionsNode = jsonNode.get("perRolePermissions");


Note: I understood that if both perRolePermissions and roles is in the json node, then perRolePermissions (the legacy format) has higher priority.

I understood that if both perRolePermissions and roles is in the json node

They cannot be. Let me add an assertion logic. 😉

jrhee17 · 2024-11-25T01:27:36Z

common/src/main/java/com/linecorp/centraldogma/common/RepositoryRole.java

+     */
+    WRITE,
+    /**
+     * Able to manage a repository.


Compared with the current permission scheme, I understood the difference is:

the ability to set ADMIN for a repository

repository settings can't change project admin roles.

minwoox · 2024-12-05T09:34:36Z

This PR will be ready after #1069 is merged.

minwoox · 2024-12-11T03:06:37Z

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

-                       .thenApply(CommitResult::revision)
-                       .exceptionally(cause -> {
-                           final Throwable peeled = Exceptions.peel(cause);
-                           if (peeled instanceof RedundantChangeException) {


I've realized that we don't need this logic anymore after introducing the transformer in #1069 because it will raise an IllegalArgumentException if there's no change.

I'm not sure IllegalArgumentException is a proper exception to understand the context.

ikhoon · 2024-12-11T04:58:25Z

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

+            final RepositoryMetadata repositoryMetadata = projectMetadata.repo(repoName);
+            assert repositoryMetadata.name().equals(repoName);
+            if (repositoryMetadata.perRolePermissions().equals(perRolePermissions)) {
+                throw new IllegalArgumentException(


What do you think of raising RedundantChangeException instead?

That's a good suggestion! How about the following approach:

Introduce UserNotFoundException

This exception would be used exclusively on the server side, similar to TokenNotFoundException.

Raise UserNotFoundException and TokenNotFoundException

These exceptions would be triggered when attempting to update an entity that doesn't exist.

Raise RedundantChangeException

This exception would be raised when an update is attempted with the same content as the existing one.

The RedundantChangeException would be swallowed, and the system would simply return the head revision. In other words, users wouldn’t encounter this exception."

Updated, PTAL. 🙇

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

ikhoon · 2024-12-11T07:25:52Z

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

-                       .thenApply(CommitResult::revision)
-                       .exceptionally(cause -> {
-                           final Throwable peeled = Exceptions.peel(cause);
-                           if (peeled instanceof RedundantChangeException) {


I'm not sure IllegalArgumentException is a proper exception to understand the context.

ikhoon · 2024-12-11T07:32:37Z

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

        final String commitSummary =
                "Update the role permission of the '" + repoName + "' in the project '" + projectName + '\'';
-        return metadataRepo.push(projectName, Project.REPO_DOGMA, author, commitSummary, change);
+        final ContentTransformer<JsonNode> transformer = new ContentTransformer<>(
+                METADATA_JSON, EntryType.JSON, node -> {


Optional) I could see some code similar to this in other places. How about creating RepositoryMetadataTransformer and duplicating them?

private static class RepositoryMetadataTransformer extends ContentTransformer { RepositoryMetadataTransformer(Function<RepositoryMetadata, RepositoryMetadata>) { ... } }

That's a good suggestion. 👍
RepositoryMetadataTransformer, ProjectMetadataTransformer and TokensTransformer are added. 😉

ikhoon

Thanks! 👍

ikhoon · 2024-12-16T06:11:04Z

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

+        for (Entry<String, Token> entry : tokens.appIds().entrySet()) {
+            if (!entry.getKey().equals(appId)) {
+                appIdsBuilder.put(entry);
+            } else {
+                appIdsBuilder.put(appId, newToken);
+            }
+        }


Side note: It seems useful if we have an ImmutableMap utility that helps to update or remove.

class ImmutableMapUtil { Map<K, V> remove(Map<K, V> old, K key) { // create a new map and remove `key`. ... } Map<K, V> add(Map<K, V> old, K key, V value) { ... } }

That looks useful. 👍 Let me try that later. 😉

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

minwoox added the cleanup label Nov 14, 2024

minwoox added this to the 0.71.0 milestone Nov 14, 2024

minwoox added 4 commits November 14, 2024 14:22

Add test

3bc765b

Fix test

4f2e577

Fix

140428b

Add test

5eea3d7

jrhee17 approved these changes Nov 19, 2024

View reviewed changes

Revamp

b19c106

minwoox changed the title ~~Prepare PerPermission Updates for #1060 Compatibility~~ Prepare RepositoryMetadata role Updates for #1060 Compatibility Nov 19, 2024

minwoox changed the title ~~Prepare RepositoryMetadata role Updates for #1060 Compatibility~~ [WIP] Prepare RepositoryMetadata role Updates for #1060 Compatibility Nov 19, 2024

ikhoon approved these changes Nov 21, 2024

View reviewed changes

Apply more changes

a1f2967

minwoox changed the title ~~[WIP] Prepare RepositoryMetadata role Updates for #1060 Compatibility~~ Prepare RepositoryMetadata role Updates for #1060 Compatibility Nov 22, 2024

minwoox mentioned this pull request Nov 22, 2024

Introduce Role-Based Authentication for Repository Management #1060

Open

jrhee17 approved these changes Nov 25, 2024

View reviewed changes

minwoox added 2 commits December 3, 2024 14:40

Merge branch 'main' into prework_1060

2b59432

WIP

2ff378d

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

minwoox marked this pull request as draft December 5, 2024 01:26

minwoox mentioned this pull request Dec 6, 2024

Refactor MetadataService to Use ContentTransformer #1069

Merged

minwoox added 4 commits December 10, 2024 22:40

Merge branch 'main' into prework_1060

059a5f4

Merge branch 'main' into prework_1060

144d172

Update

b096bad

Update

c054553

minwoox commented Dec 11, 2024

View reviewed changes

Add more test

f8d8aca

minwoox marked this pull request as ready for review December 11, 2024 03:26

Fix test

1fe2f94

ikhoon reviewed Dec 11, 2024

View reviewed changes

minwoox added 3 commits December 13, 2024 12:26

Address the comments from @ikhoon

fe31752

Empty roles

27b8044

Add test

2b2ca34

ikhoon approved these changes Dec 16, 2024

View reviewed changes

minwoox merged commit 1cafcd1 into line:main Dec 16, 2024
9 of 10 checks passed

minwoox deleted the prework_1060 branch December 16, 2024 06:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prepare RepositoryMetadata role Updates for #1060 Compatibility #1061

Prepare RepositoryMetadata role Updates for #1060 Compatibility #1061

minwoox commented Nov 14, 2024 •

edited

Loading

jrhee17 left a comment •

edited

Loading

jrhee17 Nov 19, 2024

minwoox Nov 19, 2024

ikhoon left a comment

jrhee17 left a comment

jrhee17 Nov 25, 2024

minwoox Nov 27, 2024

jrhee17 Nov 25, 2024

minwoox Nov 27, 2024 •

edited

Loading

jrhee17 Nov 25, 2024

minwoox commented Dec 5, 2024

minwoox Dec 11, 2024

ikhoon Dec 11, 2024

ikhoon Dec 11, 2024

minwoox Dec 12, 2024

minwoox Dec 13, 2024

ikhoon Dec 11, 2024

ikhoon Dec 11, 2024

minwoox Dec 13, 2024

ikhoon left a comment

ikhoon Dec 16, 2024

minwoox Dec 16, 2024

Prepare RepositoryMetadata role Updates for #1060 Compatibility #1061

Prepare RepositoryMetadata role Updates for #1060 Compatibility #1061

Conversation

minwoox commented Nov 14, 2024 • edited Loading

jrhee17 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox commented Dec 5, 2024

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

ikhoon left a 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

jrhee17 left a comment •

edited

Loading

minwoox Nov 27, 2024 •

edited

Loading