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.

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

[risk=low][no ticket] Update User and UserRole entities to favor userName over email #8989

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ public void fireEgressEvent(SumologicEgressEvent event) {
// 3. Dataproc worker nodes: all-of-us-<user_id>-w-<index>
var vmOwner =
userRoles.stream()
.map(UserRole::getEmail)
.map(UserRole::getUserName)
.map(userDao::findUserByUsername)
.filter(user -> user.getRuntimeName().equals(event.getVmPrefix()))
.findFirst()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ public class UserController implements UserApiDelegate {
private static final Function<DbUser, User> TO_USER_RESPONSE_USER =
user -> {
User modelUser = new User();
modelUser.setEmail(user.getUsername()); // deprecated, but kept for compatibility
modelUser.setUserName(user.getUsername());
modelUser.setGivenName(user.getGivenName());
modelUser.setFamilyName(user.getFamilyName());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -712,13 +712,13 @@ public ResponseEntity<WorkspaceUserRolesResponse> shareWorkspacePatch(
if (role.getRole() == null || role.getRole().toString().trim().isEmpty()) {
throw new BadRequestException("Role required.");
}
final DbUser invitedUser = userDao.findUserByUsername(role.getEmail());
final DbUser invitedUser = userDao.findUserByUsername(role.getUserName());
if (invitedUser == null) {
throw new BadRequestException(String.format("User %s doesn't exist", role.getEmail()));
throw new BadRequestException(String.format("User %s doesn't exist", role.getUserName()));
}

aclStringsByUserIdBuilder.put(invitedUser.getUserId(), role.getRole().toString());
shareRolesMapBuilder.put(role.getEmail(), role.getRole());
shareRolesMapBuilder.put(role.getUserName(), role.getRole());
}
final ImmutableMap<String, WorkspaceAccessLevel> aclsByEmail = shareRolesMapBuilder.build();

Expand All @@ -742,9 +742,9 @@ public ResponseEntity<WorkspaceUserRolesResponse> shareWorkspacePatch(
u ->
(WorkspaceAccessLevel.OWNER.equals(u.getRole())
|| WorkspaceAccessLevel.WRITER.equals(u.getRole()))
&& !finalWorkflowUsers.contains(u.getEmail())
&& !u.getEmail().equals(currentUser.getUsername()))
.map(UserRole::getEmail)
&& !finalWorkflowUsers.contains(u.getUserName())
&& !u.getUserName().equals(currentUser.getUsername()))
.map(UserRole::getUserName)
.collect(Collectors.toList());
List<String> failedRevocations =
iamService.revokeWorkflowRunnerRoleForUsers(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ public void calculateObjectNameLength() {
final List<UserRole> workspaceOwners = getWorkspaceOwnersOrWriters(workspace);
final Set<DbUser> activeUsers =
userService.findActiveUsersByUsernames(
workspaceOwners.stream().map(UserRole::getEmail).collect(Collectors.toList()));
workspaceOwners.stream().map(UserRole::getUserName).collect(Collectors.toList()));

maybeAlertUsers(bucketAuditEntry, workspace, activeUsers);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ public static Map<String, RawlsWorkspaceAccessEntry> extractAclResponse(
}

public static RawlsWorkspaceACLUpdate buildAclUpdate(
String email, WorkspaceAccessLevel updatedAccess) {
RawlsWorkspaceACLUpdate update = new RawlsWorkspaceACLUpdate().email(email);
String username, WorkspaceAccessLevel updatedAccess) {
RawlsWorkspaceACLUpdate update = new RawlsWorkspaceACLUpdate().email(username);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

can't update the name of RawlsWorkspaceACLUpdate.email because that's defined by the Rawls API

if (updatedAccess == WorkspaceAccessLevel.OWNER) {
return update
.canShare(true)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,8 @@ private RdrWorkspace toRdrWorkspace(DbWorkspace dbWorkspace) {
(userRole) ->
new RdrWorkspaceUser()
.userId(
(int) userDao.findUserByUsername(userRole.getEmail()).getUserId())
(int)
userDao.findUserByUsername(userRole.getUserName()).getUserId())
.role(
RdrWorkspaceUser.RoleEnum.fromValue(userRole.getRole().toString()))
.status(RdrWorkspaceUser.StatusEnum.ACTIVE))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,20 +12,23 @@
config = MapStructConfig.class,
uses = {CommonMappers.class, FirecloudMapper.class})
public interface UserMapper {
@Mapping(target = "userName", source = "username")
User toApiUser(DbUser user);

User toUser(UserRole userRole);

@Mapping(source = "acl", target = "role")
@Mapping(source = "user.username", target = "email")
@Mapping(source = "user.username", target = "userName")
UserRole toApiUserRole(DbUser user, RawlsWorkspaceAccessEntry acl);

@Mapping(source = "contactEmail", target = "email")
@Mapping(source = "userRole.email", target = "userName")
User toApiUser(UserRole userRole, String contactEmail);

@Mapping(source = "contactEmail", target = "email")
@Mapping(source = "username", target = "userName")
User toApiUser(DbUser dbUser);

@Mapping(source = "dbUser.userId", target = "userDatabaseId")
@Mapping(source = "dbUser.creationTime", target = "userAccountCreatedTime")
@Mapping(source = "dbUser", target = "userModel")
WorkspaceUserAdminView toWorkspaceUserAdminView(DbUser dbUser, UserRole userRole);

@Mapping(target = "userModel", source = "userRole")
@Mapping(target = "userDatabaseId", ignore = true)
@Mapping(target = "userAccountCreatedTime", ignore = true)
WorkspaceUserAdminView toPartialWorkspaceUserAdminView(UserRole userRole);
}
Original file line number Diff line number Diff line change
Expand Up @@ -524,12 +524,8 @@ private long getStorageSizeBytes(String bucketName) {
// the DbUser, but we don't check that here.
private WorkspaceUserAdminView toWorkspaceUserAdminView(UserRole userRole) {
return userService
.getByUsername(userRole.getEmail())
.getByUsername(userRole.getUserName())
.map(u -> userMapper.toWorkspaceUserAdminView(u, userRole))
.orElse(
new WorkspaceUserAdminView() // the MapStruct-generated method won't handle a partial
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cleanup/simplify by adding a MapStruct method to handle the partial creation

// conversion
.role(userRole.getRole())
.userModel(userMapper.toApiUser(userRole, null)));
.orElse(userMapper.toPartialWorkspaceUserAdminView(userRole));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -355,8 +355,8 @@ public List<UserRole> getFirecloudUserRoles(String workspaceNamespace, String fi
}
return userRoles.stream()
.sorted(
Comparator.comparing(UserRole::getRole).thenComparing(UserRole::getEmail).reversed())
.collect(Collectors.toList());
Comparator.comparing(UserRole::getRole).thenComparing(UserRole::getUserName).reversed())
.toList();
}

@Override
Expand Down Expand Up @@ -657,7 +657,7 @@ public List<DbUser> getWorkspaceOwnerList(DbWorkspace dbWorkspace) {
dbWorkspace.getWorkspaceNamespace(), dbWorkspace.getFirecloudName())
.stream()
.filter(userRole -> userRole.getRole() == WorkspaceAccessLevel.OWNER)
.map(UserRole::getEmail)
.map(UserRole::getUserName)
.map(userService::getByUsernameOrThrow)
.toList();
}
Expand Down
14 changes: 10 additions & 4 deletions api/src/main/resources/workbench-api.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -7399,11 +7399,16 @@ components:
format: double
UserRole:
required:
- email
- userName
- givenName
- familyName
- role
type: object
properties:
email:
type: string
description: Username of the user (DEPRECATED in favor of userName)
userName:
type: string
description: Username of the user
givenName:
Expand Down Expand Up @@ -9260,10 +9265,11 @@ components:
format: int32
User:
type: object
required:
- userName
- givenName
- familyName
properties:
email:
type: string
description: researchallofus email address (deprecated in favor of userName)
userName:
type: string
description: Unique researchallofus username (a Google account email)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ public void setUp() {
dbWorkspace.setWorkspaceNamespace(WORKSPACE_NAMESPACE);
dbWorkspace.setFirecloudName(WORKSPACE_FIRECLOUD_NAME);
when(workspaceDao.getByGoogleProject(GOOGLE_PROJECT)).thenReturn(Optional.of(dbWorkspace));
firecloudUserRoles.add(new UserRole().email(USER_EMAIL));
firecloudUserRoles.add(new UserRole().userName(USER_EMAIL).email(USER_EMAIL));
when(mockWorkspaceService.getFirecloudUserRoles(WORKSPACE_NAMESPACE, WORKSPACE_FIRECLOUD_NAME))
.thenReturn(firecloudUserRoles);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package org.pmiops.workbench.api;

import static com.google.common.truth.Truth.assertThat;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.when;
Expand Down Expand Up @@ -149,8 +150,8 @@ public void testUserSearch() {
userController
.userSearch(registeredTier.getShortName(), "John", null, null, null)
.getBody();
assertNotNull(response);
assertThat(response.getUsers()).hasSize(1);
assertThat(response.getUsers().get(0).getEmail()).isEqualTo(john.getUsername());
assertThat(response.getUsers().get(0).getUserName()).isEqualTo(john.getUsername());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,10 @@ public void setUp() {
.thenReturn(Optional.of(dbWorkspace));

final UserRole collaborator =
new UserRole().email("[email protected]").role(WorkspaceAccessLevel.WRITER);
new UserRole()
.userName("[email protected]")
.email("[email protected]")
.role(WorkspaceAccessLevel.WRITER);
final List<UserRole> collaborators = List.of(collaborator);
when(mockWorkspaceService.getFirecloudUserRoles(WORKSPACE_NAMESPACE, WORKSPACE_TERRA_NAME))
.thenReturn(collaborators);
Expand Down
Loading