Add Admin Password Reset Functionality#3
Conversation
Co-authored-by: magi-9 <156084637+magi-9@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Pull request overview
This pull request adds functionality for administrators to manually reset user passwords through the Admin Panel. The implementation includes backend API endpoints with proper permission checks, frontend UI components with a modal interface, and comprehensive test coverage.
Changes:
- Added backend endpoint
admin_reset_passwordwithIsAdminUserpermission andAdminPasswordResetSerializerfor validation - Added frontend "Reset Password" button (Key icon) and modal to the Admin Panel user cards
- Added comprehensive backend tests covering permissions, validation, and edge cases
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/users/views.py | Added admin_reset_password action with admin-only permissions |
| backend/users/serializers.py | Added AdminPasswordResetSerializer with 8-character minimum validation |
| backend/users/tests.py | Added comprehensive test suite covering admin permissions, non-admin restrictions, validation, and authentication |
| backend/config/test_settings.py | Added test configuration using SQLite for faster test execution |
| frontend/src/utils/api.js | Added adminResetPassword API method |
| frontend/src/components/admin/AdminUsers.jsx | Added reset password button, modal UI, and mutation handling with validation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| onSuccess: () => { | ||
| toast.success('Heslo bolo úspešne zmenené'); | ||
| setResetPasswordUser(null); | ||
| setNewPassword(''); |
There was a problem hiding this comment.
The resetPasswordMutation is missing an onError handler. If the API request fails (e.g., network error, validation error from backend, permission denied), the user won't receive any feedback about what went wrong. Add an onError handler to display appropriate error messages to the user, similar to how other mutations in this codebase don't have explicit onError handlers either. Consider adding error handling here since password reset is a sensitive operation where clear feedback is important.
| setNewPassword(''); | |
| setNewPassword(''); | |
| }, | |
| onError: (error) => { | |
| const message = | |
| error?.response?.data?.detail || | |
| error?.response?.data?.message || | |
| error?.message || | |
| 'Nepodarilo sa zmeniť heslo. Skúste to znova.'; | |
| toast.error(message); |
| class AdminPasswordResetSerializer(serializers.Serializer): | ||
| new_password = serializers.CharField(required=True, min_length=8) |
There was a problem hiding this comment.
The AdminPasswordResetSerializer only validates the minimum length of 8 characters but doesn't use Django's built-in password validators (UserAttributeSimilarityValidator, CommonPasswordValidator, NumericPasswordValidator) that are configured in settings.py. This means admins can set weak passwords like "password" or "12345678" that would normally be rejected. Consider using Django's validate_password function from django.contrib.auth.password_validation to enforce the same password strength requirements as user registration. For reference, UserRegistrationSerializer at line 27 also uses min_length=8 but creates users through create_user which may apply validators differently.
| def admin_reset_password(self, request, pk=None): | ||
| """Admin resets password for a user""" | ||
| user = self.get_object() | ||
| serializer = AdminPasswordResetSerializer(data=request.data) | ||
| serializer.is_valid(raise_exception=True) | ||
|
|
||
| user.set_password(serializer.validated_data['new_password']) | ||
| user.save() | ||
|
|
||
| return Response({'message': f'Heslo pre používateľa {user.username} bolo úspešne zmenené.'}, status=status.HTTP_200_OK) |
There was a problem hiding this comment.
The backend allows an admin to reset their own password through this endpoint, but the frontend UI prevents this by checking user.id !== currentUser.id at line 198. This creates an inconsistency where the API allows something the UI prevents. Consider adding a backend validation to prevent admins from resetting their own password for security reasons (they should use the regular change_password endpoint which requires the old password). Add a check like: if user.id == request.user.id: raise ValidationError("Cannot reset your own password. Use the change password feature instead.")
| def test_unauthenticated_cannot_reset_password(self): | ||
| """Unauthenticated user should not be able to reset password""" | ||
| url = reverse('user-admin-reset-password', kwargs={'pk': self.user.id}) | ||
| data = {'new_password': 'hackedpassword'} | ||
|
|
||
| response = self.client.post(url, data) | ||
| self.assertEqual(response.status_code, status.HTTP_401_UNAUTHORIZED) |
There was a problem hiding this comment.
Missing test case: The test suite doesn't verify the behavior when an admin tries to reset their own password. Given the UI prevents this but the backend doesn't (see comment on views.py), add a test case to document the expected behavior. For example: test_admin_cannot_reset_own_password (if the backend validation is added) or test_admin_can_reset_own_password (if this is intentionally allowed).
| <div> | ||
| <label className="block text-sm font-bold text-gray-700 mb-1">Nové heslo</label> | ||
| <input | ||
| type="text" |
There was a problem hiding this comment.
The password input field uses type="text" which displays the password in plain text. This is a security concern as the password becomes visible to anyone who can see the screen. Change the input type to "password" to mask the password characters. While this is an admin interface, it's still best practice to hide passwords during entry.
| type="text" | |
| type="password" |
| from .settings import * | ||
|
|
||
| DATABASES = { | ||
| 'default': { | ||
| 'ENGINE': 'django.db.backends.sqlite3', | ||
| 'NAME': BASE_DIR / 'db.sqlite3', |
There was a problem hiding this comment.
Import pollutes the enclosing namespace, as the imported module config.settings does not define 'all'.
| from .settings import * | |
| DATABASES = { | |
| 'default': { | |
| 'ENGINE': 'django.db.backends.sqlite3', | |
| 'NAME': BASE_DIR / 'db.sqlite3', | |
| from . import settings | |
| DATABASES = { | |
| 'default': { | |
| 'ENGINE': 'django.db.backends.sqlite3', | |
| 'NAME': settings.BASE_DIR / 'db.sqlite3', |
Co-authored-by: magi-9 <156084637+magi-9@users.noreply.github.com>
Implemented functionality for administrators to reset user passwords manually via the Admin Panel.
admin_reset_passwordaction toUserViewSetwithIsAdminUserpermission. AddedAdminPasswordResetSerializer.backend/users/tests.pycovering permissions and validation.PR created automatically by Jules for task 17660621611443880696 started by @magi-9