Permission class added for dynamics #58
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ruuushhh
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
WalkthroughThe update to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on X ? TipsChat with CodeRabbit Bot (
|
ruuushhh
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
|
ruuushhh
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (1)
- apps/workspaces/permissions.py (1 hunks)
Additional comments: 1
apps/workspaces/permissions.py (1)
- 1-52: Verify that the PR title accurately reflects the changes made. The title suggests a new permission class has been added, but the provided hunk and summary indicate the modification and removal of existing classes.
Comment on lines
+12
to
+34
| class WorkspacePermissions(permissions.BasePermission): | ||
| """ | ||
| Permission check for users <> workspaces | ||
| """ | ||
|
|
||
| def validate_and_cache(self, workspace_users, user: User, workspace_id: str, cache_users: bool = False): | ||
| if user.id in workspace_users: | ||
| if cache_users: | ||
| cache.set(workspace_id, workspace_users, 172800) | ||
| return True | ||
|
|
||
| return False | ||
|
|
||
| def has_permission(self, request, view): | ||
| workspace_id = str(view.kwargs.get('workspace_id')) | ||
| user = request.user | ||
| workspace_users = cache.get(workspace_id) | ||
|
|
||
| if workspace_users: | ||
| return self.validate_and_cache(workspace_users, user, workspace_id) | ||
| else: | ||
| workspace_users = Workspace.objects.filter(pk=workspace_id).values_list('user', flat=True) | ||
| return self.validate_and_cache(workspace_users, user, workspace_id, True) |
There was a problem hiding this comment.
The WorkspacePermissions class introduces caching logic for workspace users. Ensure that the cache key is unique and cannot collide with other cache entries. Also, consider handling potential exceptions when interacting with the cache or the database.
+ try:
+ # Existing code for caching and database interaction
+ except SomeSpecificException as e:
+ # Handle exceptions appropriately
+ passCommittable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| class WorkspacePermissions(permissions.BasePermission): | |
| """ | |
| Permission check for users <> workspaces | |
| """ | |
| def validate_and_cache(self, workspace_users, user: User, workspace_id: str, cache_users: bool = False): | |
| if user.id in workspace_users: | |
| if cache_users: | |
| cache.set(workspace_id, workspace_users, 172800) | |
| return True | |
| return False | |
| def has_permission(self, request, view): | |
| workspace_id = str(view.kwargs.get('workspace_id')) | |
| user = request.user | |
| workspace_users = cache.get(workspace_id) | |
| if workspace_users: | |
| return self.validate_and_cache(workspace_users, user, workspace_id) | |
| else: | |
| workspace_users = Workspace.objects.filter(pk=workspace_id).values_list('user', flat=True) | |
| return self.validate_and_cache(workspace_users, user, workspace_id, True) | |
| class WorkspacePermissions(permissions.BasePermission): | |
| """ | |
| Permission check for users <> workspaces | |
| """ | |
| def validate_and_cache(self, workspace_users, user: User, workspace_id: str, cache_users: bool = False): | |
| try: | |
| if user.id in workspace_users: | |
| if cache_users: | |
| cache.set(workspace_id, workspace_users, 172800) | |
| return True | |
| return False | |
| except SomeSpecificException as e: | |
| # Handle exceptions appropriately | |
| pass | |
| def has_permission(self, request, view): | |
| try: | |
| workspace_id = str(view.kwargs.get('workspace_id')) | |
| user = request.user | |
| workspace_users = cache.get(workspace_id) | |
| if workspace_users: | |
| return self.validate_and_cache(workspace_users, user, workspace_id) | |
| else: | |
| workspace_users = Workspace.objects.filter(pk=workspace_id).values_list('user', flat=True) | |
| return self.validate_and_cache(workspace_users, user, workspace_id, True) | |
| except SomeSpecificException as e: | |
| # Handle exceptions appropriately | |
| pass |
Comment on lines
+37
to
+52
| class IsAuthenticatedForTest(permissions.BasePermission): | ||
| """ | ||
| Custom auth for preparing a workspace for e2e tests | ||
| """ | ||
|
|
||
| def has_permission(self, request, view): | ||
| # Client sends a token in the header, which we decrypt and compare with the Client Secret | ||
| cipher_suite = Fernet(settings.ENCRYPTION_KEY) | ||
| try: | ||
| decrypted_password = cipher_suite.decrypt(request.headers['X-E2E-Tests-Client-ID'].encode('utf-8')).decode('utf-8') | ||
| if decrypted_password == settings.E2E_TESTS_CLIENT_SECRET: | ||
| return True | ||
| except Exception: | ||
| return False | ||
|
|
||
| return False |
There was a problem hiding this comment.
The IsAuthenticatedForTest class uses encryption keys to decrypt a client ID. Ensure that the ENCRYPTION_KEY and E2E_TESTS_CLIENT_SECRET are securely managed and that the decryption process does not introduce security vulnerabilities. Additionally, catching a generic Exception is not a best practice; catch specific exceptions instead.
[SECURITY]
- except Exception:
+ except (Fernet.InvalidToken, KeyError, AttributeError) as e:
+ # Log the error for debugging purposes
+ logger.error(f"Error during decryption: {e}")Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| class IsAuthenticatedForTest(permissions.BasePermission): | |
| """ | |
| Custom auth for preparing a workspace for e2e tests | |
| """ | |
| def has_permission(self, request, view): | |
| # Client sends a token in the header, which we decrypt and compare with the Client Secret | |
| cipher_suite = Fernet(settings.ENCRYPTION_KEY) | |
| try: | |
| decrypted_password = cipher_suite.decrypt(request.headers['X-E2E-Tests-Client-ID'].encode('utf-8')).decode('utf-8') | |
| if decrypted_password == settings.E2E_TESTS_CLIENT_SECRET: | |
| return True | |
| except Exception: | |
| return False | |
| return False | |
| class IsAuthenticatedForTest(permissions.BasePermission): | |
| """ | |
| Custom auth for preparing a workspace for e2e tests | |
| """ | |
| def has_permission(self, request, view): | |
| # Client sends a token in the header, which we decrypt and compare with the Client Secret | |
| cipher_suite = Fernet(settings.ENCRYPTION_KEY) | |
| try: | |
| decrypted_password = cipher_suite.decrypt(request.headers['X-E2E-Tests-Client-ID'].encode('utf-8')).decode('utf-8') | |
| if decrypted_password == settings.E2E_TESTS_CLIENT_SECRET: | |
| return True | |
| except (Fernet.InvalidToken, KeyError, AttributeError) as e: | |
| # Log the error for debugging purposes | |
| logger.error(f"Error during decryption: {e}") | |
| return False | |
| return False |
labhvam5
approved these changes
Dec 20, 2023
ashwin1111
approved these changes
Dec 20, 2023
anishfyle
approved these changes
Dec 20, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
DEFAULT_PERMISSION_CLASSESinsettings.py.WorkspacePermissionsclass inpermissions.pyfor workspace permission checks.IsAuthenticatedForTestclass inpermissions.pyfor custom authentication during end-to-end tests.