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

[User management] Add CreateUserResponse and GetUserResponse gRPC response wrappers [DPP-854] #12682

Merged
merged 3 commits into from
Feb 8, 2022
Merged

[User management] Add CreateUserResponse and GetUserResponse gRPC response wrappers [DPP-854] #12682

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eb5afa0
[User management] Add CreateUserResponse and getUserResponse gRPC res…
pbatko-da Jan 31, 2022
3157413
Fix tests
pbatko-da Feb 3, 2022
775fe97
Skip broken tests, tracked them in https://github.com/digital-asset/d…
stefanobaghino-da Feb 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
[User management] Add CreateUserResponse and getUserResponse gRPC res… 
…ponse wrappers [DPP-854]

changelog_begin
Ledger API Specification: CreateUser and GetUser endpoints of UserManagementService now return CreateUserResponse and GetUserResponse rather than User message.
changelog_end

Breaks-protobuf: true
  • Loading branch information
@pbatko-da @stefanobaghino-da
pbatko-da authored and stefanobaghino-da committed Feb 8, 2022
commit eb5afa02c1ae31ebb89e6c920e26e620c6f842f3
4 changes: 2 additions & 2 deletions ...a/bindings-rxjava/src/main/java/com/daml/ledger/rxjava/grpc/UserManagementClientImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ private Single<User> createUser(
StubHelper.authenticating(this.serviceFutureStub, maybeToken)
.createUser(request.toProto()),
sequencerFactory)
.map(User::fromProto);
.map(res -> User.fromProto(res.getUser()));
Comment on lines 36 to +39
Copy link
Contributor

Choose a reason for hiding this comment

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

A wrapper type for the Protobuf should be added and that should be returned, rather than keeping the User type. Otherwise we lose the advantage of being able to evolve the API over time and this is especially important for the Java bindings, which are officially supported for external customers. I'm ok merging this as is and doing the work myself, just let me know what's your preference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would be great if you could handle it, thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I'll keep an eye on this and work on it once it's done.

}

@Override
Expand All @@ -55,7 +55,7 @@ private Single<User> getUser(
StubHelper.authenticating(this.serviceFutureStub, maybeToken)
.getUser(request.toProto()),
sequencerFactory)
.map(User::fromProto);
.map(res -> User.fromProto(res.getUser()));
}

@Override
Expand Down
8 changes: 4 additions & 4 deletions ...rxjava/src/test/scala/com/daml/ledger/rxjava/grpc/helpers/UserManagementServiceImpl.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,11 @@ final class UserManagementServiceImpl extends UserManagementService with FakeAut
Future.successful(result)
}

override def createUser(request: CreateUserRequest): Future[User] =
record(request)(User.defaultInstance)
override def createUser(request: CreateUserRequest): Future[CreateUserResponse] =
record(request)(CreateUserResponse.defaultInstance)

override def getUser(request: GetUserRequest): Future[User] =
record(request)(User.defaultInstance)
override def getUser(request: GetUserRequest): Future[GetUserResponse] =
record(request)(GetUserResponse.defaultInstance)

