From 1defd4ec6558a3e75f52b561cde8670de7bb7d5f Mon Sep 17 00:00:00 2001 From: Joel Thibault Date: Mon, 9 Dec 2024 11:15:11 -0500 Subject: [PATCH 1/8] remove deprecated 'email' from the 'User' entity --- .../java/org/pmiops/workbench/api/UserController.java | 1 - .../org/pmiops/workbench/utils/mappers/UserMapper.java | 8 ++------ .../workspaceadmin/WorkspaceAdminServiceImpl.java | 2 +- api/src/main/resources/workbench-api.yaml | 3 --- 4 files changed, 3 insertions(+), 11 deletions(-) diff --git a/api/src/main/java/org/pmiops/workbench/api/UserController.java b/api/src/main/java/org/pmiops/workbench/api/UserController.java index 085ada0ae10..4737f4c6362 100644 --- a/api/src/main/java/org/pmiops/workbench/api/UserController.java +++ b/api/src/main/java/org/pmiops/workbench/api/UserController.java @@ -46,7 +46,6 @@ public class UserController implements UserApiDelegate { private static final Function 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()); diff --git a/api/src/main/java/org/pmiops/workbench/utils/mappers/UserMapper.java b/api/src/main/java/org/pmiops/workbench/utils/mappers/UserMapper.java index bf65dc2543e..5a277120e2b 100644 --- a/api/src/main/java/org/pmiops/workbench/utils/mappers/UserMapper.java +++ b/api/src/main/java/org/pmiops/workbench/utils/mappers/UserMapper.java @@ -16,16 +16,12 @@ public interface UserMapper { @Mapping(source = "user.username", target = "email") 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); + User toApiUser(UserRole userRole); @Mapping(source = "dbUser.userId", target = "userDatabaseId") @Mapping(source = "dbUser.creationTime", target = "userAccountCreatedTime") @Mapping(source = "dbUser", target = "userModel") + @Mapping(source = "userRole.email", target = "userModel.userName") WorkspaceUserAdminView toWorkspaceUserAdminView(DbUser dbUser, UserRole userRole); } diff --git a/api/src/main/java/org/pmiops/workbench/workspaceadmin/WorkspaceAdminServiceImpl.java b/api/src/main/java/org/pmiops/workbench/workspaceadmin/WorkspaceAdminServiceImpl.java index 1cdd7869dd3..50e845557cd 100644 --- a/api/src/main/java/org/pmiops/workbench/workspaceadmin/WorkspaceAdminServiceImpl.java +++ b/api/src/main/java/org/pmiops/workbench/workspaceadmin/WorkspaceAdminServiceImpl.java @@ -530,6 +530,6 @@ private WorkspaceUserAdminView toWorkspaceUserAdminView(UserRole userRole) { new WorkspaceUserAdminView() // the MapStruct-generated method won't handle a partial // conversion .role(userRole.getRole()) - .userModel(userMapper.toApiUser(userRole, null))); + .userModel(userMapper.toApiUser(userRole))); } } diff --git a/api/src/main/resources/workbench-api.yaml b/api/src/main/resources/workbench-api.yaml index b2e6052406a..d1a6e3db5e3 100644 --- a/api/src/main/resources/workbench-api.yaml +++ b/api/src/main/resources/workbench-api.yaml @@ -9261,9 +9261,6 @@ components: User: type: object properties: - email: - type: string - description: researchallofus email address (deprecated in favor of userName) userName: type: string description: Unique researchallofus username (a Google account email) From 6f6c182a6a8403e761a3a1b80bad6a53ca082317 Mon Sep 17 00:00:00 2001 From: Joel Thibault Date: Mon, 9 Dec 2024 11:42:56 -0500 Subject: [PATCH 2/8] UserRole email -> userName --- api/src/main/resources/workbench-api.yaml | 5 ++- .../pages/workspace/workspace-about.spec.tsx | 2 +- .../app/pages/workspace/workspace-about.tsx | 2 +- .../pages/workspace/workspace-share.spec.tsx | 26 +++++------ .../app/pages/workspace/workspace-share.tsx | 44 +++++++++---------- ui/src/testing/stubs/user-api-stub.ts | 2 +- ui/src/testing/stubs/workspaces.tsx | 9 ++-- 7 files changed, 44 insertions(+), 46 deletions(-) diff --git a/api/src/main/resources/workbench-api.yaml b/api/src/main/resources/workbench-api.yaml index d1a6e3db5e3..5e661da3a8a 100644 --- a/api/src/main/resources/workbench-api.yaml +++ b/api/src/main/resources/workbench-api.yaml @@ -7403,7 +7403,10 @@ components: - role type: object properties: - email: +# email: +# type: string +# description: Username of the user (DEPRECATED in favor of userName) + userName: type: string description: Username of the user givenName: diff --git a/ui/src/app/pages/workspace/workspace-about.spec.tsx b/ui/src/app/pages/workspace/workspace-about.spec.tsx index f416e25581d..4e1ddff068c 100644 --- a/ui/src/app/pages/workspace/workspace-about.spec.tsx +++ b/ui/src/app/pages/workspace/workspace-about.spec.tsx @@ -108,7 +108,7 @@ describe('WorkspaceAbout', () => { await waitForNoSpinner(); userRolesStub.forEach((role, i) => { const userRoleText = screen.getByTestId('workspaceUser-' + i).textContent; - expect(userRoleText).toContain(role.email); + expect(userRoleText).toContain(role.userName); expect(userRoleText).toContain(role.role.toString()); }); }); diff --git a/ui/src/app/pages/workspace/workspace-about.tsx b/ui/src/app/pages/workspace/workspace-about.tsx index c815ce8d38e..aebcd55496c 100644 --- a/ui/src/app/pages/workspace/workspace-about.tsx +++ b/ui/src/app/pages/workspace/workspace-about.tsx @@ -401,7 +401,7 @@ export const WorkspaceAbout = fp.flow( {workspaceUserRoles.map((user, i) => (
- {user.role + ' : ' + user.email} + {user.role + ' : ' + user.userName}
))} diff --git a/ui/src/app/pages/workspace/workspace-share.spec.tsx b/ui/src/app/pages/workspace/workspace-share.spec.tsx index 233cda33fd7..7d36b471285 100644 --- a/ui/src/app/pages/workspace/workspace-share.spec.tsx +++ b/ui/src/app/pages/workspace/workspace-share.spec.tsx @@ -10,7 +10,7 @@ import { } from 'generated/fetch'; import { render, screen, waitFor } from '@testing-library/react'; -import userEvent from '@testing-library/user-event'; +import userEvent, { UserEvent } from '@testing-library/user-event'; import { registerApiClient, workspacesApi, @@ -30,7 +30,7 @@ import { WorkspaceShare, WorkspaceShareProps } from './workspace-share'; describe(WorkspaceShare.name, () => { let props: WorkspaceShareProps; - let user; + let user: UserEvent; const component = () => { return render(); @@ -38,41 +38,37 @@ describe(WorkspaceShare.name, () => { const harry: User = { givenName: 'Harry', familyName: 'Potter', - email: 'harry.potter@hogwarts.edu', + userName: 'harry.potter@hogwarts.edu', }; const harryRole: UserRole = { ...harry, - email: harry.email, role: WorkspaceAccessLevel.OWNER, }; const hermione: User = { givenName: 'Hermione', familyName: 'Granger', - email: 'hermione.granger@hogwarts.edu', + userName: 'hermione.granger@hogwarts.edu', }; const hermioneRole: UserRole = { ...hermione, - email: hermione.email, role: WorkspaceAccessLevel.WRITER, }; const ron: User = { givenName: 'Ronald', familyName: 'Weasley', - email: 'ron.weasley@hogwarts.edu', + userName: 'ron.weasley@hogwarts.edu', }; const ronRole: UserRole = { ...ron, - email: ron.email, role: WorkspaceAccessLevel.READER, }; const luna: User = { givenName: 'Luna', familyName: 'Lovegood', - email: 'luna.lovegood@hogwarts.edu', + userName: 'luna.lovegood@hogwarts.edu', }; const lunaRole: UserRole = { ...luna, - email: luna.email, role: WorkspaceAccessLevel.NO_ACCESS, }; @@ -86,14 +82,14 @@ describe(WorkspaceShare.name, () => { const tomRiddleDiaryUserRoles = [harryRole, hermioneRole, ronRole]; const getUserRoleDropdownLabel = (desiredUser: User) => { - return `Role selector for ${desiredUser.email}`; + return `Role selector for ${desiredUser.userName}`; }; const getAddCollaboratorLabel = (desiredUser: User) => { - return `Button to add ${desiredUser.email} as a collaborator`; + return `Button to add ${desiredUser.userName} as a collaborator`; }; const getRemoveCollaboratorLabel = (desiredUser: User) => { - return `Button to remove ${desiredUser.email} as a collaborator`; + return `Button to remove ${desiredUser.userName} as a collaborator`; }; const addUser = async (userToAdd: User) => { @@ -155,7 +151,7 @@ describe(WorkspaceShare.name, () => { expectedUsers.map((u) => u.givenName + ' ' + u.familyName) ); expect(collabUserEmails.map((el) => el.textContent)).toEqual( - expectedUsers.map((u) => u.email) + expectedUsers.map((u) => u.userName) ); }); @@ -253,7 +249,7 @@ describe(WorkspaceShare.name, () => { ); await user.click(hermoineRoleDropdown); await user.click( - screen.getByLabelText(`Select Owner role for ${hermione.email}`) + screen.getByLabelText(`Select Owner role for ${hermione.userName}`) ); const spy = jest.spyOn(workspacesApi(), 'shareWorkspacePatch'); diff --git a/ui/src/app/pages/workspace/workspace-share.tsx b/ui/src/app/pages/workspace/workspace-share.tsx index 44756f15f89..68cf7620491 100644 --- a/ui/src/app/pages/workspace/workspace-share.tsx +++ b/ui/src/app/pages/workspace/workspace-share.tsx @@ -287,12 +287,12 @@ export const WorkspaceShare = fp.flow(withUserProfile())( removeCollaborator(user: UserRole): void { // remove from userRoles if it exists const userRoles = this.state.userRoles.filter( - ({ email }) => user.email !== email + ({ userName }) => user.userName !== userName ); // may or may not exist in the changeset (may have been added previously) const userRolesToChange = this.state.userRolesToChange - .filter(({ email }) => user.email !== email) + .filter(({ userName }) => user.userName !== userName) .concat({ ...user, role: WorkspaceAccessLevel.NO_ACCESS, @@ -306,9 +306,7 @@ export const WorkspaceShare = fp.flow(withUserProfile())( addCollaborator(user: User): void { const userRole: UserRole = { - givenName: user.givenName, - familyName: user.familyName, - email: user.email, + ...user, role: WorkspaceAccessLevel.READER, }; @@ -319,7 +317,7 @@ export const WorkspaceShare = fp.flow(withUserProfile())( // may or may not exist in the changeset (may have been set to NOACCESS previously) const userRolesToChange = this.state.userRolesToChange - .filter(({ email }) => user.email !== email) + .filter(({ userName }) => user.userName !== userName) .concat(userRole); this.setState({ @@ -352,7 +350,7 @@ export const WorkspaceShare = fp.flow(withUserProfile())( } response.users = fp.differenceWith( (a, b) => { - return a.email === b.email; + return a.userName === b.userName; }, response.users, this.state.userRoles @@ -367,12 +365,12 @@ export const WorkspaceShare = fp.flow(withUserProfile())( setRole(e, user: UserRole): void { const userRoles = fp.map((u) => { - return u.email === user.email ? { ...u, role: e } : u; + return u.userName === user.userName ? { ...u, role: e } : u; }, this.state.userRoles); // may or may not already exist in the changeset const userRolesToChange = this.state.userRolesToChange - .filter(({ email }) => user.email !== email) + .filter(({ userName }) => user.userName !== userName) .concat({ ...user, role: e }); this.setState({ @@ -547,7 +545,7 @@ export const WorkspaceShare = fp.flow(withUserProfile())( > {this.state.autocompleteUsers.map((user) => { return ( -
+
@@ -557,14 +555,14 @@ export const WorkspaceShare = fp.flow(withUserProfile())( data-test-id='user-email' style={styles.userName} > - {user.email} + {user.userName}
{ this.addCollaborator(user); @@ -607,7 +605,7 @@ export const WorkspaceShare = fp.flow(withUserProfile())( > {this.state.userRoles.map((user, i) => { return ( -
+
- {user.email} + {user.userName}
{/* Minimally, the z-index must be higher than that of the modal. See https://react-select.com/advanced#portaling */} @@ -642,38 +640,38 @@ export const WorkspaceShare = fp.flow(withUserProfile())( 'popup-root' )} isDisabled={ - user.email === + user.userName === this.props.profileState.profile.username } classNamePrefix={this.cleanClassNameForSelect( - user.email + user.userName )} - data-test-id={user.email + '-user-role'} + data-test-id={user.userName + '-user-role'} onChange={(e) => this.setRole(e, user)} options={UserRoleOptions} formatOptionLabel={({ label }, {}) => { return (
{label}
); }} - aria-label={`Role selector for ${user.email}`} + aria-label={`Role selector for ${user.userName}`} />
{this.hasPermission && - user.email !== + user.userName !== this.props.profileState.profile .username && ( diff --git a/ui/src/testing/stubs/user-api-stub.ts b/ui/src/testing/stubs/user-api-stub.ts index 22093d1ab58..980a7f1f6fa 100644 --- a/ui/src/testing/stubs/user-api-stub.ts +++ b/ui/src/testing/stubs/user-api-stub.ts @@ -30,8 +30,8 @@ export class UserApiStub extends UserApi { } else { usersToReturn.push({ familyName: 'User4', - email: 'sampleuser4@fake-research-aou.org', givenName: 'Sample', + userName: 'sampleuser4@fake-research-aou.org', } as User); } const userResponse: UserResponse = { diff --git a/ui/src/testing/stubs/workspaces.tsx b/ui/src/testing/stubs/workspaces.tsx index 7a1dd13d766..a006247df42 100644 --- a/ui/src/testing/stubs/workspaces.tsx +++ b/ui/src/testing/stubs/workspaces.tsx @@ -3,6 +3,7 @@ import { RecentWorkspace, RecentWorkspaceResponse, SpecificPopulationEnum, + UserRole, Workspace, WorkspaceAccessLevel, } from 'generated/fetch'; @@ -91,21 +92,21 @@ export const workspaceDataStub = { accessLevel: WorkspaceAccessLevel.OWNER, }; -export const userRolesStub = [ +export const userRolesStub: UserRole[] = [ { - email: 'sampleuser1@fake-research-aou.org', + userName: 'sampleuser1@fake-research-aou.org', givenName: 'Sample', familyName: 'User1', role: WorkspaceAccessLevel.OWNER, }, { - email: 'sampleuser2@fake-research-aou.org', + userName: 'sampleuser2@fake-research-aou.org', givenName: 'Sample', familyName: 'User2', role: WorkspaceAccessLevel.WRITER, }, { - email: 'sampleuser3@fake-research-aou.org', + userName: 'sampleuser3@fake-research-aou.org', givenName: 'Sample', familyName: 'User3', role: WorkspaceAccessLevel.READER, From 456ebad32200ea6449cecdf040570d81b8d52970 Mon Sep 17 00:00:00 2001 From: Joel Thibault Date: Mon, 9 Dec 2024 12:10:30 -0500 Subject: [PATCH 3/8] API and mapper fixes --- .../workbench/utils/mappers/UserMapper.java | 15 +++++++++++---- .../workspaceadmin/WorkspaceAdminServiceImpl.java | 8 ++------ api/src/main/resources/workbench-api.yaml | 14 ++++++++++---- 3 files changed, 23 insertions(+), 14 deletions(-) diff --git a/api/src/main/java/org/pmiops/workbench/utils/mappers/UserMapper.java b/api/src/main/java/org/pmiops/workbench/utils/mappers/UserMapper.java index 5a277120e2b..1b7ddaaa92a 100644 --- a/api/src/main/java/org/pmiops/workbench/utils/mappers/UserMapper.java +++ b/api/src/main/java/org/pmiops/workbench/utils/mappers/UserMapper.java @@ -12,16 +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 = "userRole.email", target = "userName") - User toApiUser(UserRole userRole); - @Mapping(source = "dbUser.userId", target = "userDatabaseId") @Mapping(source = "dbUser.creationTime", target = "userAccountCreatedTime") @Mapping(source = "dbUser", target = "userModel") - @Mapping(source = "userRole.email", target = "userModel.userName") 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); } diff --git a/api/src/main/java/org/pmiops/workbench/workspaceadmin/WorkspaceAdminServiceImpl.java b/api/src/main/java/org/pmiops/workbench/workspaceadmin/WorkspaceAdminServiceImpl.java index 50e845557cd..5127b604949 100644 --- a/api/src/main/java/org/pmiops/workbench/workspaceadmin/WorkspaceAdminServiceImpl.java +++ b/api/src/main/java/org/pmiops/workbench/workspaceadmin/WorkspaceAdminServiceImpl.java @@ -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 - // conversion - .role(userRole.getRole()) - .userModel(userMapper.toApiUser(userRole))); + .orElse(userMapper.toPartialWorkspaceUserAdminView(userRole)); } } diff --git a/api/src/main/resources/workbench-api.yaml b/api/src/main/resources/workbench-api.yaml index 5e661da3a8a..cf7eb56f1f8 100644 --- a/api/src/main/resources/workbench-api.yaml +++ b/api/src/main/resources/workbench-api.yaml @@ -7399,13 +7399,15 @@ 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) + email: + type: string + description: Username of the user (DEPRECATED in favor of userName) userName: type: string description: Username of the user @@ -9263,6 +9265,10 @@ components: format: int32 User: type: object + required: + - userName + - givenName + - familyName properties: userName: type: string From 030056ca8fd9a70eab1f70e8cdf626dc696c7ca0 Mon Sep 17 00:00:00 2001 From: Joel Thibault Date: Mon, 9 Dec 2024 12:14:27 -0500 Subject: [PATCH 4/8] test fixes --- .../pmiops/workbench/utils/mappers/UserMapper.java | 4 ++-- .../pmiops/workbench/api/UserControllerTest.java | 3 ++- .../exfiltration/EgressEventServiceTest.java | 13 ++----------- 3 files changed, 6 insertions(+), 14 deletions(-) diff --git a/api/src/main/java/org/pmiops/workbench/utils/mappers/UserMapper.java b/api/src/main/java/org/pmiops/workbench/utils/mappers/UserMapper.java index 1b7ddaaa92a..b30ddfa6a4a 100644 --- a/api/src/main/java/org/pmiops/workbench/utils/mappers/UserMapper.java +++ b/api/src/main/java/org/pmiops/workbench/utils/mappers/UserMapper.java @@ -12,7 +12,7 @@ config = MapStructConfig.class, uses = {CommonMappers.class, FirecloudMapper.class}) public interface UserMapper { - @Mapping(target = "userName", source="username") + @Mapping(target = "userName", source = "username") User toApiUser(DbUser user); User toUser(UserRole userRole); @@ -27,7 +27,7 @@ public interface UserMapper { @Mapping(source = "dbUser", target = "userModel") WorkspaceUserAdminView toWorkspaceUserAdminView(DbUser dbUser, UserRole userRole); - @Mapping(target = "userModel", source="userRole") + @Mapping(target = "userModel", source = "userRole") @Mapping(target = "userDatabaseId", ignore = true) @Mapping(target = "userAccountCreatedTime", ignore = true) WorkspaceUserAdminView toPartialWorkspaceUserAdminView(UserRole userRole); diff --git a/api/src/test/java/org/pmiops/workbench/api/UserControllerTest.java b/api/src/test/java/org/pmiops/workbench/api/UserControllerTest.java index eb1baedfbbb..fedaf4ed5a0 100644 --- a/api/src/test/java/org/pmiops/workbench/api/UserControllerTest.java +++ b/api/src/test/java/org/pmiops/workbench/api/UserControllerTest.java @@ -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; @@ -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()); } diff --git a/api/src/test/java/org/pmiops/workbench/exfiltration/EgressEventServiceTest.java b/api/src/test/java/org/pmiops/workbench/exfiltration/EgressEventServiceTest.java index b033c5f92b9..c2589108361 100644 --- a/api/src/test/java/org/pmiops/workbench/exfiltration/EgressEventServiceTest.java +++ b/api/src/test/java/org/pmiops/workbench/exfiltration/EgressEventServiceTest.java @@ -61,11 +61,7 @@ public class EgressEventServiceTest { @Autowired private LeonardoApiClient leonardoApiClient; private static final User USER_1 = - new User() - .givenName("Fredward") - .familyName("Fredrickson") - .userName("fred@aou.biz") - .email("freddie@fred.fred.fred.ca"); + new User().givenName("Fredward").familyName("Fredrickson").userName("fred@aou.biz"); private DbUser dbUser1; private static final WorkspaceUserAdminView ADMIN_VIEW_1 = @@ -78,11 +74,7 @@ public class EgressEventServiceTest { "2018-08-30T01:20+02:00", DateTimeFormatter.ISO_OFFSET_DATE_TIME)); private static final User USER_2 = - new User() - .givenName("Theororathy") - .familyName("Kim") - .userName("theodorothy@aou.biz") - .email("theodorothy@fred.fred.fred.org"); + new User().givenName("Theororathy").familyName("Kim").userName("theodorothy@aou.biz"); private DbUser dbUser2; private static final WorkspaceUserAdminView ADMIN_VIEW_2 = @@ -319,7 +311,6 @@ private static DbUser workspaceAdminUserViewToUser(WorkspaceUserAdminView adminV result.setGivenName(userModel.getGivenName()); result.setFamilyName(userModel.getFamilyName()); result.setUsername(userModel.getUserName()); - result.setContactEmail(userModel.getEmail()); result.setCreationTime(Timestamp.from(adminView.getUserAccountCreatedTime().toInstant())); return result; } From f2042deeade6c57487b29602ae8b93668ffa0b98 Mon Sep 17 00:00:00 2001 From: Joel Thibault Date: Mon, 9 Dec 2024 13:25:03 -0500 Subject: [PATCH 5/8] switch API uses --- .../auditors/EgressEventAuditorImpl.java | 2 +- .../workbench/api/WorkspacesController.java | 12 +- .../ObjectNameLengthServiceImpl.java | 2 +- .../firecloud/FirecloudTransforms.java | 4 +- .../workbench/rdr/RdrExportServiceImpl.java | 3 +- .../workspaces/WorkspaceServiceImpl.java | 6 +- .../auditors/EgressEventAuditorTest.java | 2 +- .../api/WorkspaceAdminControllerTest.java | 5 +- .../api/WorkspacesControllerTest.java | 109 ++++++++++++------ .../ObjectNameLengthServiceTest.java | 21 ++-- .../workbench/utils/TestMockFactory.java | 4 +- 11 files changed, 105 insertions(+), 65 deletions(-) diff --git a/api/src/main/java/org/pmiops/workbench/actionaudit/auditors/EgressEventAuditorImpl.java b/api/src/main/java/org/pmiops/workbench/actionaudit/auditors/EgressEventAuditorImpl.java index ee45dfd93ee..7780a46e450 100644 --- a/api/src/main/java/org/pmiops/workbench/actionaudit/auditors/EgressEventAuditorImpl.java +++ b/api/src/main/java/org/pmiops/workbench/actionaudit/auditors/EgressEventAuditorImpl.java @@ -88,7 +88,7 @@ public void fireEgressEvent(SumologicEgressEvent event) { // 3. Dataproc worker nodes: all-of-us--w- var vmOwner = userRoles.stream() - .map(UserRole::getEmail) + .map(UserRole::getUserName) .map(userDao::findUserByUsername) .filter(user -> user.getRuntimeName().equals(event.getVmPrefix())) .findFirst() diff --git a/api/src/main/java/org/pmiops/workbench/api/WorkspacesController.java b/api/src/main/java/org/pmiops/workbench/api/WorkspacesController.java index ccef56920da..a7829caf7ee 100644 --- a/api/src/main/java/org/pmiops/workbench/api/WorkspacesController.java +++ b/api/src/main/java/org/pmiops/workbench/api/WorkspacesController.java @@ -712,13 +712,13 @@ public ResponseEntity 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 aclsByEmail = shareRolesMapBuilder.build(); @@ -742,9 +742,9 @@ public ResponseEntity 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 failedRevocations = iamService.revokeWorkflowRunnerRoleForUsers( diff --git a/api/src/main/java/org/pmiops/workbench/exfiltration/ObjectNameLengthServiceImpl.java b/api/src/main/java/org/pmiops/workbench/exfiltration/ObjectNameLengthServiceImpl.java index 7ae7708b688..881b0fcfaec 100644 --- a/api/src/main/java/org/pmiops/workbench/exfiltration/ObjectNameLengthServiceImpl.java +++ b/api/src/main/java/org/pmiops/workbench/exfiltration/ObjectNameLengthServiceImpl.java @@ -127,7 +127,7 @@ public void calculateObjectNameLength() { final List workspaceOwners = getWorkspaceOwnersOrWriters(workspace); final Set activeUsers = userService.findActiveUsersByUsernames( - workspaceOwners.stream().map(UserRole::getEmail).collect(Collectors.toList())); + workspaceOwners.stream().map(UserRole::getUserName).collect(Collectors.toList())); maybeAlertUsers(bucketAuditEntry, workspace, activeUsers); } diff --git a/api/src/main/java/org/pmiops/workbench/firecloud/FirecloudTransforms.java b/api/src/main/java/org/pmiops/workbench/firecloud/FirecloudTransforms.java index feb65ec5604..c1ba7219c4e 100644 --- a/api/src/main/java/org/pmiops/workbench/firecloud/FirecloudTransforms.java +++ b/api/src/main/java/org/pmiops/workbench/firecloud/FirecloudTransforms.java @@ -25,8 +25,8 @@ public static Map 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); if (updatedAccess == WorkspaceAccessLevel.OWNER) { return update .canShare(true) diff --git a/api/src/main/java/org/pmiops/workbench/rdr/RdrExportServiceImpl.java b/api/src/main/java/org/pmiops/workbench/rdr/RdrExportServiceImpl.java index 757b945ce6d..a84545b3866 100644 --- a/api/src/main/java/org/pmiops/workbench/rdr/RdrExportServiceImpl.java +++ b/api/src/main/java/org/pmiops/workbench/rdr/RdrExportServiceImpl.java @@ -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)) diff --git a/api/src/main/java/org/pmiops/workbench/workspaces/WorkspaceServiceImpl.java b/api/src/main/java/org/pmiops/workbench/workspaces/WorkspaceServiceImpl.java index 1d053d35926..b1c6b8cf194 100644 --- a/api/src/main/java/org/pmiops/workbench/workspaces/WorkspaceServiceImpl.java +++ b/api/src/main/java/org/pmiops/workbench/workspaces/WorkspaceServiceImpl.java @@ -355,8 +355,8 @@ public List 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 @@ -657,7 +657,7 @@ public List getWorkspaceOwnerList(DbWorkspace dbWorkspace) { dbWorkspace.getWorkspaceNamespace(), dbWorkspace.getFirecloudName()) .stream() .filter(userRole -> userRole.getRole() == WorkspaceAccessLevel.OWNER) - .map(UserRole::getEmail) + .map(UserRole::getUserName) .map(userService::getByUsernameOrThrow) .toList(); } diff --git a/api/src/test/java/org/pmiops/workbench/actionaudit/auditors/EgressEventAuditorTest.java b/api/src/test/java/org/pmiops/workbench/actionaudit/auditors/EgressEventAuditorTest.java index 9035839679a..a06e90c65e3 100644 --- a/api/src/test/java/org/pmiops/workbench/actionaudit/auditors/EgressEventAuditorTest.java +++ b/api/src/test/java/org/pmiops/workbench/actionaudit/auditors/EgressEventAuditorTest.java @@ -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); } diff --git a/api/src/test/java/org/pmiops/workbench/api/WorkspaceAdminControllerTest.java b/api/src/test/java/org/pmiops/workbench/api/WorkspaceAdminControllerTest.java index 87fc516cb50..3e42699796d 100644 --- a/api/src/test/java/org/pmiops/workbench/api/WorkspaceAdminControllerTest.java +++ b/api/src/test/java/org/pmiops/workbench/api/WorkspaceAdminControllerTest.java @@ -107,7 +107,10 @@ public void setUp() { .thenReturn(Optional.of(dbWorkspace)); final UserRole collaborator = - new UserRole().email("test@test.test").role(WorkspaceAccessLevel.WRITER); + new UserRole() + .userName("test@test.test") + .email("test@test.test") + .role(WorkspaceAccessLevel.WRITER); final List collaborators = List.of(collaborator); when(mockWorkspaceService.getFirecloudUserRoles(WORKSPACE_NAMESPACE, WORKSPACE_TERRA_NAME)) .thenReturn(collaborators); diff --git a/api/src/test/java/org/pmiops/workbench/api/WorkspacesControllerTest.java b/api/src/test/java/org/pmiops/workbench/api/WorkspacesControllerTest.java index a836b330496..4adfd387a30 100644 --- a/api/src/test/java/org/pmiops/workbench/api/WorkspacesControllerTest.java +++ b/api/src/test/java/org/pmiops/workbench/api/WorkspacesControllerTest.java @@ -233,7 +233,7 @@ @Transactional(propagation = Propagation.NOT_SUPPORTED) public class WorkspacesControllerTest { - private static final String LOGGED_IN_USER_EMAIL = "bob@gmail.com"; + private static final String LOGGED_IN_USERNAME = "bob@researchallofus.org"; private static final String CLONE_GOOGLE_PROJECT_ID = "clone-project-id"; private static final Concept CLIENT_CONCEPT_1 = @@ -418,7 +418,7 @@ public void setUp() throws Exception { workbenchConfig.billing.accountId = "free-tier"; workbenchConfig.billing.projectNamePrefix = "aou-local"; - currentUser = createUser(LOGGED_IN_USER_EMAIL); + currentUser = createUser(LOGGED_IN_USERNAME); registeredTier = accessTierDao.save(createRegisteredTier()); when(cohortBuilderService.findAllDemographicsMap()).thenReturn(HashBasedTable.create()); @@ -452,9 +452,9 @@ public void setUp() throws Exception { .thenReturn(new ProjectBillingInfo().setBillingEnabled(true)); } - private DbUser createUser(String email) { + private DbUser createUser(String username) { DbUser user = new DbUser(); - user.setUsername(email); + user.setUsername(username); user.setDisabled(false); return userDao.save(user); } @@ -497,10 +497,11 @@ private void stubFcGetGroup() { private void stubGetWorkspace( String workspaceNamespace, String workspaceTerraName, - String creator, + String creatorUsername, WorkspaceAccessLevel access) { stubGetWorkspace( - TestMockFactory.createTerraWorkspace(workspaceNamespace, workspaceTerraName, creator), + TestMockFactory.createTerraWorkspace( + workspaceNamespace, workspaceTerraName, creatorUsername), access); } @@ -527,11 +528,11 @@ private void stubGetWorkspace(RawlsWorkspaceDetails fcWorkspace, WorkspaceAccess * if needed. */ private RawlsWorkspaceDetails stubCloneWorkspace( - String toNamespace, String toFirecloudName, String creator) { + String toNamespace, String toFirecloudName, String creatorUsername) { RawlsWorkspaceDetails fcResponse = new RawlsWorkspaceDetails(); fcResponse.setNamespace(toNamespace); fcResponse.setName(toFirecloudName); - fcResponse.setCreatedBy(creator); + fcResponse.setCreatedBy(creatorUsername); fcResponse.setGoogleProject(CLONE_GOOGLE_PROJECT_ID); when(fireCloudService.cloneWorkspace( @@ -611,8 +612,8 @@ public Cohort createDefaultCohort(String name) { private List convertUserRolesToUpdateAclRequestList( Collection collaborators) { return collaborators.stream() - .map(c -> FirecloudTransforms.buildAclUpdate(c.getEmail(), c.getRole())) - .collect(Collectors.toList()); + .map(c -> FirecloudTransforms.buildAclUpdate(c.getUserName(), c.getRole())) + .toList(); } private Workspace createWorkspaceAndGrantAccess(WorkspaceAccessLevel accessLevel) { @@ -657,7 +658,7 @@ public void testCreateWorkspace() { stubGetWorkspace( workspace.getNamespace(), workspace.getTerraName(), - LOGGED_IN_USER_EMAIL, + LOGGED_IN_USERNAME, WorkspaceAccessLevel.OWNER); Workspace retrievedWorkspace = workspacesController @@ -669,7 +670,7 @@ public void testCreateWorkspace() { assertThat(retrievedWorkspace.getCdrVersionId()).isEqualTo(cdrVersionId); assertThat(retrievedWorkspace.getAccessTierShortName()) .isEqualTo(registeredTier.getShortName()); - assertThat(retrievedWorkspace.getCreator()).isEqualTo(LOGGED_IN_USER_EMAIL); + assertThat(retrievedWorkspace.getCreator()).isEqualTo(LOGGED_IN_USERNAME); assertThat(retrievedWorkspace.getName()).isEqualTo(testWorkspaceDisplayName); assertThat(retrievedWorkspace.getDisplayName()).isEqualTo(testWorkspaceDisplayName); assertThat(retrievedWorkspace.getTerraName()).isEqualTo(testWorkspaceTerraName); @@ -1040,7 +1041,7 @@ public void testDuplicateWorkspaceAsync_and_process() { assertThat(operation2).isEqualTo(operation); // mocks Terra returning workspace duplication info - stubCloneWorkspace(workspace.getNamespace(), workspace.getTerraName(), LOGGED_IN_USER_EMAIL); + stubCloneWorkspace(workspace.getNamespace(), workspace.getTerraName(), LOGGED_IN_USERNAME); DuplicateWorkspaceTaskRequest request2 = new DuplicateWorkspaceTaskRequest() @@ -1371,7 +1372,7 @@ public void testCloneWorkspace() { req.setWorkspace(modWorkspace); final RawlsWorkspaceDetails clonedFirecloudWorkspace = stubCloneWorkspace( - modWorkspace.getNamespace(), modWorkspace.getTerraName(), LOGGED_IN_USER_EMAIL); + modWorkspace.getNamespace(), modWorkspace.getTerraName(), LOGGED_IN_USERNAME); // Assign the same bucket name as the mock-factory's bucket name, so the clone vs. get equality // assertion below will pass. clonedFirecloudWorkspace.setBucketName(TestMockFactory.WORKSPACE_BUCKET_NAME); @@ -1428,7 +1429,7 @@ public void testCloneWorkspace_resetBillingOnFailedSave() throws Exception { final CloneWorkspaceRequest req = new CloneWorkspaceRequest(); req.setWorkspace(modWorkspace); stubCloneWorkspace( - modWorkspace.getNamespace(), modWorkspace.getTerraName(), LOGGED_IN_USER_EMAIL); + modWorkspace.getNamespace(), modWorkspace.getTerraName(), LOGGED_IN_USERNAME); doThrow(new RuntimeException()).when(workspaceDao).save(any(DbWorkspace.class)); @@ -1465,7 +1466,7 @@ public void testCloneWorkspace_doNotUpdateBillingForFreeTier() { final CloneWorkspaceRequest req = new CloneWorkspaceRequest(); req.setWorkspace(modWorkspace); stubCloneWorkspace( - modWorkspace.getNamespace(), modWorkspace.getTerraName(), LOGGED_IN_USER_EMAIL); + modWorkspace.getNamespace(), modWorkspace.getTerraName(), LOGGED_IN_USERNAME); workspacesController.cloneWorkspace( originalWorkspace.getNamespace(), originalWorkspace.getTerraName(), req); @@ -1504,7 +1505,7 @@ public void testCloneWorkspace_accessTierMismatch() { final CloneWorkspaceRequest req = new CloneWorkspaceRequest(); req.setWorkspace(modWorkspace); stubCloneWorkspace( - modWorkspace.getNamespace(), modWorkspace.getTerraName(), LOGGED_IN_USER_EMAIL); + modWorkspace.getNamespace(), modWorkspace.getTerraName(), LOGGED_IN_USERNAME); workspacesController.cloneWorkspace( originalWorkspace.getNamespace(), originalWorkspace.getTerraName(), req); }); @@ -1521,17 +1522,18 @@ private void sortPopulationDetails(ResearchPurpose researchPurpose) { private void addUserRoleToShareWorkspaceRequest( ShareWorkspaceRequest shareWorkspaceRequest, - String email, + String userName, WorkspaceAccessLevel workspaceAccessLevel) { final UserRole userRole = new UserRole(); - userRole.setEmail(email); + userRole.setUserName(userName); + userRole.setEmail(userName); userRole.setRole(workspaceAccessLevel); shareWorkspaceRequest.addItemsItem(userRole); } - private DbUser createAndSaveUser(String email, long userId) { + private DbUser createAndSaveUser(String username, long userId) { DbUser writerUser = new DbUser(); - writerUser.setUsername(email); + writerUser.setUsername(username); writerUser.setUserId(userId); writerUser.setDisabled(false); @@ -1749,7 +1751,7 @@ public void testCloneWorkspaceWithCohortsAndConceptSets() { req.setWorkspace(modWorkspace); final RawlsWorkspaceDetails clonedWorkspace = stubCloneWorkspace( - modWorkspace.getNamespace(), modWorkspace.getTerraName(), LOGGED_IN_USER_EMAIL); + modWorkspace.getNamespace(), modWorkspace.getTerraName(), LOGGED_IN_USERNAME); stubGetWorkspace(clonedWorkspace, WorkspaceAccessLevel.WRITER); Workspace cloned = @@ -1933,7 +1935,7 @@ public void testCloneWorkspaceWithConceptSetNewCdrVersionNewConceptSetCount() { RawlsWorkspaceDetails clonedWorkspace = stubCloneWorkspace( - modWorkspace.getNamespace(), modWorkspace.getTerraName(), LOGGED_IN_USER_EMAIL); + modWorkspace.getNamespace(), modWorkspace.getTerraName(), LOGGED_IN_USERNAME); when(conceptBigQueryService.getParticipantCountForConcepts( Domain.CONDITION, ImmutableSet.of(dbConceptSetConceptId1, dbConceptSetConceptId2))) @@ -2035,12 +2037,12 @@ public void testCloneWorkspace_Dataset() { stubGetWorkspace( modWorkspace.getNamespace(), modWorkspace.getTerraName(), - LOGGED_IN_USER_EMAIL, + LOGGED_IN_USERNAME, WorkspaceAccessLevel.OWNER); stubFcGetWorkspaceACL(); RawlsWorkspaceDetails clonedWorkspace = stubCloneWorkspace( - modWorkspace.getNamespace(), modWorkspace.getTerraName(), LOGGED_IN_USER_EMAIL); + modWorkspace.getNamespace(), modWorkspace.getTerraName(), LOGGED_IN_USERNAME); stubGetWorkspace(clonedWorkspace, WorkspaceAccessLevel.READER); Workspace cloned = @@ -2338,7 +2340,7 @@ public void testCloneWorkspaceIncludeUserRoles() { .put("canCompute", true) .put("canShare", false)) .put( - LOGGED_IN_USER_EMAIL, + LOGGED_IN_USERNAME, new JSONObject() .put("accessLevel", "OWNER") .put("canCompute", true) @@ -2367,11 +2369,24 @@ public void testCloneWorkspaceIncludeUserRoles() { List expectedCollaboratorsAfterUpdate = convertUserRolesToUpdateAclRequestList( Set.of( - new UserRole().email(cloner.getUsername()).role(WorkspaceAccessLevel.OWNER), - new UserRole().email(LOGGED_IN_USER_EMAIL).role(WorkspaceAccessLevel.OWNER), - new UserRole().email(reader.getUsername()).role(WorkspaceAccessLevel.READER), - new UserRole().email(writer.getUsername()).role(WorkspaceAccessLevel.WRITER), new UserRole() + .userName(cloner.getUsername()) + .email(cloner.getUsername()) + .role(WorkspaceAccessLevel.OWNER), + new UserRole() + .userName(LOGGED_IN_USERNAME) + .email(LOGGED_IN_USERNAME) + .role(WorkspaceAccessLevel.OWNER), + new UserRole() + .userName(reader.getUsername()) + .email(reader.getUsername()) + .role(WorkspaceAccessLevel.READER), + new UserRole() + .userName(writer.getUsername()) + .email(writer.getUsername()) + .role(WorkspaceAccessLevel.WRITER), + new UserRole() + .userName(workspaceService.getPublishedWorkspacesGroupEmail()) .email(workspaceService.getPublishedWorkspacesGroupEmail()) .role(WorkspaceAccessLevel.NO_ACCESS))); @@ -2531,9 +2546,15 @@ public void testShareWorkspacePatch_AddBillingProjectUser() { new ShareWorkspaceRequest() .workspaceEtag(workspace.getEtag()) .addItemsItem( - new UserRole().email(writerUser.getUsername()).role(WorkspaceAccessLevel.WRITER)) + new UserRole() + .userName(writerUser.getUsername()) + .email(writerUser.getUsername()) + .role(WorkspaceAccessLevel.WRITER)) .addItemsItem( - new UserRole().email(ownerUser.getUsername()).role(WorkspaceAccessLevel.OWNER)); + new UserRole() + .userName(ownerUser.getUsername()) + .email(ownerUser.getUsername()) + .role(WorkspaceAccessLevel.OWNER)); stubFcUpdateWorkspaceACL(); workspacesController.shareWorkspacePatch( @@ -2584,9 +2605,15 @@ public void testShareWorkspacePatch_removeBillingProjectUser() { .workspaceEtag(workspace.getEtag()) // Removed WRITER, demoted OWNER to READER. .addItemsItem( - new UserRole().email(writerUser.getUsername()).role(WorkspaceAccessLevel.NO_ACCESS)) + new UserRole() + .userName(writerUser.getUsername()) + .email(writerUser.getUsername()) + .role(WorkspaceAccessLevel.NO_ACCESS)) .addItemsItem( - new UserRole().email(ownerUser.getUsername()).role(WorkspaceAccessLevel.READER)); + new UserRole() + .userName(ownerUser.getUsername()) + .email(ownerUser.getUsername()) + .role(WorkspaceAccessLevel.READER)); stubFcUpdateWorkspaceACL(); workspacesController.shareWorkspacePatch( @@ -2642,6 +2669,7 @@ public void testShareWorkspacePatch_publishedWorkspace() { // Removing writer. .addItemsItem( new UserRole() + .userName(writerUser.getUsername()) .email(writerUser.getUsername()) .role(WorkspaceAccessLevel.NO_ACCESS)); @@ -2668,8 +2696,9 @@ public void testSharePatch_workspaceNoRoleFailure() { ShareWorkspaceRequest shareWorkspaceRequest = new ShareWorkspaceRequest(); shareWorkspaceRequest.setWorkspaceEtag(workspace.getEtag()); addUserRoleToShareWorkspaceRequest( - shareWorkspaceRequest, LOGGED_IN_USER_EMAIL, WorkspaceAccessLevel.OWNER); + shareWorkspaceRequest, LOGGED_IN_USERNAME, WorkspaceAccessLevel.OWNER); UserRole writer = new UserRole(); + writer.setUserName("writerfriend@gmail.com"); writer.setEmail("writerfriend@gmail.com"); shareWorkspaceRequest.addItemsItem(writer); @@ -2697,7 +2726,7 @@ public void testUnshareWorkspace() { createWorkspaceACL( new JSONObject() .put( - LOGGED_IN_USER_EMAIL, + LOGGED_IN_USERNAME, new JSONObject() .put("accessLevel", "OWNER") .put("canCompute", true) @@ -2722,6 +2751,7 @@ public void testUnshareWorkspace() { final ShareWorkspaceRequest shareWorkspaceRequest = new ShareWorkspaceRequest(); shareWorkspaceRequest.setWorkspaceEtag(workspace.getEtag()); UserRole reader = new UserRole(); + reader.setUserName(readerUser.getUsername()); reader.setEmail(readerUser.getUsername()); reader.setRole(WorkspaceAccessLevel.NO_ACCESS); shareWorkspaceRequest.addItemsItem(reader); @@ -2816,7 +2846,10 @@ public void testGetFirecloudWorkspaceUserRoles() { assertThat(resp.getItems()) .containsExactly( - new UserRole().email(currentUser.getUsername()).role(WorkspaceAccessLevel.OWNER)); + new UserRole() + .userName(currentUser.getUsername()) + .email(currentUser.getUsername()) + .role(WorkspaceAccessLevel.OWNER)); } @Test @@ -2878,7 +2911,7 @@ public void getUserRecentWorkspaces() { stubGetWorkspace( workspace.getNamespace(), workspace.getTerraName(), - LOGGED_IN_USER_EMAIL, + LOGGED_IN_USERNAME, WorkspaceAccessLevel.OWNER); DbWorkspace dbWorkspace = workspaceDao.get(workspace.getNamespace(), workspace.getTerraName()); workspaceService.updateRecentWorkspaces(dbWorkspace); diff --git a/api/src/test/java/org/pmiops/workbench/exfiltration/ObjectNameLengthServiceTest.java b/api/src/test/java/org/pmiops/workbench/exfiltration/ObjectNameLengthServiceTest.java index 8129d485d67..b94530a7248 100644 --- a/api/src/test/java/org/pmiops/workbench/exfiltration/ObjectNameLengthServiceTest.java +++ b/api/src/test/java/org/pmiops/workbench/exfiltration/ObjectNameLengthServiceTest.java @@ -45,7 +45,7 @@ public class ObjectNameLengthServiceTest { public static final String GOOGLE_PROJECT = "google-project"; public static final String FIRECLOUD_NAME = "firecloud-name"; - public static final String USER_EMAIL = "test@aou.com"; + public static final String USERNAME = "test@aou.com"; public static final String NAMESPACE = "namespace"; public static final String PET_ACCOUNT = "pet@service.com"; @@ -97,15 +97,15 @@ void calculateObjectNameLength_firesAlert_WhenFileLengthIsGreaterThanThreshold() List userRoles = Collections.singletonList( - new UserRole().role(WorkspaceAccessLevel.OWNER).email(USER_EMAIL)); + new UserRole().role(WorkspaceAccessLevel.OWNER).email(USERNAME).userName(USERNAME)); doReturn(userRoles).when(workspaceService).getFirecloudUserRoles(NAMESPACE, FIRECLOUD_NAME); - DbUser aUser = new DbUser().setUsername(USER_EMAIL); + DbUser aUser = new DbUser().setUsername(USERNAME); Set dbUsers = Sets.newHashSet(aUser); doReturn(dbUsers) .when(userService) - .findActiveUsersByUsernames(Collections.singletonList(USER_EMAIL)); + .findActiveUsersByUsernames(Collections.singletonList(USERNAME)); doReturn(Optional.of(PET_ACCOUNT)) .when(iamService) @@ -151,18 +151,21 @@ void calculateObjectNameLength_firesAlertsForMultipleUser_WhenFileLengthIsGreate List userRoles = Lists.newArrayList( - new UserRole().role(WorkspaceAccessLevel.OWNER).email(USER_EMAIL), - new UserRole().role(WorkspaceAccessLevel.WRITER).email("1" + USER_EMAIL)); + new UserRole().role(WorkspaceAccessLevel.OWNER).userName(USERNAME).email(USERNAME), + new UserRole() + .role(WorkspaceAccessLevel.WRITER) + .userName("1" + USERNAME) + .email("1" + USERNAME)); doReturn(userRoles).when(workspaceService).getFirecloudUserRoles(NAMESPACE, FIRECLOUD_NAME); - DbUser aUser = new DbUser().setUsername(USER_EMAIL); - DbUser anotherUser = new DbUser().setUsername("1" + USER_EMAIL); + DbUser aUser = new DbUser().setUsername(USERNAME); + DbUser anotherUser = new DbUser().setUsername("1" + USERNAME); Set dbUsers = Sets.newHashSet(aUser, anotherUser); doReturn(dbUsers) .when(userService) - .findActiveUsersByUsernames(Lists.newArrayList(USER_EMAIL, "1" + USER_EMAIL)); + .findActiveUsersByUsernames(Lists.newArrayList(USERNAME, "1" + USERNAME)); doReturn(Optional.of(PET_ACCOUNT)) .when(iamService) diff --git a/api/src/test/java/org/pmiops/workbench/utils/TestMockFactory.java b/api/src/test/java/org/pmiops/workbench/utils/TestMockFactory.java index 08cdb42326e..be0ccbcb2c3 100644 --- a/api/src/test/java/org/pmiops/workbench/utils/TestMockFactory.java +++ b/api/src/test/java/org/pmiops/workbench/utils/TestMockFactory.java @@ -162,11 +162,11 @@ public static Workspace createWorkspace( } public static RawlsWorkspaceDetails createTerraWorkspace( - String namespace, String terraName, String creator) { + String namespace, String terraName, String creatorUsername) { return new RawlsWorkspaceDetails() .namespace(namespace) .name(terraName) - .createdBy(creator) + .createdBy(creatorUsername) .workspaceId(WORKSPACE_TERRA_UUID) .bucketName(WORKSPACE_BUCKET_NAME) .googleProject(DEFAULT_GOOGLE_PROJECT); From 375844862c6a68f256baf8632e196a9add9fc1d3 Mon Sep 17 00:00:00 2001 From: Joel Thibault Date: Mon, 9 Dec 2024 14:47:59 -0500 Subject: [PATCH 6/8] rename and add comments to mapper --- .../java/org/pmiops/workbench/utils/mappers/UserMapper.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/api/src/main/java/org/pmiops/workbench/utils/mappers/UserMapper.java b/api/src/main/java/org/pmiops/workbench/utils/mappers/UserMapper.java index b30ddfa6a4a..4337e1d3b7c 100644 --- a/api/src/main/java/org/pmiops/workbench/utils/mappers/UserMapper.java +++ b/api/src/main/java/org/pmiops/workbench/utils/mappers/UserMapper.java @@ -12,10 +12,12 @@ config = MapStructConfig.class, uses = {CommonMappers.class, FirecloudMapper.class}) public interface UserMapper { + // used internally by the generated implementation of toWorkspaceUserAdminView() @Mapping(target = "userName", source = "username") - User toApiUser(DbUser user); + User mapperInternalToUser(DbUser user); - User toUser(UserRole userRole); + // used internally by the generated implementation of toPartialWorkspaceUserAdminView() + User mapperInternalToUser(UserRole userRole); @Mapping(source = "acl", target = "role") @Mapping(source = "user.username", target = "email") From c9e5c74aa9c4debfc2fdfc7a8f0288abb86dca6c Mon Sep 17 00:00:00 2001 From: Joel Thibault Date: Mon, 9 Dec 2024 15:04:48 -0500 Subject: [PATCH 7/8] test cleanup --- .../auditors/EgressEventAuditorTest.java | 12 ++--- .../api/WorkspaceAdminControllerTest.java | 5 +- .../api/WorkspacesControllerTest.java | 54 +++++-------------- .../ObjectNameLengthServiceTest.java | 9 ++-- 4 files changed, 24 insertions(+), 56 deletions(-) diff --git a/api/src/test/java/org/pmiops/workbench/actionaudit/auditors/EgressEventAuditorTest.java b/api/src/test/java/org/pmiops/workbench/actionaudit/auditors/EgressEventAuditorTest.java index a06e90c65e3..321f6379e32 100644 --- a/api/src/test/java/org/pmiops/workbench/actionaudit/auditors/EgressEventAuditorTest.java +++ b/api/src/test/java/org/pmiops/workbench/actionaudit/auditors/EgressEventAuditorTest.java @@ -51,7 +51,7 @@ public class EgressEventAuditorTest { private static final long USER_ID = 1L; - private static final String USER_EMAIL = "user@researchallofus.org"; + private static final String USERNAME = "user@researchallofus.org"; private static final long WORKSPACE_ID = 1L; private static final String WORKSPACE_NAMESPACE = "aou-rw-test-c7dec260"; private static final String GOOGLE_PROJECT = "gcp-project-id"; @@ -89,8 +89,8 @@ static class Configuration {} public void setUp() { dbUser = new DbUser(); dbUser.setUserId(USER_ID); - dbUser.setUsername(USER_EMAIL); - when(mockUserDao.findUserByUsername(USER_EMAIL)).thenReturn(dbUser); + dbUser.setUsername(USERNAME); + when(mockUserDao.findUserByUsername(USERNAME)).thenReturn(dbUser); dbWorkspace = new DbWorkspace(); dbWorkspace.setWorkspaceId(WORKSPACE_ID); @@ -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().userName(USER_EMAIL).email(USER_EMAIL)); + firecloudUserRoles.add(new UserRole().userName(USERNAME)); when(mockWorkspaceService.getFirecloudUserRoles(WORKSPACE_NAMESPACE, WORKSPACE_FIRECLOUD_NAME)) .thenReturn(firecloudUserRoles); } @@ -125,7 +125,7 @@ public void testFireEgressEvent() { assertThat(events.stream().map(event -> event.agentIdMaybe()).collect(Collectors.toSet())) .containsExactly(USER_ID); assertThat(events.stream().map(event -> event.agentEmailMaybe()).collect(Collectors.toSet())) - .containsExactly(USER_EMAIL); + .containsExactly(USERNAME); assertThat(events.stream().map(event -> event.targetIdMaybe()).collect(Collectors.toSet())) .containsExactly(WORKSPACE_ID); @@ -173,7 +173,7 @@ public void testFireEgressEventForUser() { assertThat(events.stream().map(event -> event.agentIdMaybe()).collect(Collectors.toSet())) .containsExactly(USER_ID); assertThat(events.stream().map(event -> event.agentEmailMaybe()).collect(Collectors.toSet())) - .containsExactly(USER_EMAIL); + .containsExactly(USERNAME); assertThat(events.stream().map(event -> event.targetIdMaybe()).collect(Collectors.toSet())) .containsExactly(WORKSPACE_ID); diff --git a/api/src/test/java/org/pmiops/workbench/api/WorkspaceAdminControllerTest.java b/api/src/test/java/org/pmiops/workbench/api/WorkspaceAdminControllerTest.java index 3e42699796d..a124fea2913 100644 --- a/api/src/test/java/org/pmiops/workbench/api/WorkspaceAdminControllerTest.java +++ b/api/src/test/java/org/pmiops/workbench/api/WorkspaceAdminControllerTest.java @@ -107,10 +107,7 @@ public void setUp() { .thenReturn(Optional.of(dbWorkspace)); final UserRole collaborator = - new UserRole() - .userName("test@test.test") - .email("test@test.test") - .role(WorkspaceAccessLevel.WRITER); + new UserRole().userName("test@test.test").role(WorkspaceAccessLevel.WRITER); final List collaborators = List.of(collaborator); when(mockWorkspaceService.getFirecloudUserRoles(WORKSPACE_NAMESPACE, WORKSPACE_TERRA_NAME)) .thenReturn(collaborators); diff --git a/api/src/test/java/org/pmiops/workbench/api/WorkspacesControllerTest.java b/api/src/test/java/org/pmiops/workbench/api/WorkspacesControllerTest.java index 4adfd387a30..c266c71e70a 100644 --- a/api/src/test/java/org/pmiops/workbench/api/WorkspacesControllerTest.java +++ b/api/src/test/java/org/pmiops/workbench/api/WorkspacesControllerTest.java @@ -2,6 +2,7 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertWithMessage; +import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.fail; import static org.mockito.ArgumentMatchers.any; @@ -1526,7 +1527,6 @@ private void addUserRoleToShareWorkspaceRequest( WorkspaceAccessLevel workspaceAccessLevel) { final UserRole userRole = new UserRole(); userRole.setUserName(userName); - userRole.setEmail(userName); userRole.setRole(workspaceAccessLevel); shareWorkspaceRequest.addItemsItem(userRole); } @@ -2369,25 +2369,12 @@ public void testCloneWorkspaceIncludeUserRoles() { List expectedCollaboratorsAfterUpdate = convertUserRolesToUpdateAclRequestList( Set.of( - new UserRole() - .userName(cloner.getUsername()) - .email(cloner.getUsername()) - .role(WorkspaceAccessLevel.OWNER), - new UserRole() - .userName(LOGGED_IN_USERNAME) - .email(LOGGED_IN_USERNAME) - .role(WorkspaceAccessLevel.OWNER), - new UserRole() - .userName(reader.getUsername()) - .email(reader.getUsername()) - .role(WorkspaceAccessLevel.READER), - new UserRole() - .userName(writer.getUsername()) - .email(writer.getUsername()) - .role(WorkspaceAccessLevel.WRITER), + new UserRole().userName(cloner.getUsername()).role(WorkspaceAccessLevel.OWNER), + new UserRole().userName(LOGGED_IN_USERNAME).role(WorkspaceAccessLevel.OWNER), + new UserRole().userName(reader.getUsername()).role(WorkspaceAccessLevel.READER), + new UserRole().userName(writer.getUsername()).role(WorkspaceAccessLevel.WRITER), new UserRole() .userName(workspaceService.getPublishedWorkspacesGroupEmail()) - .email(workspaceService.getPublishedWorkspacesGroupEmail()) .role(WorkspaceAccessLevel.NO_ACCESS))); currentUser = cloner; @@ -2546,15 +2533,9 @@ public void testShareWorkspacePatch_AddBillingProjectUser() { new ShareWorkspaceRequest() .workspaceEtag(workspace.getEtag()) .addItemsItem( - new UserRole() - .userName(writerUser.getUsername()) - .email(writerUser.getUsername()) - .role(WorkspaceAccessLevel.WRITER)) + new UserRole().userName(writerUser.getUsername()).role(WorkspaceAccessLevel.WRITER)) .addItemsItem( - new UserRole() - .userName(ownerUser.getUsername()) - .email(ownerUser.getUsername()) - .role(WorkspaceAccessLevel.OWNER)); + new UserRole().userName(ownerUser.getUsername()).role(WorkspaceAccessLevel.OWNER)); stubFcUpdateWorkspaceACL(); workspacesController.shareWorkspacePatch( @@ -2607,13 +2588,9 @@ public void testShareWorkspacePatch_removeBillingProjectUser() { .addItemsItem( new UserRole() .userName(writerUser.getUsername()) - .email(writerUser.getUsername()) .role(WorkspaceAccessLevel.NO_ACCESS)) .addItemsItem( - new UserRole() - .userName(ownerUser.getUsername()) - .email(ownerUser.getUsername()) - .role(WorkspaceAccessLevel.READER)); + new UserRole().userName(ownerUser.getUsername()).role(WorkspaceAccessLevel.READER)); stubFcUpdateWorkspaceACL(); workspacesController.shareWorkspacePatch( @@ -2670,7 +2647,6 @@ public void testShareWorkspacePatch_publishedWorkspace() { .addItemsItem( new UserRole() .userName(writerUser.getUsername()) - .email(writerUser.getUsername()) .role(WorkspaceAccessLevel.NO_ACCESS)); stubFcUpdateWorkspaceACL(); @@ -2699,7 +2675,6 @@ public void testSharePatch_workspaceNoRoleFailure() { shareWorkspaceRequest, LOGGED_IN_USERNAME, WorkspaceAccessLevel.OWNER); UserRole writer = new UserRole(); writer.setUserName("writerfriend@gmail.com"); - writer.setEmail("writerfriend@gmail.com"); shareWorkspaceRequest.addItemsItem(writer); // Simulate time between API calls to trigger last-modified/@Version changes. @@ -2752,7 +2727,6 @@ public void testUnshareWorkspace() { shareWorkspaceRequest.setWorkspaceEtag(workspace.getEtag()); UserRole reader = new UserRole(); reader.setUserName(readerUser.getUsername()); - reader.setEmail(readerUser.getUsername()); reader.setRole(WorkspaceAccessLevel.NO_ACCESS); shareWorkspaceRequest.addItemsItem(reader); @@ -2844,12 +2818,12 @@ public void testGetFirecloudWorkspaceUserRoles() { .getFirecloudWorkspaceUserRoles(workspace.getNamespace(), workspace.getTerraName()) .getBody(); - assertThat(resp.getItems()) - .containsExactly( - new UserRole() - .userName(currentUser.getUsername()) - .email(currentUser.getUsername()) - .role(WorkspaceAccessLevel.OWNER)); + assertNotNull(resp); + List roles = resp.getItems(); + assertThat(roles).hasSize(1); + UserRole role = roles.get(0); + assertThat(role.getUserName()).isEqualTo(currentUser.getUsername()); + assertThat(role.getRole()).isEqualTo(WorkspaceAccessLevel.OWNER); } @Test diff --git a/api/src/test/java/org/pmiops/workbench/exfiltration/ObjectNameLengthServiceTest.java b/api/src/test/java/org/pmiops/workbench/exfiltration/ObjectNameLengthServiceTest.java index b94530a7248..aff635bef9f 100644 --- a/api/src/test/java/org/pmiops/workbench/exfiltration/ObjectNameLengthServiceTest.java +++ b/api/src/test/java/org/pmiops/workbench/exfiltration/ObjectNameLengthServiceTest.java @@ -97,7 +97,7 @@ void calculateObjectNameLength_firesAlert_WhenFileLengthIsGreaterThanThreshold() List userRoles = Collections.singletonList( - new UserRole().role(WorkspaceAccessLevel.OWNER).email(USERNAME).userName(USERNAME)); + new UserRole().role(WorkspaceAccessLevel.OWNER).userName(USERNAME)); doReturn(userRoles).when(workspaceService).getFirecloudUserRoles(NAMESPACE, FIRECLOUD_NAME); DbUser aUser = new DbUser().setUsername(USERNAME); @@ -151,11 +151,8 @@ void calculateObjectNameLength_firesAlertsForMultipleUser_WhenFileLengthIsGreate List userRoles = Lists.newArrayList( - new UserRole().role(WorkspaceAccessLevel.OWNER).userName(USERNAME).email(USERNAME), - new UserRole() - .role(WorkspaceAccessLevel.WRITER) - .userName("1" + USERNAME) - .email("1" + USERNAME)); + new UserRole().role(WorkspaceAccessLevel.OWNER).userName(USERNAME), + new UserRole().role(WorkspaceAccessLevel.WRITER).userName("1" + USERNAME)); doReturn(userRoles).when(workspaceService).getFirecloudUserRoles(NAMESPACE, FIRECLOUD_NAME); DbUser aUser = new DbUser().setUsername(USERNAME); From da86397b511a2dca628e7617d21cce2e1f2fadb8 Mon Sep 17 00:00:00 2001 From: Joel Thibault Date: Mon, 9 Dec 2024 15:09:40 -0500 Subject: [PATCH 8/8] rm line --- .../org/pmiops/workbench/api/WorkspacesControllerTest.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/api/src/test/java/org/pmiops/workbench/api/WorkspacesControllerTest.java b/api/src/test/java/org/pmiops/workbench/api/WorkspacesControllerTest.java index c266c71e70a..2b2e1824305 100644 --- a/api/src/test/java/org/pmiops/workbench/api/WorkspacesControllerTest.java +++ b/api/src/test/java/org/pmiops/workbench/api/WorkspacesControllerTest.java @@ -2819,9 +2819,8 @@ public void testGetFirecloudWorkspaceUserRoles() { .getBody(); assertNotNull(resp); - List roles = resp.getItems(); - assertThat(roles).hasSize(1); - UserRole role = roles.get(0); + assertThat(resp.getItems()).hasSize(1); + UserRole role = resp.getItems().get(0); assertThat(role.getUserName()).isEqualTo(currentUser.getUsername()); assertThat(role.getRole()).isEqualTo(WorkspaceAccessLevel.OWNER); }