Added API routes to create and delete users - #294
oliverbates94 wants to merge 1 commit into
Conversation
create and delete users
tchapi
left a comment
tchapi
left a comment
There was a problem hiding this comment.
Thanks for the work. I'm overall ok for these two new APIs, but we need to make them right 🙏🏼
Apart from the in-line comments above:
-
Most lines are copy-pasted from
Admin\UserController::userCreate/userDelete.Utils::createPasswordlessUserWithDefaultObjects()already exists and does 90% of the create path; I think the right move is to extendUtils(e.g.createUserWithDefaultObjects(username, displayName, email, password, isAdmin)and adeleteUserAndObjects(User)or something like that) and call it from both controllers. -
No email validation. The admin path gets EmailType + Assert\Email on
Principal::$emailvia the form validator but the API path never runs the validator, so any string is stored as an email (which then ends up in mailto: share hrefs). -
I'll need some functional test added under
tests/Functional+ update of docs/api
| $userIsAdmin = $data['is_admin'] ?? null; | ||
| if (empty($userIsAdmin) || !in_array($userIsAdmin, [true, false, 'true', 'false'], true)) { | ||
| return $this->json(['status' => 'error', 'message' => 'Invalid User Is Admin', 'timestamp' => $this->getTimestamp()], 400); | ||
| } |
There was a problem hiding this comment.
this is broken -> "everyone becomes admin".
"is_admin": false→ empty(false) is true → rejected with 400"is_admin": "false"→ passesin_array, thensetIsAdmin(bool $isAdmin)receives the string 'false'. The file has no declare(strict_types=1), so PHP coerces it totrue(only "" and "0" are falsy strings).
Result: there is no input that creates a non-admin user; "false" silently creates an admin. Look at the existing endpoints
| $entityManager = $doctrine->getManager(); | ||
| $entityManager->remove($user); | ||
|
|
||
| $principal = $doctrine->getRepository(Principal::class)->findOneByUri($user->getPrincipalUri()); |
There was a problem hiding this comment.
findOneByUri() can return null (orphaned user, or a DB where the principal was removed), then $principal->getUri() throws an \Error, which catch (\Exception) does not catch → unhandled 500.
| if (empty($userPassword)) { | ||
| return $this->json(['status' => 'error', 'message' => 'Invalid User Password', 'timestamp' => $this->getTimestamp()], 400); | ||
| } |
There was a problem hiding this comment.
empty($userPassword) / empty($userName) rejects the literal "0"
| 'timestamp' => $this->getTimestamp(), | ||
| ]; | ||
|
|
||
| return $this->json($response, 200); |
There was a problem hiding this comment.
nit: 201 would be the right status for create
| * @return JsonResponse A JSON response indicating the success or failure of the operation | ||
| */ | ||
| #[Route('/users/{userId}', name: 'user_delete', methods: ['DELETE'], requirements: ['userId' => '\d+'])] | ||
| public function deleteUser(Request $request, int $userId, ManagerRegistry $doctrine): JsonResponse |
| return $this->json(['status' => 'error', 'message' => 'Invalid User Name', 'timestamp' => $this->getTimestamp()], 400); | ||
| } | ||
| $userDisplayName = $data['display_name'] ?? null; | ||
| if (empty($userDisplayName) || 1 !== preg_match('/^[a-zA-Z0-9 ._-]{1,64}$/', $userDisplayName)) { |
There was a problem hiding this comment.
I don't know where this display-name regex ^[a-zA-Z0-9 ._-]{1,64}$ comes from, it's nowhere else in the codebase and is stricter than the UI: "Éric Dupont" or "José" is rejected by the API but accepted by the form
2 participants
Hi! I'm not sure if this is the right approach to this, I basically duplicated the code from the UserController, but I needed these two API routes
Tell me if there's something I need to change!