test: api test update user only by admins + 1 minor test #3123
Conversation
📝 WalkthroughWalkthroughTwo new tests were added to the authenticated Users tests in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app_tests/api/test_api_users.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app_tests/api/test_api_users.py (1)
backend/iam/models.py (2)
is_admin(757-758)User(508-802)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: build (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: build_community_frontend
- GitHub Check: build_enterprise_frontend
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
| def test_user_visibility_scope(self, test): | ||
| user, _ = User.objects.get_or_create( | ||
| email=USER_EMAIL, | ||
| defaults={ | ||
| "first_name": USER_FIRSTNAME, | ||
| "last_name": USER_NAME, | ||
| "password": USER_PASSWORD, | ||
| "is_active": True, | ||
| }, | ||
| ) | ||
|
|
||
| client = test.client | ||
| response = client.get(reverse(API_ENDPOINT)) | ||
| data = response.json() | ||
|
|
||
| users_list = data.get("results", data) | ||
|
|
||
| if test.user_group.startswith("Global"): | ||
| assert len(users_list) >= 2 | ||
| else: | ||
| assert any(u["email"] == user.email for u in users_list) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the test file and get its overall structure
wc -l backend/app_tests/api/test_api_users.pyRepository: intuitem/ciso-assistant-community
Length of output: 120
🏁 Script executed:
# Find GROUPS_PERMISSIONS definition
rg -n "GROUPS_PERMISSIONS\s*=" backend/app_tests/ -A 20Repository: intuitem/ciso-assistant-community
Length of output: 1338
🏁 Script executed:
# Check the test method around line 95 mentioned in the review
sed -n '80,110p' backend/app_tests/api/test_api_users.pyRepository: intuitem/ciso-assistant-community
Length of output: 1161
🏁 Script executed:
# Check the actual test method in question (lines 230-250)
sed -n '230,260p' backend/app_tests/api/test_api_users.pyRepository: intuitem/ciso-assistant-community
Length of output: 1117
🏁 Script executed:
# Find all user_group values used in tests
rg -n "user_group.*=" backend/app_tests/api/test_api_users.py | head -20Repository: intuitem/ciso-assistant-community
Length of output: 305
🏁 Script executed:
# Verify what test.user_group values actually contain
grep -n "test.user_group\|user_group=" backend/app_tests/api/test_api_users.py | head -5Repository: intuitem/ciso-assistant-community
Length of output: 323
🏁 Script executed:
# Check imports to see if GROUPS_PERMISSIONS is imported in the test file
head -30 backend/app_tests/api/test_api_users.py | grep -E "^import|^from"Repository: intuitem/ciso-assistant-community
Length of output: 364
🏁 Script executed:
# Let's check if there are any comments or documentation about expected visibility behavior
rg -n "visibility|Global|scope" backend/app_tests/api/test_api_users.py -B 2 -A 2Repository: intuitem/ciso-assistant-community
Length of output: 1323
🏁 Script executed:
# Check the test class structure to understand test parametrization
sed -n '1,50p' backend/app_tests/api/test_api_users.pyRepository: intuitem/ciso-assistant-community
Length of output: 1422
🏁 Script executed:
# Check what user_group values are actually used in test parametrization
rg -n "parametrize|user_group" backend/app_tests/api/test_api_users.py | head -10Repository: intuitem/ciso-assistant-community
Length of output: 430
🏁 Script executed:
# Find the test class that contains test_user_visibility_scope and check its parametrization
sed -n '48,250p' backend/app_tests/api/test_api_users.py | head -80Repository: intuitem/ciso-assistant-community
Length of output: 2718
🏁 Script executed:
# Look for @pytest.mark.parametrize to understand which user groups are tested
rg -n "@pytest.mark.parametrize" backend/app_tests/api/test_api_users.py -A 5Repository: intuitem/ciso-assistant-community
Length of output: 59
🏁 Script executed:
# Search for conftest.py which typically contains pytest fixtures
find backend/app_tests -name "conftest.py" -exec cat {} \;Repository: intuitem/ciso-assistant-community
Length of output: 1923
🏁 Script executed:
# Also check for pytest.mark.parametrize with different pattern or fixture definition
rg -n "def test\(" backend/app_tests/api/test_api_users.py -B 3 | head -50Repository: intuitem/ciso-assistant-community
Length of output: 59
🏁 Script executed:
# Look for fixture definition that provides 'test' parameter
rg -n "@pytest.fixture|def test" backend/app_tests/ | grep -E "fixture|conftest"Repository: intuitem/ciso-assistant-community
Length of output: 811
🏁 Script executed:
# Let's verify the complete GROUPS_PERMISSIONS structure
cat backend/app_tests/test_vars.py | grep -A 5 "GROUPS_PERMISSIONS\|\"folder\""Repository: intuitem/ciso-assistant-community
Length of output: 1009
Fix incorrect scope check and add restricted visibility assertion for non-Global users.
The test has critical issues:
-
Line 247:
test.user_group.startswith("Global")will never be true. User group values are "BI-UG-ADM", "BI-UG-GAD", etc.—not "Global"-prefixed strings. UseGROUPS_PERMISSIONS[test.user_group]["folder"] == "Global"instead. -
Lines 248-250: For non-Global users, the test only verifies the created user is in the results but doesn't verify they cannot see unauthorized users. Add an assertion that non-Global users only see themselves.
Suggested fix
- if test.user_group.startswith("Global"):
+ folder_scope = GROUPS_PERMISSIONS[test.user_group]["folder"]
+ if folder_scope == "Global":
assert len(users_list) >= 2
else:
+ assert len(users_list) == 1
assert any(u["email"] == user.email for u in users_list)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/app_tests/api/test_api_users.py around lines 230 to 250, the
visibility check incorrectly tests test.user_group.startswith("Global") and
fails to assert restricted visibility for non-Global users; replace that
condition with GROUPS_PERMISSIONS[test.user_group]["folder"] == "Global" to
detect Global folder permission, and for the non-Global branch assert that the
returned users_list contains only the created user's email (e.g., either
len(users_list) == 1 and users_list[0]["email"] == user.email or assert
all(u["email"] == user.email for u in users_list)) so non-Global users cannot
see unauthorized users.
| def test_superuser_cannot_be_deactivated(self, test): | ||
| superuser, _ = User.objects.get_or_create( | ||
| email="[email protected]", | ||
| defaults={ | ||
| "first_name": "Admin", | ||
| "last_name": "User", | ||
| "password": USER_PASSWORD, | ||
| "is_superuser": True, | ||
| "is_active": True, | ||
| }, | ||
| ) | ||
|
|
||
| url = reverse("users-detail", args=[superuser.id]) | ||
| response = test.client.patch(url, {"is_active": False}, format="json") | ||
| superuser.refresh_from_db() | ||
|
|
||
| assert superuser.is_active is True |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing response status verification.
The test verifies that superuser.is_active remains True after attempting deactivation, but it doesn't check the API response status code. This leaves ambiguity about how the protection works:
- Does the API return
403 FORBIDDENto reject the operation? - Does it return
200 OKbut silently ignore the change? - Does it return
400 BAD_REQUESTwith a validation error?
Without verifying the response, the test cannot confirm the API's intended behavior for this edge case.
🔎 Suggested improvements
url = reverse("users-detail", args=[superuser.id])
response = test.client.patch(url, {"is_active": False}, format="json")
+
+ # Verify the API rejected or ignored the deactivation attempt
+ assert response.status_code in (
+ status.HTTP_200_OK, # If API silently ignores
+ status.HTTP_403_FORBIDDEN, # If API explicitly rejects
+ status.HTTP_400_BAD_REQUEST, # If API returns validation error
+ ), f"Unexpected status code: {response.status_code}"
+
superuser.refresh_from_db()
-
assert superuser.is_active is TrueAdditionally, consider documenting which status code is the expected behavior and assert specifically for that code.
🤖 Prompt for AI Agents
In backend/app_tests/api/test_api_users.py around lines 252-268, the test checks
that superuser.is_active remains True after a PATCH but doesn't assert the HTTP
response status; update the test to assert the explicit expected response status
(e.g., assert response.status_code == 403 or the chosen expected code) and add a
one-line comment or update the test name documenting that the API should reject
superuser deactivation with that status code so the behavior is unambiguous.
melinoix
changed the title
Here are 2 new tests :
test_update_only_if_admin – Only admin users can update other users; non-admins are blocked.
test_superuser_cannot_be_deactivated – Superusers cannot be deactivated via the API.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.