override def deleteUser(request: DeleteUserRequest): Future[DeleteUserResponse] =
record(request)(DeleteUserResponse.defaultInstance)
Expand Down
14 changes: 12 additions & 2 deletions ledger-api/grpc-definitions/com/daml/ledger/api/v1/admin/user_management_service.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,10 @@ option csharp_namespace = "Com.Daml.Ledger.Api.V1.Admin";
service UserManagementService {

// Create a new user, failing if it already exists.
rpc CreateUser (CreateUserRequest) returns (User);
rpc CreateUser (CreateUserRequest) returns (CreateUserResponse);

// Get the user data of a specific user or the authenticated user.
rpc GetUser (GetUserRequest) returns (User);
rpc GetUser (GetUserRequest) returns (GetUserResponse);

// Delete an existing user and all its rights.
rpc DeleteUser (DeleteUserRequest) returns (DeleteUserResponse);
Expand Down Expand Up @@ -104,13 +104,23 @@ message CreateUserRequest {
repeated Right rights = 2;
}

message CreateUserResponse {
// Created user.
User user = 1;
mziolekda marked this conversation as resolved.
Show resolved Hide resolved
}

// Required authorization: ``HasRight(ParticipantAdmin) OR IsAuthenticatedUser(user_id)``
message GetUserRequest {
// The user whose data to retrieve.
// If set to empty string (the default), then the data for the authenticated user will be retrieved.
string user_id = 1;
}

message GetUserResponse {
// Retrieved user.
User user = 1;
}

// Required authorization: ``HasRight(ParticipantAdmin)``
message DeleteUserRequest {
// The user to delete.
Expand Down
4 changes: 2 additions & 2 deletions .../scala/com/digitalasset/ledger/api/auth/services/UserManagementServiceAuthorization.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,10 @@ private[daml] final class UserManagementServiceAuthorization(
private implicit val errorLogger: ContextualizedErrorLogger =
new DamlContextualizedErrorLogger(logger, loggingContext, None)

override def createUser(request: CreateUserRequest): Future[User] =
override def createUser(request: CreateUserRequest): Future[CreateUserResponse] =
authorizer.requireAdminClaims(service.createUser)(request)

override def getUser(request: GetUserRequest): Future[User] =
override def getUser(request: GetUserRequest): Future[GetUserResponse] =
defaultToAuthenticatedUser(request.userId) match {
case Failure(ex) => Future.failed(ex)
case Success(Some(userId)) => service.getUser(request.copy(userId = userId))
Expand Down
6 changes: 3 additions & 3 deletions ...t/src/main/scala/com/digitalasset/ledger/client/services/admin/UserManagementClient.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,21 +30,21 @@ final class UserManagementClient(service: UserManagementServiceStub)(implicit
LedgerClient
.stub(service, token)
.createUser(request)
.map(fromProtoUser)
.map(res => fromProtoUser(res.user.get))
}

def getUser(userId: UserId, token: Option[String] = None): Future[User] =
LedgerClient
.stub(service, token)
.getUser(proto.GetUserRequest(userId.toString))
.map(fromProtoUser)
.map(res => fromProtoUser(res.user.get))

/** Retrieve the User information for the user authenticated by the token(s) on the call . */
def getAuthenticatedUser(token: Option[String] = None): Future[User] =
LedgerClient
.stub(service, token)
.getUser(proto.GetUserRequest())
.map(fromProtoUser)
.map(res => fromProtoUser(res.user.get))

def deleteUser(userId: UserId, token: Option[String] = None): Future[Unit] =
LedgerClient
Expand Down
18 changes: 10 additions & 8 deletions ...est-tool/src/main/scala/com/daml/ledger/api/testtool/suites/UserManagementServiceIT.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,11 @@ import com.daml.ledger.api.testtool.infrastructure.LedgerTestSuite
import com.daml.ledger.api.testtool.infrastructure.participant.ParticipantTestContext
import com.daml.ledger.api.v1.admin.user_management_service.{
CreateUserRequest,
CreateUserResponse,
DeleteUserRequest,
DeleteUserResponse,
GetUserRequest,
GetUserResponse,
GrantUserRightsRequest,
ListUserRightsRequest,
ListUserRightsResponse,
Expand Down Expand Up @@ -100,11 +102,11 @@ final class UserManagementServiceIT extends LedgerTestSuite {

} yield {
assertTooManyUserRightsError(create1)
assertEquals(create2, user1)
assertEquals(create2.user, Some(user1))
assertTooManyUserRightsError(grant1)
assertEquals(rights1.rights.size, permissionsMaxAndOne.tail.size)
assertSameElements(rights1.rights, permissionsMaxAndOne.tail)
assertEquals(create3, user2)
assertEquals(create3.user, Some(user2))
}
})

Expand Down Expand Up @@ -193,7 +195,7 @@ final class UserManagementServiceIT extends LedgerTestSuite {
get1 <- ledger.userManagement.getUser(GetUserRequest(AdminUserId))
rights1 <- ledger.userManagement.listUserRights(ListUserRightsRequest(AdminUserId))
} yield {
assertEquals(get1, User(AdminUserId, ""))
assertEquals(get1.user, Some(User(AdminUserId, "")))
assertEquals(rights1, ListUserRightsResponse(Seq(adminPermission)))
}
})
Expand All @@ -216,9 +218,9 @@ final class UserManagementServiceIT extends LedgerTestSuite {
res3 <- ledger.userManagement.createUser(CreateUserRequest(Some(user2), Nil))
res4 <- ledger.userManagement.deleteUser(DeleteUserRequest(userId2))
} yield {
assertEquals(res1, user1)
assertEquals(res1, CreateUserResponse(Some(user1)))
assertUserAlreadyExists(res2)
assertEquals(res3, user2)
assertEquals(res3, CreateUserResponse(Some(user2)))
Comment on lines -219 to +223
Copy link
Contributor

Choose a reason for hiding this comment

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

Could these changes require to add some exclusions to compatibility tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added Breaks-protobuf: true to the commit message as per ci/check-protobuf-stability.sh

assertEquals(res4, DeleteUserResponse())
}
})
Expand All @@ -240,7 +242,7 @@ final class UserManagementServiceIT extends LedgerTestSuite {
.mustFail("retrieving non-existent user")
} yield {
assertUserNotFound(res2)
assert(res1 == user1)
assert(res1 == GetUserResponse(Some(user1)))
}
})

