Skip to content
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

Add HTTP API for updating a token level #942

Merged
merged 7 commits into from
Aug 13, 2024
Merged

Add HTTP API for updating a token level #942

merged 7 commits into from
Aug 13, 2024

Conversation

byungjunn
Copy link
Contributor

@byungjunn byungjunn commented Apr 10, 2024 •
edited by minwoox
Loading

Motivation:

Modification:

  • Change admin from true to false or vice versa.

Result:

  • You can now change the level of a token via HTTP API.
image image

ikhoon
ikhoon reviewed Apr 18, 2024
View reviewed changes
server/src/main/java/com/linecorp/centraldogma/server/internal/api/TokenService.java Outdated Show resolved Hide resolved
server/src/main/java/com/linecorp/centraldogma/server/internal/api/TokenService.java Outdated Show resolved Hide resolved
server/src/main/java/com/linecorp/centraldogma/server/internal/api/TokenService.java Outdated
token -> {
if (token.isAdmin()) {
return mds.updateTokenToUser(author, appId).thenApply(
unused -> mds.findTokenByAppId(appId).join().withoutSecret()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@byungjunn byungjunn Apr 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove the join() .

@byungjunn byungjunn requested a review from ikhoon April 25, 2024 08:38
@minwoox
Copy link
Member

minwoox commented May 10, 2024

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

@byungjunn byungjunn requested a review from ikhoon May 19, 2024 08:56
minwoox
minwoox reviewed May 21, 2024
View reviewed changes
Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great so far. 😉

server/src/test/java/com/linecorp/centraldogma/server/internal/api/TokenServiceTest.java Outdated Show resolved Hide resolved
server/src/test/java/com/linecorp/centraldogma/server/internal/api/TokenServiceTest.java Outdated Show resolved Hide resolved
server/src/main/java/com/linecorp/centraldogma/server/internal/api/TokenLevelRequest.java Outdated Show resolved Hide resolved
server/src/main/java/com/linecorp/centraldogma/server/internal/api/TokenService.java Outdated
@@ -192,6 +193,41 @@ public CompletableFuture<Token> updateToken(ServiceRequestContext ctx,
);
}

/**
* PATCH /tokens/{appId}/permission
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use permission or level?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

server/src/main/java/com/linecorp/centraldogma/server/internal/api/TokenService.java Outdated
return mds.updateTokenToAdmin(author, appId).thenCompose(
unused -> mds.findTokenByAppId(appId).thenApply(Token::withoutSecret));
default:
throw new IllegalArgumentException("Unexpected token level: " + tokenLevelRequest);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can do this before calling getTokenOrRespondForbidden

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before calling getTokenOrRespondForbidden, i will check the argument.

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

server/src/main/java/com/linecorp/centraldogma/server/internal/api/TokenService.java Outdated Show resolved Hide resolved
server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java Outdated Show resolved Hide resolved
@byungjunn byungjunn requested a review from minwoox May 27, 2024 05:05
minwoox
minwoox approved these changes Jun 17, 2024
View reviewed changes
Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of nits. Great job, @byungjunn! 👍

server/src/main/java/com/linecorp/centraldogma/server/internal/api/TokenLevelRequest.java Outdated Show resolved Hide resolved
server/src/main/java/com/linecorp/centraldogma/server/internal/api/TokenService.java Outdated Show resolved Hide resolved
server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java Outdated Show resolved Hide resolved
@minwoox minwoox added this to the 0.67.0 milestone Jun 17, 2024
@byungjunn byungjunn requested a review from minwoox June 25, 2024 01:32
@minwoox minwoox modified the milestones: 0.67.0, 0.67.1 Jul 10, 2024
@minwoox minwoox modified the milestones: 0.67.1, 0.67.2 Jul 23, 2024
@minwoox minwoox modified the milestones: 0.67.2, 0.69.0 Jul 23, 2024
ikhoon
ikhoon approved these changes Jul 23, 2024
View reviewed changes
Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! 💯💯

jrhee17
jrhee17 approved these changes Jul 30, 2024
View reviewed changes
Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good for merging 👍 👍 👍

server/src/main/java/com/linecorp/centraldogma/server/internal/api/TokenService.java
*/
@Patch("/tokens/{appId}/level")
@RequiresAdministrator
public CompletableFuture<Token> updateTokenLevel(ServiceRequestContext ctx,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit; I'm fine with adding an API, but in retrospect I wonder if we could've just reused updateToken

minwoox
minwoox approved these changes Aug 13, 2024
View reviewed changes
Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️ ❤️ ❤️

@minwoox minwoox changed the title Update token level Add HTTP API for updating a token level Aug 13, 2024
@minwoox minwoox merged commit b06bc11 into line:main Aug 13, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ikhoon ikhoon ikhoon approved these changes

@jrhee17 jrhee17 jrhee17 approved these changes

@minwoox minwoox minwoox approved these changes

@trustin trustin Awaiting requested review from trustin trustin is a code owner

Assignees
No one assigned
Labels
new feature
Projects
None yet
Milestone
0.69.0
Development

Successfully merging this pull request may close these issues.

Provide a way to update the permission of a token
4 participants
@byungjunn @minwoox @ikhoon @jrhee17