API: Cannot update User entity over update user API endpoint

[baierjan]

Description of the problem

I am experiencing some weird and unexpected behavior for the /api/v4/users/{id} endpoint. If I am not mistaken similarly to #2381, POST to the normal /api/v4/users endpoint is not allowed to set the external id for entity due to "reasons" and it is instead possible to use PUT for creating a specific entity (and this is working well, although the id from the path is completely ignored).

According to the docs, the PUT method for endpoint /api/v4/users/{id} should be able to also update the entity, but that is not the case; instead an error is shown when the entity is already present. This prevents updating the team_id on the User entity over the API.

Your environment

DOMjudge 9.0.0/d8c018e4c

Steps to reproduce

1. curl -X PUT /api/v4/users/whatever --json '{"id": "externalId", "name": "name", "username": "username", "roles": []}'

{"last_login_time":null,"last_api_login_time":null,"first_login_time":null,"team":null,"team_id":null,"roles":[],"type":null,"userid":84,"id":"externalId","username":"username","name":"name","email":null,"last_ip":null,"ip":null,"enabled":true}

2. curl -X PUT /api/v4/users/whatever --json '{"id": "externalId", "name": "new name", "username": "username", "roles": []}'

{"code":400,"message":"User username already exists"}

Expected behaviour

I like how the groups endpoint for creating/updating Team categories from #2383 works. It allows to create a category, update the category and when called repeatedly with the same data no error is reported, which allows to replay import scripts easily without the need to delete the data first.

curl -X PUT /api/v4/contests/demo/groups/testgroup --json '{"id": "testgroup", "name": "Test group"}'
{"hidden":false,"categoryid":17,"id":"testgroup","icpc_id":null,"name":"Test group","sortorder":0,"color":null,"allow_self_registration":false}

curl -X PUT /api/v4/contests/demo/groups/testgroup --json '{"id": "testgroup", "name": "Test group"}'
{"hidden":false,"categoryid":17,"id":"testgroup","icpc_id":null,"name":"Test group","sortorder":0,"color":null,"allow_self_registration":false}

curl -X PUT /api/v4/contests/demo/groups/testgroup --json '{"id": "testgroup", "name": "New Test group"}'
{"hidden":false,"categoryid":17,"id":"testgroup","icpc_id":null,"name":"New Test group","sortorder":0,"color":null,"allow_self_registration":false}

Any other information that you want to share?

I briefly looked into the code and was able to find the problematic code. It probably should not ignore the id from path and check it against the id provided in the object like it is done in other update endpoints. Basically it is missing a check similar to

domjudge/webapp/src/Controller/API/GroupController.php

Lines 157 to 159 in 795cc23

	if ($id !== $teamCategoryPut->id) {
	throw new BadRequestHttpException('ID in URL does not match ID in payload');
	}

For the main part of the problem, it might be also a little bit problematic, that the username also needs to be unique so currently the entity basically has 3 different unique IDs (userid, id and username).

The error message itself is due to

domjudge/webapp/src/Controller/API/UserController.php

Lines 350 to 352 in 795cc23

	if ($this->em->getRepository(User::class)->findOneBy(['username' => $addUser->username])) {
	throw new BadRequestHttpException(sprintf("User %s already exists", $addUser->username));
	}

I guess the condition there is missing a !($addUser instanceof UpdateUser) check, but even then I was not able to make it work due to some Integrity constraint violation / Duplicate entries in the database. My knowledge of the code base is currently not enough to find out where a new entity is created instead of updated.

[vmcj]

Related to: #3040