Expand Down Expand Up @@ -289,7 +291,7 @@ final class UserManagementServiceIT extends LedgerTestSuite {
def filterUsers(users: Iterable[User]) = users.filter(u => u.id == userId1 || u.id == userId2)

assertSameElements(filterUsers(res1.users), Seq(user1))
assertEquals(res2, user2)
assertEquals(res2.user, Some(user2))
assertSameElements(
filterUsers(res3.users),
Set(user1, user2),
Expand Down Expand Up @@ -497,7 +499,7 @@ final class UserManagementServiceIT extends LedgerTestSuite {
res6 <- ledger.userManagement
.listUserRights(ListUserRightsRequest(userId1))
} yield {
assertEquals(res1, user1)
assertEquals(res1.user, Some(user1))
assertEquals(res2, ListUserRightsResponse(Seq.empty))
assertSameElements(
res3.newlyGrantedRights.toSet,
Expand Down
9 changes: 5 additions & 4 deletions ...ation-api/src/main/scala/platform/apiserver/services/admin/ApiUserManagementService.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import com.daml.error.{
ErrorCodesVersionSwitcher,
}
import com.daml.ledger.api.domain._
import com.daml.ledger.api.v1.admin.user_management_service.{CreateUserResponse, GetUserResponse}
import com.daml.ledger.api.v1.admin.{user_management_service => proto}
import com.daml.ledger.participant.state.index.v2.UserManagementStore
import com.daml.lf.data.Ref
Expand Down Expand Up @@ -52,7 +53,7 @@ private[apiserver] final class ApiUserManagementService(
override def bindService(): ServerServiceDefinition =
proto.UserManagementServiceGrpc.bindService(this, executionContext)

override def createUser(request: proto.CreateUserRequest): Future[proto.User] =
override def createUser(request: proto.CreateUserRequest): Future[CreateUserResponse] =
withValidation {
for {
pUser <- requirePresence(request.user, "user")
Expand All @@ -67,17 +68,17 @@ private[apiserver] final class ApiUserManagementService(
rights = pRights,
)
.flatMap(handleResult("creating user"))
.map(_ => request.user.get)
.map(_ => CreateUserResponse(Some(request.user.get)))
}

override def getUser(request: proto.GetUserRequest): Future[proto.User] =
override def getUser(request: proto.GetUserRequest): Future[GetUserResponse] =
withValidation(
requireUserId(request.userId, "user_id")
)(userId =>
userManagementStore
.getUser(userId)
.flatMap(handleResult("getting user"))
.map(toProtoUser)
.map(u => GetUserResponse(Some(toProtoUser(u))))
)

override def deleteUser(request: proto.DeleteUserRequest): Future[proto.DeleteUserResponse] =
Expand Down
4 changes: 3 additions & 1 deletion ...rc/test/it/scala/com/daml/ledger/platform/sandbox/auth/GetUserWithGivenUserIdAuthIT.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ class GetUserWithGivenUserIdAuthIT extends AdminServiceCallAuthTests {
val testId = UUID.randomUUID().toString

def getUser(userId: String): Future[User] =
stub(UserManagementServiceGrpc.stub(channel), token).getUser(GetUserRequest(userId))
stub(UserManagementServiceGrpc.stub(channel), token)
.getUser(GetUserRequest(userId))
.map(_.user.get)

for {
// create a normal users
Expand Down
2 changes: 1 addition & 1 deletion ...sandbox-on-x/src/test/lib/scala/com/daml/platform/sandbox/auth/ServiceCallAuthTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,6 @@ trait ServiceCallAuthTests
val req = proto.CreateUserRequest(Some(proto.User(userId)), rights)
stub(proto.UserManagementServiceGrpc.stub(channel), canReadAsAdminStandardJWT)
.createUser(req)
.map((_, userToken))
.map(res => (res.user.get, userToken))
}
}