🐛 Handle non-existing user IDs in read_user_by_id

[saltie2193]

Fix an issue where read_user_by_id would fail to return if the requested user ID did not exist.

• Return 404 - Not Found when ID does not exist.
• Request without sufficient permission will always result in 403 - Unauthorized.
• Add tests to test requesting non-existing user IDs as superuser and normal user.

[berar]

Hello. I'd like to know why this pull request has not been approved. It is valid.

[jonbzt]

Yeah pretty minor upgrade but it makes sens to merge IMO.

[YuriiMotov]

@saltie2193, thanks for working on this!

Please, take a look at my change suggestions.
Basically, I think we should leave existing tests untouched (only rename test_get_existing_user -> test_get_existing_user_as_superuser) and add 2 test for non-existing user (superuser - 404, regular user - 403)

[YuriiMotov]

Case with is_superuser=True is already covered by test_get_existing_user_as_superuser.
No need to parameterize this test

[saltie2193]

It should not be the same as in test_get_existing_user_as_superuser.

test_get_existing_user_as_superuser tests whether a user authenticated as superuser is able to access another user. (User A tries to access data of user B)

test_get_exiting_user_current_user however tests whether a user is allowed to access his own data, independent of them being a superuser or not. (User A tries to access data of user A)

That's at least what they are intended to test.

[YuriiMotov]

So, this is to ensure that superuser can access their own user information, not only other user's information, right?
I still think this is redundant. I believe the chance somebody writes the algorithm this way is extremely low..

[saltie2193]

Yes. It's to ensure superuser privileges don't influence it.
Sure might be excessive but it's just an additional simple case of the test, ensuring the correct function of the backend, and not even expensive.

[YuriiMotov]

I think I can come up with 10 more additional tests. And all of them will be hypothetically possible.
I think we should consider:

• what is the probability that somebody writes this function this way (superuser can access random user, but can't access their own info. At the same time normal user can access their own info)?
• every change is additional load for people who will review it and read this code later

[saltie2193]

I mean especially when talking about authentication and authorization it do think it's not unlikely to not think about something like this, that seems really easy ans stupid. Especially when looking at differences between superusers and non superuser. Often the most simple parts are overlooked because people thought it's obvious how it's supposed to work and nobody would ever make a mistake like that.
This just uses as simple test case in a preventive way ensuring the backend is working as intended and at the same time explicitly documents the intended behavior.

But I do understand and have to respect it if this project does not want to use test cases in this way.

So we only test if the current user (authenticated as non-superuser) is able to access his data and I drop the changes to test_get_existing_user_current_user?

[YuriiMotov]

I think we should keep test_get_existing_user_current_user as it was before this PR

[saltie2193]

@YuriiMotov please have a look at the adjustments I made. I hope I could address all your concerns.

Note, I rebased the original commits onto the master branch so test_get_existing_user_current_user does not show as code contributed by me, since I did not change it.

[YuriiMotov]

LGTM

I added a few suggestions that can reduce the diff a bit. Feel free to ignore

@saltie2193, thank you!

[saltie2193]

From my side this is ready to merge.

@YuriiMotov, thank you for taking the time to review this pull request!

[github-actions]

This pull request has a merge conflict that needs to be resolved.