added service account flag and token generation endpoints #3440
Conversation
📝 WalkthroughWalkthroughAdds service account support: a User.is_service_account field and manager method, DB migration, TokenAuthentication support, ServiceAccountPermissions and ServiceAccountAccess authorization checks, UserViewSet endpoints to create/list/generate/revoke service account tokens, and filters to exclude service accounts by default. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (2)
care/users/models.py (1)
44-53: Consider extracting hardcoded defaults and address potential user_type issue.A few observations about the implementation:
The hardcoded
phone_number("+919696969696") andgender(3) values duplicate those increate_superuser(Lines 39-40). Consider extracting these as class-level constants.Line 48 sets
extra_fields["email"] = email, which seems redundant sinceUnlike
create_superuser, this method doesn't setuser_type. Looking at the viewset'sperform_createmethod (care/emr/api/viewsets/user.py, Lines 85-116),user_typeis used to create/assign organizations. Service accounts created through this path might not follow that flow, which could be intentional, but it's worth verifying that service accounts shouldn't have a user_type or organization assignment.🔎 Suggested refactor to reduce duplication
+class CustomUserManager(UserManager): + # Default values for system accounts + DEFAULT_SYSTEM_PHONE = "+919696969696" + DEFAULT_SYSTEM_GENDER = 3 + def get_queryset(self): qs = super().get_queryset() return qs.filter(deleted=False) def get_entire_queryset(self): return super().get_queryset() def create_superuser(self, username, email, password, **extra_fields): - extra_fields["phone_number"] = "+919696969696" - extra_fields["gender"] = 3 + extra_fields["phone_number"] = self.DEFAULT_SYSTEM_PHONE + extra_fields["gender"] = self.DEFAULT_SYSTEM_GENDER extra_fields["user_type"] = 40 return super().create_superuser(username, email, password, **extra_fields) def create_service_account(self, username, email, **extra_fields): extra_fields["is_service_account"] = True - extra_fields["phone_number"] = "+919696969696" - extra_fields["gender"] = 3 - extra_fields["email"] = email + extra_fields.setdefault("phone_number", self.DEFAULT_SYSTEM_PHONE) + extra_fields.setdefault("gender", self.DEFAULT_SYSTEM_GENDER) user = self.model(username=username, **extra_fields) user.set_unusable_password() user.save(using=self._db) return usercare/emr/api/viewsets/user.py (1)
249-261: Add audit logging for token revocation.Token revocation is a security-relevant event that should be logged for audit purposes. This is especially important for compliance and security investigations.
🔎 Suggested improvement
+import logging + +logger = logging.getLogger(__name__) + @action(detail=True, methods=["DELETE"]) def revoke_service_account_token(self, request, *args, **kwargs): if not request.user.is_superuser: raise PermissionDenied("Only superusers can revoke tokens") user = self.get_object() if not user.is_service_account: return Response({"error": "Not a service account"}, status=400) Token.objects.filter(user=user).delete() + + logger.info( + f"Service account token revoked for {user.username} by {request.user.username}" + ) return Response({"message": "Token revoked successfully"})
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
care/emr/api/viewsets/user.pycare/users/migrations/0025_user_is_service_account.pycare/users/models.pyconfig/settings/base.py
🧰 Additional context used
📓 Path-based instructions (8)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance).
Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables).
Files:
care/users/migrations/0025_user_is_service_account.pycare/users/models.pyconfig/settings/base.pycare/emr/api/viewsets/user.py
**/{models,views,forms}.py
📄 CodeRabbit inference engine (.cursorrules)
Use Django’s built-in user model and authentication framework for user management.
Files:
care/users/models.py
**/{forms,models}.py
📄 CodeRabbit inference engine (.cursorrules)
Use Django's validation framework to validate form and model data.
Files:
care/users/models.py
**/{views,models,tasks}.py
📄 CodeRabbit inference engine (.cursorrules)
Prefer try-except blocks for handling exceptions in business logic and views.
Files:
care/users/models.py
**/{models,forms,views}.py
📄 CodeRabbit inference engine (.cursorrules)
Keep business logic in models and forms; keep views light and focused on request handling.
Files:
care/users/models.py
**/{views,models}*.py
📄 CodeRabbit inference engine (.cursorrules)
**/{views,models}*.py: Leverage Django’s caching framework to optimize performance for frequently accessed data.
Use Django’s cache framework with backend support (e.g., Redis or Memcached) to reduce database load.
Files:
care/users/models.py
**/{views,models}.py
📄 CodeRabbit inference engine (.cursorrules)
Optimize query performance using Django ORM's select_related and prefetch_related for related object fetching.
Files:
care/users/models.py
**/models.py
📄 CodeRabbit inference engine (.cursorrules)
Implement database indexing and query optimization techniques for better performance.
Files:
care/users/models.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: ohcnetwork/care PR: 0
File: .cursorrules:0-0
Timestamp: 2025-07-18T15:17:40.483Z
Learning: Applies to **/{models,views,forms}.py : Use Django’s built-in user model and authentication framework for user management.
🧬 Code graph analysis (2)
care/users/models.py (1)
care/emr/api/viewsets/user.py (1)
create_service_account(198-226)
care/emr/api/viewsets/user.py (1)
care/users/models.py (3)
create_service_account(44-53)User(107-225)check_username_exists(218-219)
🪛 Ruff (0.14.10)
care/users/migrations/0025_user_is_service_account.py
8-10: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
12-18: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
care/emr/api/viewsets/user.py
198-198: Unused method argument: args
(ARG002)
198-198: Unused method argument: kwargs
(ARG002)
200-200: Avoid specifying long messages outside the exception class
(TRY003)
229-229: Unused method argument: args
(ARG002)
229-229: Unused method argument: kwargs
(ARG002)
231-231: Avoid specifying long messages outside the exception class
(TRY003)
250-250: Unused method argument: args
(ARG002)
250-250: Unused method argument: kwargs
(ARG002)
252-252: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (1)
- GitHub Check: Test / Test
🔇 Additional comments (2)
config/settings/base.py (1)
371-371: LGTM - TokenAuthentication properly enabled.The addition of TokenAuthentication to the authentication classes is correct and aligns with the new service account token management functionality. The required
rest_framework.authtokenapp is already present in INSTALLED_APPS (Line 123).care/users/models.py (1)
179-180: Field definition looks good.The
is_service_accountBooleanField is correctly defined with an appropriate default value.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
care/emr/api/viewsets/user.py (4)
214-216: Wrap service account creation in try/except for graceful error handling.The database operation could fail (e.g., email uniqueness constraint, database errors), and currently there's no handling for such cases. The user would receive an unhandled 500 error, which isn't exactly the best user experience one could hope for.
🔎 Proposed fix
+ try: service_account = User.objects.create_service_account( username=username, email=email ) + except IntegrityError: + return Response({"error": "Failed to create service account. Email may already exist."}, status=400)
232-237: Consider converting QuerySet to list and adding pagination.Returning a QuerySet directly to
Responserelies on implicit serialization. While this works, it's slightly unconventional. Also, if service accounts grow over time, the lack of pagination could become... interesting.🔎 Proposed improvement
return Response( - User.objects.get_entire_queryset() - .filter(is_service_account=True, deleted=False) - .values("external_id", "username"), + list( + User.objects.get_entire_queryset() + .filter(is_service_account=True, deleted=False) + .values("external_id", "username") + ), status=200, )For pagination, consider using DRF's built-in pagination classes in a future iteration.
260-272: Consider returning 204 No Content for successful deletion.For REST semantics, a successful DELETE typically returns 204 with no body, rather than 200 with a message. The current implementation works, but it's slightly inconsistent with typical REST conventions. Not that anyone's counting, of course.
Also, similar to token generation, audit logging would be beneficial here for tracking who revoked which service account's token.
🔎 Proposed fix
Token.objects.filter(user=user).delete() - return Response({"message": "Token revoked successfully"}) + return Response(status=204)Or keep the message but add explicit status:
- return Response({"message": "Token revoked successfully"}) + return Response({"message": "Token revoked successfully"}, status=200)
197-198: Missing@extend_schemadecorators for API documentation.The existing
getcurrentuserendpoint (Line 139) uses@extend_schemafor OpenAPI documentation, but none of the new service account endpoints have this decorator. This means they'll appear in the generated API docs with minimal information, which could make life harder for API consumers trying to understand the expected request/response formats.Consider adding schema decorators, for example:
from drf_spectacular.utils import extend_schema, OpenApiResponse @extend_schema( request={"type": "object", "properties": {"username": {"type": "string"}, "email": {"type": "string"}}}, responses={201: OpenApiResponse(description="Service account created successfully")} ) @action(detail=False, methods=["POST"]) def create_service_account(self, request, *args, **kwargs): ...Also applies to: 228-229, 239-240, 260-261
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
care/emr/api/viewsets/user.pycare/users/models.py
🚧 Files skipped from review as they are similar to previous changes (1)
- care/users/models.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance).
Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables).
Files:
care/emr/api/viewsets/user.py
🪛 Ruff (0.14.10)
care/emr/api/viewsets/user.py
198-198: Unused method argument: args
(ARG002)
198-198: Unused method argument: kwargs
(ARG002)
200-200: Avoid specifying long messages outside the exception class
(TRY003)
229-229: Unused method argument: args
(ARG002)
229-229: Unused method argument: kwargs
(ARG002)
231-231: Avoid specifying long messages outside the exception class
(TRY003)
240-240: Unused method argument: args
(ARG002)
240-240: Unused method argument: kwargs
(ARG002)
242-242: Avoid specifying long messages outside the exception class
(TRY003)
261-261: Unused method argument: args
(ARG002)
261-261: Unused method argument: kwargs
(ARG002)
263-263: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (1)
- GitHub Check: Test / Test
🔇 Additional comments (2)
care/emr/api/viewsets/user.py (2)
7-7: LGTM!The
Tokenimport is correctly placed alongside other rest_framework imports and is necessary for the new token management functionality.
252-258: Token response is appropriate, but ensure clients understand this is a one-time reveal.The token key is only visible at creation time (as is standard with DRF Token auth). This is the expected behavior, though it might be worth documenting this in API docs so users don't come asking why they can't retrieve their token later.
Previous review comments on this method regarding transaction atomicity, rate limiting, and audit logging remain valid and should be addressed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
care/users/models.py (1)
179-179: Consider adding a database index for query performance.If service accounts will be filtered frequently (e.g., in admin interfaces or API endpoints that list/filter by account type), adding
db_index=Truewould optimize those queries on large user tables. As per the coding guidelines, implementing database indexing is recommended for better performance.🔎 Proposed refactor
- is_service_account = models.BooleanField(default=False) + is_service_account = models.BooleanField(default=False, db_index=True)
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
care/users/models.pyconfig/settings/base.py
🚧 Files skipped from review as they are similar to previous changes (1)
- config/settings/base.py
🧰 Additional context used
📓 Path-based instructions (8)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance).
Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables).
Files:
care/users/models.py
**/{models,views,forms}.py
📄 CodeRabbit inference engine (.cursorrules)
Use Django’s built-in user model and authentication framework for user management.
Files:
care/users/models.py
**/{forms,models}.py
📄 CodeRabbit inference engine (.cursorrules)
Use Django's validation framework to validate form and model data.
Files:
care/users/models.py
**/{views,models,tasks}.py
📄 CodeRabbit inference engine (.cursorrules)
Prefer try-except blocks for handling exceptions in business logic and views.
Files:
care/users/models.py
**/{models,forms,views}.py
📄 CodeRabbit inference engine (.cursorrules)
Keep business logic in models and forms; keep views light and focused on request handling.
Files:
care/users/models.py
**/{views,models}*.py
📄 CodeRabbit inference engine (.cursorrules)
**/{views,models}*.py: Leverage Django’s caching framework to optimize performance for frequently accessed data.
Use Django’s cache framework with backend support (e.g., Redis or Memcached) to reduce database load.
Files:
care/users/models.py
**/{views,models}.py
📄 CodeRabbit inference engine (.cursorrules)
Optimize query performance using Django ORM's select_related and prefetch_related for related object fetching.
Files:
care/users/models.py
**/models.py
📄 CodeRabbit inference engine (.cursorrules)
Implement database indexing and query optimization techniques for better performance.
Files:
care/users/models.py
🧬 Code graph analysis (1)
care/users/models.py (1)
care/emr/api/viewsets/user.py (1)
create_service_account(202-234)
⏰ 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). (1)
- GitHub Check: Test / Test
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #3440 +/- ##
=========================================
Coverage 74.46% 74.47%
=========================================
Files 449 459 +10
Lines 20700 20957 +257
Branches 2152 2165 +13
=========================================
+ Hits 15415 15608 +193
- Misses 4822 4882 +60
- Partials 463 467 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| return Response({}) | ||
|
|
||
| @action(detail=False, methods=["POST"]) | ||
| def create_service_account(self, request, *args, **kwargs): |
There was a problem hiding this comment.
Why recreate the user creation flow? can't we reuse the same endpoint?
There was a problem hiding this comment.
There are alot of validations and roles involved in the user create endpoint, which are not required for service account, therefore makes it difficult to reuse for creating service account. Creating a seprate endpoint keeps it simpler
There was a problem hiding this comment.
@praffq Service Account should have all validations of a User, only difference would be that it has the flag enabled.
care/emr/api/viewsets/user.py
Outdated
|
|
||
| @action(detail=False, methods=["POST"]) | ||
| def create_service_account(self, request, *args, **kwargs): | ||
| if not request.user.is_superuser: |
There was a problem hiding this comment.
I don't think this is a super user level action;
care/emr/api/viewsets/user.py
Outdated
| @action(detail=False, methods=["GET"]) | ||
| def get_service_accounts(self, request, *args, **kwargs): | ||
| if not request.user.is_superuser: | ||
| raise PermissionDenied("Only superusers can list service accounts") | ||
| return Response( | ||
| User.objects.get_entire_queryset() | ||
| .filter(is_service_account=True, deleted=False) | ||
| .values("external_id", "username"), | ||
| status=200, | ||
| ) |
There was a problem hiding this comment.
shouldn't the other list APIs exclude service accounts in that case? 🤔
There was a problem hiding this comment.
@rithviknishad I did update the User manager to exclude the service account but migrations started throwing errors so had to revert changes.
context - https://github.com/ohcnetwork/care/actions/runs/20658115097/job/59314852938
There was a problem hiding this comment.
Added fix for this in latest commit
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
care/emr/api/viewsets/user.py (1)
256-279: Add transaction wrapping, rate limiting, and audit logging for token operations.Lines 270-271 perform token deletion and creation without atomic transaction wrapping, which could leave the service account without a token if creation fails. Additionally, this sensitive operation lacks rate limiting and audit logging, as noted in previous review comments.
🔎 Proposed improvements
+from django.db import transaction +from django_ratelimit.decorators import ratelimit +import logging + +logger = logging.getLogger(__name__) + +@ratelimit(key='user', rate='5/h', method='POST') @action(detail=True, methods=["POST"]) def generate_service_account_token(self, request, *args, **kwargs): if not AuthorizationController.call( "can_manage_service_account_token", self.request.user ): raise PermissionDenied( "You do not have permission to update token for service account" ) user = self.get_object() if not user.is_service_account: return Response({"error": "Only service accounts allowed"}, status=400) - Token.objects.filter(user=user).delete() - token = Token.objects.create(user=user) + with transaction.atomic(): + Token.objects.filter(user=user).delete() + token = Token.objects.create(user=user) + + logger.info( + f"Service account token generated for {user.username} by {request.user.username}" + ) return Response( { "token": token.key, "user": user.username, "created": token.created.isoformat(), } )
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
care/emr/api/viewsets/user.pycare/security/authorization/service_account.pycare/security/permissions/service_account.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance).
Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables).
Files:
care/security/authorization/service_account.pycare/emr/api/viewsets/user.pycare/security/permissions/service_account.py
🧬 Code graph analysis (2)
care/security/authorization/service_account.py (3)
care/emr/models/organization.py (2)
FacilityOrganizationUser(124-131)OrganizationUser(110-121)care/security/authorization/base.py (4)
AuthorizationController(72-113)AuthorizationHandler(11-69)get_role_from_permissions(61-69)register_internal_controller(111-113)care/security/permissions/service_account.py (1)
ServiceAccountPermissions(16-45)
care/emr/api/viewsets/user.py (2)
care/users/models.py (5)
get_queryset(31-33)create_service_account(44-53)User(107-225)check_username_exists(218-219)get_entire_queryset(35-36)care/security/authorization/base.py (2)
AuthorizationController(72-113)call(97-108)
🪛 Ruff (0.14.10)
care/emr/api/viewsets/user.py
202-202: Unused method argument: args
(ARG002)
202-202: Unused method argument: kwargs
(ARG002)
206-208: Avoid specifying long messages outside the exception class
(TRY003)
241-241: Unused method argument: request
(ARG002)
241-241: Unused method argument: args
(ARG002)
241-241: Unused method argument: kwargs
(ARG002)
245-247: Avoid specifying long messages outside the exception class
(TRY003)
257-257: Unused method argument: request
(ARG002)
257-257: Unused method argument: args
(ARG002)
257-257: Unused method argument: kwargs
(ARG002)
261-263: Avoid specifying long messages outside the exception class
(TRY003)
282-282: Unused method argument: request
(ARG002)
282-282: Unused method argument: args
(ARG002)
282-282: Unused method argument: kwargs
(ARG002)
286-288: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (1)
- GitHub Check: Test / Test
🔇 Additional comments (7)
care/security/authorization/service_account.py (2)
10-24: LGTM!The authorization logic correctly checks superuser status and validates role membership across both OrganizationUser and FacilityOrganizationUser tables.
42-56: LGTM!The token management authorization check correctly uses the
can_manage_service_accountpermission and follows the same pattern as the other methods.care/security/permissions/service_account.py (1)
16-45: LGTM!The permission definitions are well-structured with appropriate role assignments. The broader role access for
can_list_service_account(including DOCTOR_ROLE, NURSE_ROLE, etc.) makes sense for read operations, while create and manage operations are correctly restricted to administrative roles.care/emr/api/viewsets/user.py (4)
1-11: LGTM!The new imports support the service account functionality appropriately: uuid for email generation, email validation utilities, and Token model for authentication.
87-87: Good filtering adjustment.Excluding service accounts from the default queryset ensures they don't appear in standard user listings, which aligns with the separate
get_service_accountsendpoint.
240-254: LGTM!The list action correctly uses the authorization check and
get_entire_queryset()to bypass the default service account filter, returning only the necessary fields.
281-297: LGTM!The revocation logic correctly validates the service account status and clears tokens, with appropriate authorization checks in place.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
care/emr/api/viewsets/user.py (1)
256-279: Token generation still lacks transaction safety, rate limiting, and audit logging.The concerns raised in the previous review remain unaddressed. Specifically:
- Transaction atomicity: Token deletion (line 270) and creation (line 271) should be wrapped in
transaction.atomic()to prevent leaving a service account tokenless if creation fails.- Rate limiting: Prevents abuse if credentials are compromised.
- Audit logging: Essential for security audit trails.
These improvements were detailed in the previous review with implementation examples.
🧹 Nitpick comments (3)
care/emr/api/viewsets/user.py (3)
216-217: Email generation could be more robust.Slicing the string representation of UUID (
str(uuid.uuid4())[:5]) works but produces only 5 characters of a hyphenated UUID string. Consider usinguuid.uuid4().hex[:8]for a cleaner, more distinctive identifier that avoids hyphens.🔎 Suggested improvement
- if email is None: - email = username + str(uuid.uuid4())[:5] + "@service.local" + if email is None: + email = f"{username}.{uuid.uuid4().hex[:8]}@service.local"
201-238: Consider adding rate limiting and audit logging.Service account creation is a sensitive operation that would benefit from:
- Rate limiting to prevent abuse (django_ratelimit is already available in the project)
- Audit logging to track who created which service accounts and when
These aren't blocking issues but would improve the security posture.
🔎 Example implementation
import logging from django_ratelimit.decorators import ratelimit logger = logging.getLogger(__name__) @ratelimit(key='user', rate='10/h', method='POST') @action(detail=False, methods=["POST"]) def create_service_account(self, request, *args, **kwargs): # ... existing authorization and validation ... service_account = User.objects.create_service_account( username=username, email=email ) logger.info( f"Service account '{service_account.username}' created by {request.user.username}", extra={ "service_account_id": str(service_account.external_id), "created_by": request.user.username, } ) return Response(...)
281-297: Consider adding audit logging for token revocation.Token revocation is a security-relevant operation. Logging who revoked which service account's token would provide useful audit trails, especially for compliance and incident investigation.
🔎 Example implementation
import logging logger = logging.getLogger(__name__) @action(detail=True, methods=["DELETE"]) def revoke_service_account_token(self, request, *args, **kwargs): # ... existing checks ... Token.objects.filter(user=user).delete() logger.info( f"Token revoked for service account '{user.username}' by {request.user.username}", extra={ "service_account_id": str(user.external_id), "revoked_by": request.user.username, } ) return Response({"message": "Token revoked successfully"})
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
care/emr/api/viewsets/user.pycare/security/authorization/service_account.py
🚧 Files skipped from review as they are similar to previous changes (1)
- care/security/authorization/service_account.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance).
Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables).
Files:
care/emr/api/viewsets/user.py
🧬 Code graph analysis (1)
care/emr/api/viewsets/user.py (3)
care/users/models.py (5)
get_queryset(31-33)create_service_account(44-53)User(107-225)check_username_exists(218-219)get_entire_queryset(35-36)care/security/authorization/base.py (2)
AuthorizationController(72-113)call(97-108)care/utils/models/base.py (1)
delete(30-32)
🪛 Ruff (0.14.10)
care/emr/api/viewsets/user.py
202-202: Unused method argument: args
(ARG002)
202-202: Unused method argument: kwargs
(ARG002)
206-208: Avoid specifying long messages outside the exception class
(TRY003)
241-241: Unused method argument: request
(ARG002)
241-241: Unused method argument: args
(ARG002)
241-241: Unused method argument: kwargs
(ARG002)
245-247: Avoid specifying long messages outside the exception class
(TRY003)
257-257: Unused method argument: request
(ARG002)
257-257: Unused method argument: args
(ARG002)
257-257: Unused method argument: kwargs
(ARG002)
261-263: Avoid specifying long messages outside the exception class
(TRY003)
282-282: Unused method argument: request
(ARG002)
282-282: Unused method argument: args
(ARG002)
282-282: Unused method argument: kwargs
(ARG002)
286-288: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (1)
- GitHub Check: Test / Test
🔇 Additional comments (3)
care/emr/api/viewsets/user.py (3)
1-11: Imports look appropriate.All new imports are properly utilized in the service account endpoints.
86-87: Good separation of service accounts from regular user listings.The queryset filter appropriately excludes service accounts, ensuring they only appear in the dedicated service account endpoint.
240-254: Service account listing implementation looks solid.Correctly bypasses the normal queryset filter using
get_entire_queryset()and applies appropriate filters for service accounts. The minimal response fields (external_id, username) are suitable for a listing endpoint.
| "Can manage service account in care", | ||
| "", | ||
| PermissionContext.FACILITY, | ||
| [ADMIN_ROLE, FACILITY_ADMIN_ROLE, ADMINISTRATOR], |
There was a problem hiding this comment.
I don't think we need to assign it to anyone by default; let deployments choose to add if they want.
care/emr/api/viewsets/user.py
Outdated
| ) | ||
|
|
||
| @action(detail=False, methods=["GET"]) | ||
| def get_service_accounts(self, request, *args, **kwargs): |
There was a problem hiding this comment.
Again; we are duplicate efforts here, i am not sure what's stopping us from giving a simple filter is users list 🤔
care/emr/api/viewsets/user.py
Outdated
| def get_service_accounts(self, request, *args, **kwargs): | ||
| if not AuthorizationController.call( | ||
| "can_list_service_account", self.request.user | ||
| ): | ||
| raise PermissionDenied( | ||
| "You do not have permission to list service accounts" | ||
| ) | ||
|
|
||
| return Response( | ||
| User.objects.get_entire_queryset() | ||
| .filter(is_service_account=True, deleted=False) | ||
| .values("external_id", "username"), | ||
| status=200, | ||
| ) |
There was a problem hiding this comment.
this makes it harder to get service accounts scoped under a facility's dept.
we should treat service accounts just like user accounts right?
it might be simpler to add a filter in the filterset for is_service_account and maybe default it to false if none provided?
There was a problem hiding this comment.
this needs to be imported in authorization/__init__.py in order for it to be registered right?
There was a problem hiding this comment.
this needs to be added to the internal_permission_handlers in permissions/base.py right?
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @care/emr/api/viewsets/user.py:
- Around line 221-222: The temporary email generation uses a truncated UUID
(uuid.uuid4()[:5]) which greatly increases collision risk; update the logic in
the user creation path (the block that sets email when email is None,
referencing the variables email and username) to append the full uuid4 string
(or at least a much longer portion, e.g., the entire uuid.hex) instead of the
5-character slice so generated emails are effectively unique; ensure formatting
still yields username + uuid + "@service.local" and that any downstream code
that parses or validates email handles the longer identifier.
♻️ Duplicate comments (1)
care/emr/api/viewsets/user.py (1)
245-274: Previous feedback on rate limiting, audit logging, and transaction safety remains unaddressed.The concerns raised in the earlier review about missing rate limiting, audit logging, and transaction atomicity for the token delete/create operations are still valid. The past comment provided detailed suggestions with code examples.
However, there's an additional consideration: Line 258 allows service accounts to regenerate their own tokens via
is_own_account = self.request.user.id == user.id. If a service account's token is compromised, the attacker could use it to generate a new token, maintaining persistent access. You might want to restrict token generation to authorized human users only.Should service accounts be allowed to regenerate their own tokens, or should this be restricted to authorized administrators?
🧹 Nitpick comments (3)
care/emr/api/viewsets/user.py (3)
206-243: Consider adding audit logging for service account creation.Creating service accounts is a privileged operation that would benefit from audit trails showing who created which service accounts and when. This helps with security compliance and troubleshooting.
🔎 Suggested implementation
+import logging + +logger = logging.getLogger(__name__) + @action(detail=False, methods=["POST"]) def create_service_account(self, request, *args, **kwargs): if not AuthorizationController.call( "can_create_service_account", self.request.user ): raise PermissionDenied( "You do not have permission to create service accounts" ) username = request.data.get("username") email = request.data.get("email") if not username: return Response({"error": "username is required"}, status=400) if email is None: email = username + str(uuid.uuid4())[:5] + "@service.local" else: try: validate_email(email) except ValidationError: return Response({"error": "Invalid email format"}, status=400) if User.check_username_exists(username): return Response({"error": "Username already exists"}, status=400) service_account = User.objects.create_service_account( username=username, email=email ) + + logger.info( + f"Service account created: {username} (ID: {service_account.external_id}) " + f"by user {request.user.username} (ID: {request.user.id})" + ) return Response( { "id": str(service_account.external_id), "username": service_account.username, "is_service_account": True, }, status=201, )
276-298: Authorization logic is duplicated between token generation and revocation.Lines 286-294 duplicate the authorization check from
generate_service_account_token(lines 255-263). This creates a maintenance burden and increases the risk of inconsistencies if the authorization logic needs to change.🔎 Proposed refactor
Extract the authorization logic into a helper method:
def _authorize_service_account_token_operation(self, user): """Check if the current user can manage tokens for the given service account.""" has_permission = AuthorizationController.call( "can_manage_service_account_token", self.request.user ) is_own_account = self.request.user.id == user.id if not (has_permission or is_own_account): raise PermissionDenied( "You do not have permission to update token for service account" ) @action(detail=True, methods=["POST"]) def generate_service_account_token(self, request, *args, **kwargs): user = get_object_or_404( User.objects.filter(deleted=False), **{self.lookup_field: self.kwargs[self.lookup_field]}, ) if not user.is_service_account: return Response({"error": "Only service accounts allowed"}, status=400) self._authorize_service_account_token_operation(user) Token.objects.filter(user=user).delete() token = Token.objects.create(user=user) return Response( { "token": token.key, "user": user.username, "created": token.created.isoformat(), } ) @action(detail=True, methods=["DELETE"]) def revoke_service_account_token(self, request, *args, **kwargs): user = get_object_or_404( User.objects.filter(deleted=False), **{self.lookup_field: self.kwargs[self.lookup_field]}, ) if not user.is_service_account: return Response({"error": "Not a service account"}, status=400) self._authorize_service_account_token_operation(user) Token.objects.filter(user=user).delete() return Response({"message": "Token revoked successfully"})Also, the same security consideration mentioned for token generation applies here: allowing service accounts to revoke their own tokens via
is_own_account(line 289) might not be desirable depending on your threat model.
296-296: Consider adding audit logging for token revocation.While perhaps less critical than logging token generation, recording token revocations can still be valuable for security audits and troubleshooting, especially if tokens are being cycled or access is being removed.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
care/emr/api/viewsets/user.pycare/security/authorization/__init__.pycare/security/authorization/service_account.pycare/security/permissions/base.pycare/security/permissions/service_account.py
🚧 Files skipped from review as they are similar to previous changes (2)
- care/security/authorization/service_account.py
- care/security/permissions/service_account.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance).
Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables).
Files:
care/security/permissions/base.pycare/security/authorization/__init__.pycare/emr/api/viewsets/user.py
🧬 Code graph analysis (2)
care/security/permissions/base.py (1)
care/security/permissions/service_account.py (1)
ServiceAccountPermissions(11-24)
care/emr/api/viewsets/user.py (2)
care/utils/shortcuts.py (1)
get_object_or_404(6-15)care/users/models.py (3)
create_service_account(44-53)User(107-225)check_username_exists(218-219)
🪛 Ruff (0.14.10)
care/emr/api/viewsets/user.py
207-207: Unused method argument: args
(ARG002)
207-207: Unused method argument: kwargs
(ARG002)
211-213: Avoid specifying long messages outside the exception class
(TRY003)
246-246: Unused method argument: request
(ARG002)
246-246: Unused method argument: args
(ARG002)
246-246: Unused method argument: kwargs
(ARG002)
261-263: Avoid specifying long messages outside the exception class
(TRY003)
277-277: Unused method argument: request
(ARG002)
277-277: Unused method argument: args
(ARG002)
277-277: Unused method argument: kwargs
(ARG002)
292-294: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (1)
- GitHub Check: Test / Test
🔇 Additional comments (5)
care/security/authorization/__init__.py (1)
27-27: LGTM!The import follows the established pattern and is correctly placed in alphabetical order.
care/security/permissions/base.py (1)
37-37: LGTM!The import and registration of
ServiceAccountPermissionsfollow the established pattern. The new permission handler is now properly integrated into the permission system.Also applies to: 96-96
care/emr/api/viewsets/user.py (3)
1-1: LGTM!The new imports are all utilized in the service account management actions and follow Django/DRF conventions.
Also applies to: 3-4, 11-11, 39-39
74-74: LGTM!The filter addition enables proper filtering of service accounts and integrates well with the updated
get_querysetmethod.
89-92: LGTM!This change properly addresses the previous feedback about excluding service accounts from regular user listings by default. The logic correctly includes service accounts only when explicitly requested via the filter.
| if email is None: | ||
| email = username + str(uuid.uuid4())[:5] + "@service.local" |
There was a problem hiding this comment.
UUID truncation significantly reduces collision resistance.
Using uuid.uuid4()[:5] provides only ~1 million possible values, which could lead to email collisions in systems with many service accounts. While the generated email is for internal use only (.local domain), collisions would still cause account creation failures.
🔎 Proposed fix
if email is None:
- email = username + str(uuid.uuid4())[:5] + "@service.local"
+ email = f"{username}-{uuid.uuid4()}@service.local"This uses the full UUID to virtually eliminate collision risk.
🤖 Prompt for AI Agents
In @care/emr/api/viewsets/user.py around lines 221-222, The temporary email
generation uses a truncated UUID (uuid.uuid4()[:5]) which greatly increases
collision risk; update the logic in the user creation path (the block that sets
email when email is None, referencing the variables email and username) to
append the full uuid4 string (or at least a much longer portion, e.g., the
entire uuid.hex) instead of the 5-character slice so generated emails are
effectively unique; ensure formatting still yields username + uuid +
"@service.local" and that any downstream code that parses or validates email
handles the longer identifier.
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @care/emr/api/viewsets/facility_organization.py:
- Around line 379-386: get_queryset currently filters service accounts only by
query param but does not enforce extra authorization when
is_service_account=true; update the method (get_queryset in FacilityOrganization
viewset) to detect when self.request.query_params.get("is_service_account") is
truthy and then perform an explicit permission check (e.g., call a new or
existing permission like can_view_service_accounts or use request.user.has_perm/
self.check_object_permissions with can_list_facility_organization_users_obj
scoped to service accounts) and raise PermissionDenied if the check fails,
otherwise proceed to include service accounts; ensure the permission symbol
referenced is unique (can_list_facility_organization_users_obj or a new
can_view_service_accounts) so reviewers can find and wire the check where the
queryset is built.
🧹 Nitpick comments (1)
care/emr/api/viewsets/facility_organization.py (1)
368-386: Consider documenting the default filtering behavior.The exclusion of service accounts by default (when the filter parameter is not provided) is a reasonable design choice, but it might not be immediately obvious to future maintainers or API consumers. A brief docstring note would clarify the intent.
Optional: Add documentation
def get_queryset(self): """ Only users part of the organization can access its users + + Note: Service accounts are excluded by default unless explicitly + requested via the is_service_account query parameter. """ organization = self.get_organization_obj()
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
care/emr/api/viewsets/facility_organization.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance).
Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables).
Files:
care/emr/api/viewsets/facility_organization.py
🧠 Learnings (1)
📚 Learning: 2024-11-26T05:41:40.226Z
Learnt from: aravindm4
Repo: ohcnetwork/care PR: 2585
File: care/facility/api/viewsets/facility_flag.py:14-27
Timestamp: 2024-11-26T05:41:40.226Z
Learning: In the `care/facility/api/viewsets/facility_flag.py` file and similar viewsets in this Django REST Framework project, pagination is added by default. For APIs restricted to superusers using the `IsSuperUser` permission class, additional rate limiting or pagination adjustments are not required.
Applied to files:
care/emr/api/viewsets/facility_organization.py
🪛 Ruff (0.14.10)
care/emr/api/viewsets/facility_organization.py
250-250: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ 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). (1)
- GitHub Check: Test / Test
🔇 Additional comments (2)
care/emr/api/viewsets/facility_organization.py (2)
240-242: LGTM!The filter class is correctly implemented following django-filter conventions.
249-250: LGTM! Configuration is correct.The filterset and filter backends are properly configured. The static analysis warning (RUF012) about
filter_backendsis a false positive—this is the standard Django REST Framework pattern where class attributes are meant to be inherited/overridden without requiringClassVarannotation.
| queryset = FacilityOrganizationUser.objects.filter( | ||
| organization=organization | ||
| ).select_related("organization", "user", "role") | ||
|
|
||
| if "is_service_account" not in self.request.query_params: | ||
| queryset = queryset.filter(user__is_service_account=False) | ||
|
|
||
| return queryset |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if there are additional authorization controls for service account access
# Search for service account permission definitions
ast-grep --pattern 'class ServiceAccountPermission'
# Search for authorization rules mentioning service accounts
rg -n --type=py 'service_account.*permission|permission.*service_account' -i
# Check if the authorization controller has service-account-specific rules
rg -n --type=py 'can_.*service_account' -iRepository: ohcnetwork/care
Length of output: 1660
🏁 Script executed:
# Check the file structure and understand the authorization context
fd -t f "permission" care/emr/api | head -20Repository: ohcnetwork/care
Length of output: 41
🏁 Script executed:
# Look for the viewset class and its permission_classes
rg -n --type=py -B5 -A10 'facility_organization' care/emr/api/viewsets/ | grep -i "permission\|class"Repository: ohcnetwork/care
Length of output: 2495
🏁 Script executed:
# Check for any authorization decorator or method in the facility_organization viewset
cat care/emr/api/viewsets/facility_organization.py | head -100Repository: ohcnetwork/care
Length of output: 4205
🏁 Script executed:
# Get the method containing lines 379-386
sed -n '360,400p' care/emr/api/viewsets/facility_organization.pyRepository: ohcnetwork/care
Length of output: 1098
🏁 Script executed:
# Search for the get_queryset method and related authorization checks
rg -n --type=py -B10 "def get_queryset" care/emr/api/viewsets/facility_organization.pyRepository: ohcnetwork/care
Length of output: 968
🏁 Script executed:
# Check what authorization checks exist for listing organization users
rg -n --type=py -B5 -A5 "can_list_facility_organization_users" care/emr/api/viewsets/facility_organization.pyRepository: ohcnetwork/care
Length of output: 572
🏁 Script executed:
# Search for any service account visibility or authorization logic
rg -n --type=py "is_service_account" care/emr/api/viewsets/facility_organization.pyRepository: ohcnetwork/care
Length of output: 293
🏁 Script executed:
# Check the authorization controller to see if there's a method for listing organization users
cat care/security/authorization/__init__.pyRepository: ohcnetwork/care
Length of output: 1310
🏁 Script executed:
# Check the authorization controller implementation for can_list_facility_organization_users_obj
rg -n --type=py -B5 -A15 "can_list_facility_organization_users_obj" care/security/authorization/Repository: ohcnetwork/care
Length of output: 2040
🏁 Script executed:
# Check the complete facilityorganization authorization file
cat care/security/authorization/facilityorganization.pyRepository: ohcnetwork/care
Length of output: 8034
🏁 Script executed:
# Look for any tests that verify service account filtering behavior
fd -t f "test.*facility_organization" care/ | xargs grep -l "is_service_account"Repository: ohcnetwork/care
Length of output: 43
No additional authorization controls exist for service account visibility.
The get_queryset method applies the same can_list_facility_organization_users_obj permission check for all organization users, regardless of whether service accounts are being requested. The code doesn't enforce additional authorization when is_service_account=true is passed—any user with the general list permission can reveal service accounts. While the default behavior (excluding service accounts unless explicitly requested) is sensible, consider adding a dedicated permission check if service account visibility should be restricted beyond the standard organization user list permissions.
🤖 Prompt for AI Agents
In @care/emr/api/viewsets/facility_organization.py around lines 379-386,
get_queryset currently filters service accounts only by query param but does not
enforce extra authorization when is_service_account=true; update the method
(get_queryset in FacilityOrganization viewset) to detect when
self.request.query_params.get("is_service_account") is truthy and then perform
an explicit permission check (e.g., call a new or existing permission like
can_view_service_accounts or use request.user.has_perm/
self.check_object_permissions with can_list_facility_organization_users_obj
scoped to service accounts) and raise PermissionDenied if the check fails,
otherwise proceed to include service accounts; ensure the permission symbol
referenced is unique (can_list_facility_organization_users_obj or a new
can_view_service_accounts) so reviewers can find and wire the check where the
queryset is built.
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @care/emr/api/viewsets/organization.py:
- Around line 326-328: Add tests for OrganizationViewSet filtering behavior:
create a regular user and a service account user, call the organizations
endpoint (via the OrganizationViewSet/get_queryset path) without the
is_service_account param and assert the service account is excluded; call with
is_service_account=true and assert the service account is present; call with
is_service_account=false and assert the service account is excluded. Use the
existing test fixtures/auth helpers and DRF APIClient to make requests and
assert on returned IDs or counts so the three cases (absent, true, false) are
covered.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
care/emr/api/viewsets/organization.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance).
Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables).
Files:
care/emr/api/viewsets/organization.py
🧬 Code graph analysis (1)
care/emr/api/viewsets/organization.py (1)
care/emr/models/organization.py (1)
OrganizationUser(110-121)
⏰ 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). (1)
- GitHub Check: Test / Test
🔇 Additional comments (1)
care/emr/api/viewsets/organization.py (1)
228-228: Service account filter is correct and consistent.The filter syntax is properly structured using Django's field lookup notation, and the
User.is_service_accountfield exists in the model with an appropriate migration. The pattern aligns with the existing implementation infacility_organization.pyand follows the same default exclusion behavior for service accounts.
| if "is_service_account" not in self.request.query_params: | ||
| queryset = queryset.filter(user__is_service_account=False) | ||
| return queryset |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Get file info and full method context
echo "=== File Info ==="
wc -l care/emr/api/viewsets/organization.py
echo -e "\n=== Full method context around lines 326-328 ==="
sed -n '300,340p' care/emr/api/viewsets/organization.py | cat -n
echo -e "\n=== Search for OrganizationUser tests ==="
find . -path ./node_modules -prune -o -type f -name '*.py' -exec grep -l 'OrganizationUser' {} \; | grep -i test | head -20Repository: ohcnetwork/care
Length of output: 1747
🏁 Script executed:
#!/bin/bash
# Search for tests using is_service_account parameter or filter
echo "=== Tests with is_service_account ==="
rg -n 'is_service_account' --type=py -g '**/test*.py' -B2 -A2 | head -100
echo -e "\n=== Tests for OrganizationUserViewSet ==="
rg -n 'OrganizationUser.*[Vv]iew' --type=py -g '**/test*.py' -B2 -A5 | head -150Repository: ohcnetwork/care
Length of output: 139
🏁 Script executed:
#!/bin/bash
# Check for any filtering behavior tests or documentation
echo "=== Check for get_queryset or filter tests in organization tests ==="
fd -type f -name '*organization*test*.py' --exec cat {} \; 2>/dev/null | head -300
echo -e "\n=== Look for API documentation ==="
find . -type f \( -name '*.md' -o -name '*.rst' \) | xargs grep -l 'service.*account' 2>/dev/null | head -10Repository: ohcnetwork/care
Length of output: 163
🏁 Script executed:
#!/bin/bash
# Check the actual test files for relevant tests
echo "=== test_organization_api.py content ==="
wc -l ./care/emr/tests/test_organization_api.py
echo "---"
cat ./care/emr/tests/test_organization_api.py | head -200
echo -e "\n=== Search for any is_service_account usage in tests ==="
grep -r "is_service_account" ./care/emr/tests/ 2>/dev/null
echo -e "\n=== Search for any service_account references in tests ==="
grep -r "service_account" ./care/emr/tests/ 2>/dev/null | head -20Repository: ohcnetwork/care
Length of output: 8224
🏁 Script executed:
#!/bin/bash
# Check if there are any tests for get_queryset or filtering in organization tests
echo "=== Look for filter or queryset tests ==="
grep -n "queryset\|filter" ./care/emr/tests/test_organization_api.py | head -30
echo -e "\n=== Check the full organization test file for related tests ==="
cat ./care/emr/tests/test_organization_api.py | tail -200Repository: ohcnetwork/care
Length of output: 8939
🏁 Script executed:
#!/bin/bash
# Search for any mention of service accounts in the codebase
echo "=== OrganizationUser model definition ==="
rg -n 'class OrganizationUser' --type=py -A10
echo -e "\n=== Check if there are migration files or comments about this feature ==="
fd '*.py' -x grep -l 'service_account' {} \; | grep -E 'migration|model'Repository: ohcnetwork/care
Length of output: 7563
🏁 Script executed:
#!/bin/bash
# Check git history to understand if this is new or modified code
echo "=== Check recent commits to organization.py ==="
cd care/emr/api/viewsets && git log --oneline -20 organization.py 2>/dev/null | head -20
echo -e "\n=== Check git blame for lines around 327 ==="
cd care/emr/api/viewsets && git blame -L320,330 organization.py 2>/dev/null
echo -e "\n=== Search for any TODO or FIXME comments related to service accounts ==="
rg -n 'TODO|FIXME' care/emr/api/viewsets/organization.pyRepository: ohcnetwork/care
Length of output: 440
🏁 Script executed:
#!/bin/bash
# Look for any other references to this behavior or discussions
echo "=== Check if User model has is_service_account field ==="
rg -n 'is_service_account' care/users/models.py -A2 -B2 | head -30
echo -e "\n=== Check if there are integration/e2e tests that might cover this ==="
find . -path ./node_modules -prune -o -type f -name '*.py' -exec grep -l 'organization.*user' {} \; | xargs grep -l 'test\|spec' | grep -i test | head -10Repository: ohcnetwork/care
Length of output: 1040
🏁 Script executed:
#!/bin/bash
# Verify the actual test cases more carefully - are there service account tests anywhere?
echo "=== Search all tests for service account references ==="
rg -n 'service_account' . --type=py -g '**/test*.py' 2>/dev/null | head -50
echo -e "\n=== Check if there are any fixture factories creating service accounts ==="
rg -n 'create.*user\|factory' care/utils/tests/base.py -A5 | head -100Repository: ohcnetwork/care
Length of output: 190
Add comprehensive test coverage for the service account filtering behavior.
This newly added filtering logic excludes service accounts by default unless explicitly requested via the is_service_account query parameter. While the implementation is clean, there's zero test coverage for either scenario—neither the default exclusion case nor the explicit inclusion case when the parameter is provided.
Given this is a recent addition that changes the endpoint's default response, add tests that verify:
- Service accounts are excluded when
is_service_accountparameter is absent - Service accounts are included when
is_service_account=trueis explicitly passed - Service accounts are excluded when
is_service_account=falseis explicitly passed
No documentation updates were found mentioning this new filtering behavior either.
🤖 Prompt for AI Agents
In @care/emr/api/viewsets/organization.py around lines 326-328, Add tests for
OrganizationViewSet filtering behavior: create a regular user and a service
account user, call the organizations endpoint (via the
OrganizationViewSet/get_queryset path) without the is_service_account param and
assert the service account is excluded; call with is_service_account=true and
assert the service account is present; call with is_service_account=false and
assert the service account is excluded. Use the existing test fixtures/auth
helpers and DRF APIClient to make requests and assert on returned IDs or counts
so the three cases (absent, true, false) are covered.
Proposed Changes
Associated Issue
Architecture changes
Merge Checklist
/docsOnly PR's with test cases included and passing lint and test pipelines will be reviewed
@ohcnetwork/care-backend-maintainers @ohcnetwork/care-backend-admins
Summary by CodeRabbit
New Features
Security
Chores
✏️ Tip: You can customize this high-level summary in your review settings.