fix: return 403 when non-admin tries to do admin requests

[saw-jan]

Description

With this PR, all the graph admin requests if done by the non-admin users will respond with 403 Forbidden instead of 401 Unauthorized.

Motivation: #5742 (comment)

Now the 401 Unauthorized vs 403 Forbidden vs 404 Not Found status code may look tricky, but hara are the guidelines:

• we don't want to expose existence of resources if a user has no access to them, so we return a 404 Not Found instead of a 403 Forbidden.
• when a user has access to a resource but tries to execute an action that he does not have enough permissions for, e.g. when he tries to write to a read only share, we return a 403 Forbidden

• We are expecting 403 in some test scenarios.

But the above comment doesn't describe more about 401 Unauthorized vs 403 Forbidden. It is agreeable to respond with 403 when non-admin users do admin requests but the standard/policy and also from a security point of view, it could be different in oCIS.

The main motive of the PR is to have some conclusion and if agreed upon then hopefully merge the fix, if not then change the test expectation.

**Non-admin scenarios: should they respond with 403, 401 or 404 ❓ **

- try to change other users' role
- try to create a new user
- try to delete (own / non-existing / another / disabled) account
- try to change (own / another) account (email / displayname / password)
- try to disable/enable another user
- try to get all users
- try to get users of a group
- try to get users with a role
- try to add/remove (own/another) account to a group
- try to create a new group
- try to delete/rename a group
- try to get all groups

Related Issue

• Fixes API requests from an unauthorized user should return 403 #5938

Motivation and Context

How Has This Been Tested?

• test environment:

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Technical debt
• Tests only (no source changes)

Checklist:

• Code changes
• Unit tests added
• Acceptance tests added
• Documentation ticket raised:

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

[micbar]

@saw-jan Still a thing? or close?

[saw-jan]

@saw-jan Still a thing? or close?

I there can be a clarification on the following query, then it would be helpful to decide whether to work on it or close.

Non-admin scenarios: should they respond with 403, 401 or 404 ❓

Sorry, I totally forgot about this PR

[saw-jan]

#5938 (comment)

I agree with that in the current context. So 403 is the way to go.

Needs to be reviewed by dev team if this is correct and right place to do it

[saw-jan]

Failing tests are expected ones
Updated the tests expectations

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[phil-davis]

The other thing that we need to check is that these status codes should not "leak" information about what exists and does not exist.

When a user with not enough privileges tries to modify or delete something (user, group, space...) that exists, they get a 403 "forbidden" - good. If they try the same modify or delete to a non-existent user, group, space, then they should also get a 403 "forbidden". If they get 404 "not found" then they will know that the item really does not exist, and that "403" indicates that the item exists (and they do not have permission).

Similarly for "create" requests - if a 403 "forbidden"is given when trying to create a new user that does not already exist, then 403 should also be given if "create" is attempted for a user that already exists.

[phil-davis]

@saw-jan is there an issue about testing the cases where 403 and/or 404 status responses might leak information?
Or do the test scenarios already cover that well?

[saw-jan]

@saw-jan is there an issue about testing the cases where 403 and/or 404 status responses might leak information? Or do the test scenarios already cover that well?

There are some tests like this one

ocis/tests/acceptance/features/apiGraphUserGroup/deleteUser.feature

Lines 78 to 82 in 775913c

	Scenario Outline: non-admin user tries to delete a nonexistent user
	Given the administrator has assigned the role "<user-role>" to user "Alice" using the Graph API
	When the user "Alice" tries to delete a nonexistent user using the Graph API
	Then the HTTP status code should be "403"
	Examples:

[phil-davis]

There are some tests like this one

Good. If there are the same sort of test scenarios for non-existent group, non-existent space... for trying to delete and trying to modify then we are good.

[saw-jan]

Good. If there are the same sort of test scenarios for non-existent group, non-existent space... for trying to delete and trying to modify then we are good.

I will check for other test cases