From f649d960c1a4c4ee33970e8b14741934d4065246 Mon Sep 17 00:00:00 2001 From: Pavlo Tkach <3469726+ptkach@users.noreply.github.com> Date: Fri, 13 Dec 2024 14:47:21 -0500 Subject: [PATCH] Add user email prefix to the console user create (#2623) --- ...ponent.html => userDetails.component.html} | 31 +---- ...ponent.scss => userDetails.component.scss} | 0 ....component.ts => userDetails.component.ts} | 38 +++--- .../src/app/users/userEditForm.component.html | 39 ++++++ .../src/app/users/userEditForm.component.scss | 0 .../src/app/users/userEditForm.component.ts | 58 ++++++++ .../src/app/users/users.component.html | 19 ++- .../src/app/users/users.component.ts | 11 +- console-webapp/src/app/users/users.service.ts | 4 +- .../ui/server/console/ConsoleUsersAction.java | 129 ++++++++---------- .../console/ConsoleUsersActionTest.java | 25 +++- 11 files changed, 224 insertions(+), 130 deletions(-) rename console-webapp/src/app/users/{userEdit.component.html => userDetails.component.html} (78%) rename console-webapp/src/app/users/{userEdit.component.scss => userDetails.component.scss} (100%) rename console-webapp/src/app/users/{userEdit.component.ts => userDetails.component.ts} (79%) create mode 100644 console-webapp/src/app/users/userEditForm.component.html create mode 100644 console-webapp/src/app/users/userEditForm.component.scss create mode 100644 console-webapp/src/app/users/userEditForm.component.ts diff --git a/console-webapp/src/app/users/userEdit.component.html b/console-webapp/src/app/users/userDetails.component.html similarity index 78% rename from console-webapp/src/app/users/userEdit.component.html rename to console-webapp/src/app/users/userDetails.component.html index 508609a8d35..ec317972a96 100644 --- a/console-webapp/src/app/users/userEdit.component.html +++ b/console-webapp/src/app/users/userDetails.component.html @@ -11,31 +11,10 @@

Editing {{ userDetails().emailAddress }}

arrow_back -
-

- - User Role: - help_outline - - Editor - Viewer - - -

- -
+ } @else { @if(isNewUser) {

{{ userDetails().emailAddress + " successfully created" }} @@ -53,7 +32,7 @@

User details

mat-flat-button color="primary" aria-label="Edit User" - (click)="userRole = userDetails().role; isEditing = true" + (click)="isEditing = true" > edit Edit diff --git a/console-webapp/src/app/users/userEdit.component.scss b/console-webapp/src/app/users/userDetails.component.scss similarity index 100% rename from console-webapp/src/app/users/userEdit.component.scss rename to console-webapp/src/app/users/userDetails.component.scss diff --git a/console-webapp/src/app/users/userEdit.component.ts b/console-webapp/src/app/users/userDetails.component.ts similarity index 79% rename from console-webapp/src/app/users/userEdit.component.ts rename to console-webapp/src/app/users/userDetails.component.ts index 324c89f4ad0..3dc2ede75b1 100644 --- a/console-webapp/src/app/users/userEdit.component.ts +++ b/console-webapp/src/app/users/userDetails.component.ts @@ -19,13 +19,14 @@ import { SelectedRegistrarModule } from '../app.module'; import { MaterialModule } from '../material.module'; import { RegistrarService } from '../registrar/registrar.service'; import { SnackBarModule } from '../snackbar.module'; -import { UsersService, roleToDescription } from './users.service'; +import { UsersService, roleToDescription, User } from './users.service'; import { FormsModule } from '@angular/forms'; +import { UserEditFormComponent } from './userEditForm.component'; @Component({ selector: 'app-user-edit', - templateUrl: './userEdit.component.html', - styleUrls: ['./userEdit.component.scss'], + templateUrl: './userDetails.component.html', + styleUrls: ['./userDetails.component.scss'], standalone: true, imports: [ FormsModule, @@ -33,15 +34,15 @@ import { FormsModule } from '@angular/forms'; SnackBarModule, CommonModule, SelectedRegistrarModule, + UserEditFormComponent, ], providers: [], }) -export class UserEditComponent { +export class UserDetailsComponent { isEditing = false; isPasswordVisible = false; isNewUser = false; isLoading = false; - userRole = ''; userDetails = computed(() => { return this.usersService @@ -84,22 +85,17 @@ export class UserEditComponent { this.usersService.currentlyOpenUserEmail.set(''); } - saveEdit() { + saveEdit(user: User) { this.isLoading = true; - this.usersService - .updateUser({ - role: this.userRole, - emailAddress: this.userDetails().emailAddress, - }) - .subscribe({ - error: (err) => { - this._snackBar.open(err.error || err.message); - this.isLoading = false; - }, - complete: () => { - this.isLoading = false; - this.isEditing = false; - }, - }); + this.usersService.updateUser(user).subscribe({ + error: (err) => { + this._snackBar.open(err.error || err.message); + this.isLoading = false; + }, + complete: () => { + this.isLoading = false; + this.isEditing = false; + }, + }); } } diff --git a/console-webapp/src/app/users/userEditForm.component.html b/console-webapp/src/app/users/userEditForm.component.html new file mode 100644 index 00000000000..143c1d4d992 --- /dev/null +++ b/console-webapp/src/app/users/userEditForm.component.html @@ -0,0 +1,39 @@ +
+

+ + User name prefix: + help_outline + + +

+

+ + User Role: + help_outline + + Editor + Viewer + + +

+ +
diff --git a/console-webapp/src/app/users/userEditForm.component.scss b/console-webapp/src/app/users/userEditForm.component.scss new file mode 100644 index 00000000000..e69de29bb2d diff --git a/console-webapp/src/app/users/userEditForm.component.ts b/console-webapp/src/app/users/userEditForm.component.ts new file mode 100644 index 00000000000..0c6ba9791e3 --- /dev/null +++ b/console-webapp/src/app/users/userEditForm.component.ts @@ -0,0 +1,58 @@ +// Copyright 2024 The Nomulus Authors. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import { CommonModule } from '@angular/common'; +import { + Component, + ElementRef, + EventEmitter, + input, + Output, + ViewChild, +} from '@angular/core'; +import { MaterialModule } from '../material.module'; +import { FormsModule } from '@angular/forms'; +import { User } from './users.service'; + +@Component({ + selector: 'app-user-edit-form', + templateUrl: './userEditForm.component.html', + styleUrls: ['./userEditForm.component.scss'], + standalone: true, + imports: [FormsModule, MaterialModule, CommonModule], + providers: [], +}) +export class UserEditFormComponent { + @ViewChild('form') form!: ElementRef; + isNew = input(false); + user = input( + { + emailAddress: '', + role: 'ACCOUNT_MANAGER', + }, + // @ts-ignore - legit option, typescript fails to match it to a proper type + { transform: (user: User) => structuredClone(user) } + ); + + @Output() onEditComplete = new EventEmitter(); + + saveEdit(e: SubmitEvent) { + e.preventDefault(); + if (this.form.nativeElement.checkValidity()) { + this.onEditComplete.emit(this.user()); + } else { + this.form.nativeElement.reportValidity(); + } + } +} diff --git a/console-webapp/src/app/users/users.component.html b/console-webapp/src/app/users/users.component.html index 44a5ea31e17..790f764dc01 100644 --- a/console-webapp/src/app/users/users.component.html +++ b/console-webapp/src/app/users/users.component.html @@ -4,10 +4,8 @@ } @else if(selectingExistingUser) { -

Add existing user

-

} @else if(usersService.currentlyOpenUserEmail()) { + } @else if(isNew) { +

New User Form

+
+

+ +

+ } @else {
@@ -79,11 +90,11 @@

Users

diff --git a/console-webapp/src/app/users/users.component.ts b/console-webapp/src/app/users/users.component.ts index 6623f66e02c..6affd7d1c66 100644 --- a/console-webapp/src/app/users/users.component.ts +++ b/console-webapp/src/app/users/users.component.ts @@ -20,12 +20,13 @@ import { SelectedRegistrarModule } from '../app.module'; import { MaterialModule } from '../material.module'; import { RegistrarService } from '../registrar/registrar.service'; import { SnackBarModule } from '../snackbar.module'; -import { UserEditComponent } from './userEdit.component'; +import { UserDetailsComponent } from './userDetails.component'; import { User, UsersService } from './users.service'; import { UserDataService } from '../shared/services/userData.service'; import { FormsModule } from '@angular/forms'; import { UsersListComponent } from './usersList.component'; import { MatSelectChange } from '@angular/material/select'; +import { UserEditFormComponent } from './userEditForm.component'; @Component({ selector: 'app-users', @@ -39,12 +40,14 @@ import { MatSelectChange } from '@angular/material/select'; CommonModule, SelectedRegistrarModule, UsersListComponent, - UserEditComponent, + UserEditFormComponent, + UserDetailsComponent, ], providers: [UsersService], }) export class UsersComponent { isLoading = false; + isNew = false; selectingExistingUser = false; selectedRegistrarId = ''; usersSelection: User[] = []; @@ -87,9 +90,9 @@ export class UsersComponent { }); } - createNewUser() { + createNewUser(user: User) { this.isLoading = true; - this.usersService.createOrAddNewUser(null).subscribe({ + this.usersService.createOrAddNewUser(user).subscribe({ error: (err: HttpErrorResponse) => { this._snackBar.open(err.error || err.message); this.isLoading = false; diff --git a/console-webapp/src/app/users/users.service.ts b/console-webapp/src/app/users/users.service.ts index 0374f101a00..ed5c9ad482c 100644 --- a/console-webapp/src/app/users/users.service.ts +++ b/console-webapp/src/app/users/users.service.ts @@ -60,9 +60,9 @@ export class UsersService { ); } - createOrAddNewUser(maybeExistingUser: User | null) { + createOrAddNewUser(user: User) { return this.backendService - .createUser(this.registrarService.registrarId(), maybeExistingUser) + .createUser(this.registrarService.registrarId(), user) .pipe( tap((newUser: User) => { if (newUser) { diff --git a/core/src/main/java/google/registry/ui/server/console/ConsoleUsersAction.java b/core/src/main/java/google/registry/ui/server/console/ConsoleUsersAction.java index 03aa3c4349c..e175862b829 100644 --- a/core/src/main/java/google/registry/ui/server/console/ConsoleUsersAction.java +++ b/core/src/main/java/google/registry/ui/server/console/ConsoleUsersAction.java @@ -29,7 +29,6 @@ import com.google.api.services.directory.Directory; import com.google.api.services.directory.model.UserName; -import com.google.common.base.Splitter; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.gson.annotations.Expose; @@ -52,7 +51,6 @@ import java.util.Objects; import java.util.Optional; import java.util.stream.Collectors; -import java.util.stream.IntStream; import javax.annotation.Nullable; import javax.inject.Inject; import javax.inject.Named; @@ -67,7 +65,6 @@ public class ConsoleUsersAction extends ConsoleApiAction { static final String PATH = "/console-api/users"; private static final int PASSWORD_LENGTH = 16; - private static final Splitter EMAIL_SPLITTER = Splitter.on('@').trimResults(); private final String registrarId; private final Directory directory; @@ -102,12 +99,7 @@ protected void postHandler(User user) { // Temporary flag while testing if (user.getUserRoles().isAdmin()) { checkPermission(user, registrarId, ConsolePermission.MANAGE_USERS); - if (userData.isPresent()) { // Adding existing user to registrar - tm().transact(this::runAppendUserInTransaction); - } else { // Adding new user to registrar - tm().transact(this::runCreateInTransaction); - } - + tm().transact(this::runPostInTransaction); } else { consoleApiParams.response().setStatus(SC_FORBIDDEN); } @@ -152,10 +144,16 @@ protected void deleteHandler(User user) { } } - private void runAppendUserInTransaction() { - if (!isModifyingRequestValid(false)) { - return; + private void runPostInTransaction() throws IOException { + validateRequestParams(); + if (!tm().exists(VKey.create(User.class, this.userData.get().emailAddress))) { + this.runCreate(); + } else { + this.runAppendUserToExistingRegistrar(); } + } + + private void runAppendUserToExistingRegistrar() { ImmutableList allRegistrarUsers = getAllRegistrarUsers(registrarId); if (allRegistrarUsers.size() >= 4) { throw new BadRequestException("Total users amount per registrar is limited to 4"); @@ -164,20 +162,17 @@ private void runAppendUserInTransaction() { updateUserRegistrarRoles( this.userData.get().emailAddress, registrarId, - RegistrarRole.valueOf(this.userData.get().role), - false); + RegistrarRole.valueOf(this.userData.get().role)); consoleApiParams.response().setStatus(SC_OK); } private void runDeleteInTransaction() throws IOException { - if (!isModifyingRequestValid(true)) { + if (!isModifyingRequestValid()) { return; } String email = this.userData.get().emailAddress; - User updatedUser = - updateUserRegistrarRoles( - email, registrarId, RegistrarRole.valueOf(this.userData.get().role), true); + User updatedUser = updateUserRegistrarRoles(email, registrarId, null); // User has no registrars assigned if (updatedUser.getUserRoles().getRegistrarRoles().size() == 0) { @@ -193,34 +188,33 @@ private void runDeleteInTransaction() throws IOException { User.revokeIapPermission(email, maybeGroupEmailAddress, cloudTasksUtils, null, iamClient); } - consoleApiParams.response().setStatus(SC_OK); } - private void runCreateInTransaction() throws IOException { + private void runCreate() throws IOException { ImmutableList allRegistrarUsers = getAllRegistrarUsers(registrarId); if (allRegistrarUsers.size() >= 4) { throw new BadRequestException("Total users amount per registrar is limited to 4"); } - String nextAvailableEmail = - IntStream.range(1, 5) - .mapToObj(i -> String.format("%s-user%s@%s", registrarId, i, gSuiteDomainName)) - .filter(email -> tm().loadByKeyIfPresent(VKey.create(User.class, email)).isEmpty()) - .findFirst() - // Can only happen if registrar cycled through 20 users, which is unlikely - .orElseThrow( - () -> new BadRequestException("Failed to find available increment for new user")); + String newEmailPrefix = userData.get().emailAddress.trim(); + + if (!newEmailPrefix.matches("^[a-zA-Z0-9]{3}$")) { + throw new BadRequestException("Email prefix is invalid"); + } + + String newEmail = String.format("%s.%s@%s", newEmailPrefix, registrarId, gSuiteDomainName); + if (tm().loadByKeyIfPresent(VKey.create(User.class, newEmail)).isPresent()) { + throw new BadRequestException("Email prefix is not available"); + } com.google.api.services.directory.model.User newUser = new com.google.api.services.directory.model.User(); newUser.setName( - new UserName() - .setFamilyName(registrarId) - .setGivenName(EMAIL_SPLITTER.splitToList(nextAvailableEmail).get(0))); + new UserName().setFamilyName(registrarId).setGivenName(newEmailPrefix + "." + registrarId)); newUser.setPassword(passwordGenerator.createString(PASSWORD_LENGTH)); - newUser.setPrimaryEmail(nextAvailableEmail); + newUser.setPrimaryEmail(newEmail); try { directory.users().insert(newUser).execute(); @@ -234,11 +228,9 @@ private void runCreateInTransaction() throws IOException { .setRegistrarRoles(ImmutableMap.of(registrarId, ACCOUNT_MANAGER)) .build(); - User.Builder builder = - new User.Builder().setUserRoles(userRoles).setEmailAddress(newUser.getPrimaryEmail()); + User.Builder builder = new User.Builder().setUserRoles(userRoles).setEmailAddress(newEmail); tm().put(builder.build()); - User.grantIapPermission( - nextAvailableEmail, maybeGroupEmailAddress, cloudTasksUtils, null, iamClient); + User.grantIapPermission(newEmail, maybeGroupEmailAddress, cloudTasksUtils, null, iamClient); consoleApiParams.response().setStatus(SC_CREATED); consoleApiParams @@ -246,72 +238,69 @@ private void runCreateInTransaction() throws IOException { .setPayload( consoleApiParams .gson() - .toJson( - new UserData( - newUser.getPrimaryEmail(), - ACCOUNT_MANAGER.toString(), - newUser.getPassword()))); + .toJson(new UserData(newEmail, ACCOUNT_MANAGER.toString(), newUser.getPassword()))); } private void runUpdateInTransaction() { - if (!isModifyingRequestValid(true)) { + if (!isModifyingRequestValid()) { return; } updateUserRegistrarRoles( this.userData.get().emailAddress, registrarId, - RegistrarRole.valueOf(this.userData.get().role), - false); + RegistrarRole.valueOf(this.userData.get().role)); consoleApiParams.response().setStatus(SC_OK); } - private boolean isModifyingRequestValid(boolean verifyAccess) { + private boolean isModifyingRequestValid() { + validateRequestParams(); + User userToUpdate = verifyUserExists(userData.get().emailAddress); + return validateUserRegistrarAssociation(userToUpdate); + } + + private void validateRequestParams() { if (userData.isEmpty() || isNullOrEmpty(userData.get().emailAddress) || isNullOrEmpty(userData.get().role)) { throw new BadRequestException("User data is missing or incomplete"); } - String email = userData.get().emailAddress; - User userToUpdate = - tm().loadByKeyIfPresent(VKey.create(User.class, email)) - .orElseThrow( - () -> new BadRequestException(String.format("User %s doesn't exist", email))); - - if (verifyAccess && !userToUpdate.getUserRoles().getRegistrarRoles().containsKey(registrarId)) { - setFailedResponse( - String.format("Can't update user not associated with registrarId %s", registrarId), - SC_FORBIDDEN); - return false; + } + + private User verifyUserExists(String email) { + return tm().loadByKeyIfPresent(VKey.create(User.class, email)) + .orElseThrow(() -> new BadRequestException(String.format("User %s doesn't exist", email))); + } + + private boolean validateUserRegistrarAssociation(User user) { + if (user.getUserRoles().getRegistrarRoles().containsKey(registrarId)) { + return true; } - return true; + setFailedResponse( + String.format("Can't update user not associated with registrarId %s", registrarId), + SC_FORBIDDEN); + return false; } - private User updateUserRegistrarRoles( - String email, String registrarId, RegistrarRole newRole, boolean isDelete) { - User userToUpdate = tm().loadByKeyIfPresent(VKey.create(User.class, email)).get(); + private User updateUserRegistrarRoles(String email, String registrarId, RegistrarRole newRole) { Map updatedRegistrarRoles; - if (isDelete) { + User user = verifyUserExists(email); + if (newRole == null) { updatedRegistrarRoles = - userToUpdate.getUserRoles().getRegistrarRoles().entrySet().stream() + user.getUserRoles().getRegistrarRoles().entrySet().stream() .filter(entry -> !Objects.equals(entry.getKey(), registrarId)) .collect(ImmutableMap.toImmutableMap(Map.Entry::getKey, Map.Entry::getValue)); } else { updatedRegistrarRoles = ImmutableMap.builder() - .putAll(userToUpdate.getUserRoles().getRegistrarRoles()) + .putAll(user.getUserRoles().getRegistrarRoles()) .put(registrarId, newRole) .buildKeepingLast(); } var updatedUser = - userToUpdate - .asBuilder() + user.asBuilder() .setUserRoles( - userToUpdate - .getUserRoles() - .asBuilder() - .setRegistrarRoles(updatedRegistrarRoles) - .build()) + user.getUserRoles().asBuilder().setRegistrarRoles(updatedRegistrarRoles).build()) .build(); tm().put(updatedUser); return updatedUser; diff --git a/core/src/test/java/google/registry/ui/server/console/ConsoleUsersActionTest.java b/core/src/test/java/google/registry/ui/server/console/ConsoleUsersActionTest.java index dbe13c4a324..5654aa5c069 100644 --- a/core/src/test/java/google/registry/ui/server/console/ConsoleUsersActionTest.java +++ b/core/src/test/java/google/registry/ui/server/console/ConsoleUsersActionTest.java @@ -159,6 +159,24 @@ void testFailure_noPermission() throws IOException { assertThat(response.getStatus()).isEqualTo(HttpServletResponse.SC_FORBIDDEN); } + @Test + void testFailure_invalidPrefix() throws IOException { + User user = DatabaseHelper.createAdminUser("email@email.com"); + AuthResult authResult = AuthResult.createUser(user); + ConsoleUsersAction action = + createAction( + Optional.of(ConsoleApiParamsUtils.createFake(authResult)), + Optional.of("POST"), + Optional.of(new UserData("a@d", RegistrarRole.ACCOUNT_MANAGER.toString(), null))); + action.cloudTasksUtils = cloudTasksHelper.getTestCloudTasksUtils(); + when(directory.users()).thenReturn(users); + when(users.insert(any(com.google.api.services.directory.model.User.class))).thenReturn(insert); + action.run(); + var response = ((FakeResponse) consoleApiParams.response()); + assertThat(response.getStatus()).isEqualTo(SC_BAD_REQUEST); + assertThat(response.getPayload()).contains("Email prefix is invalid"); + } + @Test void testSuccess_createsUser() throws IOException { User user = DatabaseHelper.createAdminUser("email@email.com"); @@ -167,7 +185,7 @@ void testSuccess_createsUser() throws IOException { createAction( Optional.of(ConsoleApiParamsUtils.createFake(authResult)), Optional.of("POST"), - Optional.empty()); + Optional.of(new UserData("lol", RegistrarRole.ACCOUNT_MANAGER.toString(), null))); action.cloudTasksUtils = cloudTasksHelper.getTestCloudTasksUtils(); when(directory.users()).thenReturn(users); when(users.insert(any(com.google.api.services.directory.model.User.class))).thenReturn(insert); @@ -176,7 +194,7 @@ void testSuccess_createsUser() throws IOException { assertThat(response.getStatus()).isEqualTo(SC_CREATED); assertThat(response.getPayload()) .contains( - "{\"emailAddress\":\"TheRegistrar-user1@email.com\",\"role\":\"ACCOUNT_MANAGER\",\"password\":\"abcdefghijklmnop\"}"); + "{\"emailAddress\":\"lol.TheRegistrar@email.com\",\"role\":\"ACCOUNT_MANAGER\",\"password\":\"abcdefghijklmnop\"}"); } @Test @@ -319,7 +337,8 @@ void testFailure_limitedTo4UsersPerRegistrar() throws IOException { createAction( Optional.of(ConsoleApiParamsUtils.createFake(authResult)), Optional.of("POST"), - Optional.empty()); + Optional.of( + new UserData("test3@test.com", RegistrarRole.ACCOUNT_MANAGER.toString(), null))); action.cloudTasksUtils = cloudTasksHelper.getTestCloudTasksUtils(); when(directory.users()).thenReturn(users); when(users.insert(any(com.google.api.services.directory.model.User.class))).thenReturn(insert);