Update token level #942

byungjunn · 2024-04-10T13:16:54Z

motivation:

Resolve Provide a way to update the permission of a token #906
A lot of effort is used to change permissions.

modification:

Change admin from true to false or vice versa.

result:

ikhoon · 2024-04-18T06:10:22Z

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

+     * <p>Updates a permission of a token of the specified ID.
+     */
+    @Patch("/tokens/{appId}/level")
+    @Consumes("application/json-patch+json")


json-patch+json seems not used for this API

Okay, I'll remove the annotation.

ikhoon · 2024-04-18T06:21:41Z

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

+                                                     Author author, User loginUser) {
+        return getTokenOrRespondForbidden(ctx, appId, loginUser).thenCompose(
+                token -> {
+                    if (token.isAdmin()) {


Should we make this API idempotent instead of changing the behavior depending on the token's status?
Some people may want to retry an update request if the previous request is timed out somehow.

How about taking the desired level in the body?

PATCH /tokens/{appId}/permission { "level": "USER" }

@ikhoon
Oh, i got it.
In this project, in which package should the request body class be placed?

public class TokenLevel { private String level; public TokenLevel() { } public String getLevel() { return level; } public void setLevel(String level) { this.level = level; } }

We may place this class in the same package of TokenService.

Could remove public

Rename TokenLevel to TokenLevelRequest for clarity.

Okay, Thanks for your comment. I'll add it.

ikhoon · 2024-04-18T06:22:41Z

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

+                token -> {
+                    if (token.isAdmin()) {
+                        return mds.updateTokenToUser(author, appId).thenApply(
+                                unused -> mds.findTokenByAppId(appId).join().withoutSecret()


join() should not be called for asynchronous code. thenCompose() could be used instead.

I'll remove the join() .

minwoox · 2024-05-10T06:12:58Z

Could you add an e2e test to verify that the request is converted correctly?

ikhoon · 2024-05-10T10:14:58Z

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

+                                                     Author author, User loginUser) {
+
+        return getTokenOrRespondForbidden(ctx, appId, loginUser).thenCompose(
+                token -> {


Should we return 304 Not Modified if the token is at the desired level already?

Okay, I'll add

throw HttpStatusException.of(HttpStatus.NOT_MODIFIED);

when the current status and desired one is same.

minwoox

Looks great so far. 😉

minwoox · 2024-05-21T05:06:19Z

server/src/test/java/com/linecorp/centraldogma/server/internal/api/TokenServiceTest.java

@@ -203,4 +244,52 @@ public void updateToken() {
                .isInstanceOf(CompletionException.class)
                .hasCauseInstanceOf(IllegalArgumentException.class);
    }
+
+    @Test
+    public void updateTokenLevel() {


nit:

Suggested change

public void updateTokenLevel() {

void updateTokenLevel() {

minwoox · 2024-05-21T05:08:47Z

server/src/test/java/com/linecorp/centraldogma/server/internal/api/TokenServiceTest.java

+    }
+
+    @Test
+    public void createTokenAndUpdateLevel() throws JsonParseException {


nit:

Suggested change

public void createTokenAndUpdateLevel() throws JsonParseException {

void createTokenAndUpdateLevel() throws JsonParseException {

minwoox · 2024-05-21T05:17:55Z

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

+class TokenLevelRequest {
+    private String level;
+
+    TokenLevelRequest() {
+    }
+
+    TokenLevelRequest(String level) {
+        this.level = level;
+    }
+
+    String getLevel() {
+        return level;
+    }
+
+    void setLevel(String level) {
+        this.level = level;
+    }
+}


Let's use Jackson annotations to make it clear. 😉

Suggested change

class TokenLevelRequest {

private String level;

TokenLevelRequest() {

}

TokenLevelRequest(String level) {

this.level = level;

}

String getLevel() {

return level;

}

void setLevel(String level) {

this.level = level;

}

}

class TokenLevelRequest {

private final String level;

@JsonCreator

TokenLevelRequest(@JsonProperty("level") String level) {

this.level = level;

}

@JsonProperty

String level() {

return level;

}

}

minwoox · 2024-05-21T05:36:07Z

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

@@ -192,6 +193,41 @@ public CompletableFuture<Token> updateToken(ServiceRequestContext ctx,
        );
    }

+    /**
+     * PATCH /tokens/{appId}/permission


Should we use permission or level?

I will change "permission" to "level" in the comment.

minwoox · 2024-05-21T05:37:31Z

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

+                            return mds.updateTokenToAdmin(author, appId).thenCompose(
+                                    unused -> mds.findTokenByAppId(appId).thenApply(Token::withoutSecret));
+                        default:
+                            throw new IllegalArgumentException("Unexpected token level: " + tokenLevelRequest);


I think we can do this before calling getTokenOrRespondForbidden

Before calling getTokenOrRespondForbidden, i will check the argument.

checkArgument(Arrays.asList("user", "admin").contains(tokenLevelRequest.level().toLowerCase()), "Unsupported token level: " + tokenLevelRequest.level());

minwoox · 2024-05-21T05:40:28Z

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

+
+        return getTokenOrRespondForbidden(ctx, appId, loginUser).thenCompose(
+                token -> {
+                    switch (tokenLevelRequest.getLevel()) {


What do you think about supporting case-insensitive requests?

Suggested change

switch (tokenLevelRequest.getLevel()) {

switch (tokenLevelRequest.getLevel().toLowerCase()) {

minwoox · 2024-05-21T05:42:19Z

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

+    /**
+     * Update the {@link Token} of the specified {@code appId} to admin.
+     */
+    public CompletableFuture<Revision> updateTokenToAdmin(Author author, String appId) {


I think we can merge these two methods with a boolean parameter. Could you merge them?

Okay, it could be more simple.

byungjunn requested review from ikhoon, jrhee17, minwoox and trustin as code owners April 10, 2024 13:16

Update token level

8af41e5

byungjunn force-pushed the update-token-level branch from 61d8995 to 8af41e5 Compare April 10, 2024 13:32

lint

a1ea206

ikhoon reviewed Apr 18, 2024

View reviewed changes

byungjunn force-pushed the update-token-level branch from 8801c98 to 539202f Compare April 25, 2024 07:47

Add tokenRequest body

e7ee043

byungjunn force-pushed the update-token-level branch from 539202f to e7ee043 Compare April 25, 2024 08:07

byungjunn requested a review from ikhoon April 25, 2024 08:38

ikhoon added the new feature label May 10, 2024

ikhoon reviewed May 10, 2024

View reviewed changes

byungjunn added 2 commits May 19, 2024 17:43

Add HttpStatusException

3d1b621

Add api test

cd82fd8

byungjunn requested a review from ikhoon May 19, 2024 08:56

minwoox reviewed May 21, 2024

View reviewed changes

Merge the updateTokenLevel methods

1e9ef81

byungjunn force-pushed the update-token-level branch from 1985215 to 1e9ef81 Compare May 24, 2024 07:13

byungjunn requested a review from minwoox May 27, 2024 05:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update token level #942

Update token level #942

byungjunn commented Apr 10, 2024

ikhoon Apr 18, 2024

byungjunn Apr 18, 2024 •

edited

ikhoon Apr 18, 2024

byungjunn Apr 18, 2024

ikhoon Apr 22, 2024

byungjunn Apr 25, 2024

ikhoon Apr 18, 2024

byungjunn Apr 18, 2024 •

edited

minwoox commented May 10, 2024

ikhoon May 10, 2024

byungjunn May 19, 2024

minwoox left a comment

minwoox May 21, 2024

minwoox May 21, 2024

minwoox May 21, 2024

minwoox May 21, 2024

byungjunn May 22, 2024

minwoox May 21, 2024

byungjunn May 24, 2024

minwoox May 21, 2024

minwoox May 21, 2024

byungjunn May 24, 2024

	public void createTokenAndUpdateLevel() throws JsonParseException {
	void createTokenAndUpdateLevel() throws JsonParseException {

	switch (tokenLevelRequest.getLevel()) {
	switch (tokenLevelRequest.getLevel().toLowerCase()) {

Update token level #942

Are you sure you want to change the base?

Update token level #942

Conversation

byungjunn commented Apr 10, 2024

motivation:

modification:

result:

Choose a reason for hiding this comment

byungjunn Apr 18, 2024 • edited

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

byungjunn Apr 18, 2024 • edited

Choose a reason for hiding this comment

minwoox commented May 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox 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

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

byungjunn Apr 18, 2024 •

edited

byungjunn Apr 18, 2024 •

edited