project(ui): confine users to projects and standardize slug usage #154
Conversation
psyray
changed the title
|
@AnonymousWP Could you test the functionality on your side. |
AnonymousWP
force-pushed
the
71-confine-user-to-project
branch
from
db591cd to
6ce133b
Compare
…s-Alliance/rengine-ng into 71-confine-user-to-project
|
I've tested the feature
Thus being said on first login for a new user Maybe Users should be redirected to a project they are allowed to access But it is a matter of UX |
|
Thanks for testing
You're right I will modify it. |
|
@sourcery-ai review |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @psyray - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding more comprehensive unit tests to cover the new project access control functionality, especially in the ProjectAccessMiddleware and SlugMiddleware.
- It might be beneficial to add more detailed inline documentation for some of the more complex functions, particularly in the dashboard/utils.py file, to explain the logic behind the project access checks.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟡 Security: 3 issues found
- 🟡 Testing: 7 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
- Added custom_filters template tag loading in various templates. - Refactored user management functionalities in admin.html for better readability and maintainability. - Updated update_user_modal and create_user_modal functions to handle project selection based on user roles. - Enhanced error handling in user management views. - Removed custom_tags template tag loading from multiple templates. - Deleted custom_tags.py files from startScan and scanEngine apps.
- Refactored multiple views to improve code readability and maintainability. - Consolidated repetitive code into helper functions. - Enhanced error handling and logging for better debugging. - Updated test cases to reflect changes in views and added new tests for better coverage. - Adjusted middleware order in settings for better functionality. - Improved cookie handling for project selection with enhanced security attributes.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- Added logging to gpt.py for better error tracking. - Refined error handling in GPTAttackSuggestionGenerator to log errors and return a generic error message. - Minor code cleanup in views.py and test_dashboard.py. - Improved error message in handle_delete_user function in views.py.
There was a problem hiding this comment.
Hey @psyray - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding caching mechanisms to the new middleware, especially for project access checks, to optimize performance.
- Ensure thorough testing of the new project access controls and URL routing changes to prevent any regressions.
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 8 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| except Exception as e: | ||
| response['error'] = str(e) | ||
| return Response(response, status=400) | ||
| def get(self, request): |
There was a problem hiding this comment.
suggestion: Improve error handling and response structure in OllamaManager GET method
Consider using a consistent error response structure and add logging for better debugging. This will make the API more robust and easier to maintain.
def get(self, request):
try:
model_name = request.query_params.get('model')
if not model_name:
return Response({'error': 'Model name is required'}, status=400)
# Rest of the method implementation
except Exception as e:
logger.error(f"Error in OllamaManager GET: {str(e)}")
return Response({'error': 'An unexpected error occurred'}, status=500)
|
|
||
| delete_model_api = f'{OLLAMA_INSTANCE}/api/delete' | ||
|
|
||
| class GPTAttackSuggestion(APIView): |
There was a problem hiding this comment.
suggestion: Enhance error handling in GPTAttackSuggestion class
Implement more robust error handling, particularly for API calls and data processing. This will improve the reliability of the attack suggestion feature.
class GPTAttackSuggestion(APIView):
def get(self, request):
try:
# API call and data processing logic here
pass
except Exception as e:
return Response({'error': str(e)}, status=status.HTTP_500_INTERNAL_SERVER_ERROR)
| insert_date = models.DateTimeField() | ||
| id = models.AutoField(primary_key=True) | ||
| name = models.CharField(max_length=500) | ||
| description = models.TextField(blank=True, null=True) |
There was a problem hiding this comment.
suggestion (performance): Consider adding a max_length to the description field
While TextField doesn't require a max_length, it's often good practice to set one to prevent excessively long descriptions. This can help with database optimization and front-end display.
| description = models.TextField(blank=True, null=True) | |
| description = models.TextField(max_length=5000, blank=True, null=True) |
| from django.shortcuts import get_object_or_404 | ||
| from .models import Project | ||
|
|
||
| class ProjectAccessMiddleware: |
There was a problem hiding this comment.
suggestion: Consider adding logging to ProjectAccessMiddleware
Adding logging to this middleware could help with debugging and monitoring access control issues in production. Consider logging attempts to access projects without proper permissions.
import logging
logger = logging.getLogger(__name__)
class ProjectAccessMiddleware:
def __init__(self, get_response):
self.get_response = get_response
logger.info("ProjectAccessMiddleware initialized")
| def test_profile_view(self): | ||
| """Test the profile view.""" | ||
| response = self.client.get(reverse('profile', kwargs={'slug': self.data_generator.project.slug})) | ||
| response = self.client.get(reverse('profile')) # Suppression du paramètre 'slug' | ||
| self.assertEqual(response.status_code, 200) | ||
| self.assertIn('form', response.context) | ||
| self.assertEqual(response.context['current_project'], self.data_generator.project) | ||
| self.assertTemplateUsed(response, 'dashboard/profile.html') # Vérification du modèle utilisé |
There was a problem hiding this comment.
suggestion (testing): Update profile view test to remove slug parameter and add template assertion
The test has been updated to reflect changes in the view, but it might be beneficial to add more assertions to verify the content of the response.
| def test_profile_view(self): | |
| """Test the profile view.""" | |
| response = self.client.get(reverse('profile', kwargs={'slug': self.data_generator.project.slug})) | |
| response = self.client.get(reverse('profile')) # Suppression du paramètre 'slug' | |
| self.assertEqual(response.status_code, 200) | |
| self.assertIn('form', response.context) | |
| self.assertEqual(response.context['current_project'], self.data_generator.project) | |
| self.assertTemplateUsed(response, 'dashboard/profile.html') # Vérification du modèle utilisé | |
| def test_profile_view(self): | |
| """Test the profile view.""" | |
| response = self.client.get(reverse('profile')) | |
| self.assertEqual(response.status_code, 200) | |
| self.assertTemplateUsed(response, 'dashboard/profile.html') | |
| self.assertContains(response, 'Profile') | |
| self.assertContains(response, self.user.username) | |
| self.assertContains(response, self.user.email) |
| self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) | ||
| self.assertEqual(response.data["message"], "pull model manifest: file does not exist") |
There was a problem hiding this comment.
suggestion (testing): Update assertion for API response in case of model download failure
The test has been updated to expect a 400 status code instead of 200. Ensure this change aligns with the actual API behavior.
| if request.method == "POST": | ||
| headers = { | ||
| 'Accept': 'application/json' | ||
| } | ||
| body = json.loads(request.body) | ||
| r = requests.get( | ||
| response = requests.get( | ||
| 'https://api.hackerone.com/v1/hackers/payments/balance', | ||
| auth=(body['username'], body['api_key']), | ||
| headers = headers | ||
| headers={'Accept': 'application/json'} | ||
| ) | ||
| if r.status_code == 200: | ||
| return http.JsonResponse({"status": 200}) | ||
|
|
||
| return http.JsonResponse({"status": response.status_code}) |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
|
|
||
| most_common_vulnerabilities = list(most_common_vulnerabilities) | ||
| try: | ||
| limit = safe_int_cast(data.get('limit', 20)) |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Use named expression to simplify assignment and conditional (
use-named-expression) - Replace if statement with if expression (
assign-if-exp) - Extract code out into method (
extract-method)
| ) | ||
| from dashboard.models import Project, OpenAiAPIKey, NetlasAPIKey | ||
| from dashboard.forms import ProjectForm | ||
| from reNgine.definitions import PERM_MODIFY_SYSTEM_CONFIGURATIONS, FOUR_OH_FOUR_URL | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| def index(request, slug): |
There was a problem hiding this comment.
issue (code-quality): Low code quality found in index - 18% (low-code-quality)
Explanation
The quality score for this function is below the quality threshold of 25%.This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines. - Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.
| @has_permission_decorator(PERM_MODIFY_WORDLISTS, redirect_url=FOUR_OH_FOUR_URL) | ||
| def add_wordlist(request, slug): | ||
| def add_wordlist(request): | ||
| context = {'scan_engine_nav_active': 'active', 'wordlist_li': 'active'} |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Move assignment closer to its usage within a block (
move-assign-in-block) - Swap positions of nested conditionals (
swap-nested-ifs) - Merge dictionary assignment with declaration (
merge-dict-assign)
|
You could test and merge |
|
Does it need to be re-tested? Considering @0b3ud already tested it. |
I will retest it to be sure and then re review the code :) Update :
|
|
Then you can approve. @psyray needs to fix merge conflicts, then it can be squashed and merged due to many commits (keeps history clean). |
Fixed |
There was a problem hiding this comment.
While reviewing the code found a new bug : (
It on admin interface page
When you try to disable a user you get redirected to this page instead of being redirected back to admin interface page
The feature still works fine, the users are disable and enabled but u need to manually go back to admin interface
- Modified the handle_get_request function to redirect to the admin interface and provide user activation feedback. - Updated test cases to reflect changes in response status codes and added a new test for invalid mode handling.
Fixed |
There was a problem hiding this comment.
Hello :)
Found another bug on this PR
When you select a projet (landing on the projects dashboard), and u go on another page the project automatically changes to the first project that the user is assigned to.
Example :
-Choose any project in your list except for the first one
-Click on targets per example
-Look at the difference in URLS
Let me know if you need any further details
Thanks in advance
…n logic - Removed JavaScript function for setting the current project and its associated event listeners. - Added a new endpoint in the backend to handle setting the current project via a GET request. - Updated the project selection dropdown to use the new backend endpoint.
Fixed, you can test |
|
Next time, please squash and merge if it has so many commits. |
Could you explain how to do this, or a link to a doc |
|
fixes #71
fixes #173
I've added the possibility to confine users to projects
User creation/update with projects to assign
Project creation/update with users to assign
If user try to access a non assign project it returns a 403 page
Project display is restricted to their assigned projects for user
I've also remove the * import for the views.py in favor of defined import
It's a bad practice to use *
If someone could also test on his side it could be good
Summary by Sourcery
Confine users to specific projects and standardize slug usage across the application. Introduce project-based user management, restrict project access, and enhance user interface for project and user management. Refactor code for improved readability and maintainability, and update tests to cover new features.
New Features:
Bug Fixes:
Enhancements:
Tests:
Chores: