-
Notifications
You must be signed in to change notification settings - Fork 2
BENCH-418 & BENCH-422: Authentication and permissions #90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
…Views to aid permissions setting
Still need to update tests. |
@@ -1,11 +1,17 @@ | |||
import re | |||
from abc import ABC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this?
pass | ||
|
||
|
||
class IsManagerUser(BasePermission): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the same as IsStaffUser so can get rid of this
@@ -118,7 +118,7 @@ | |||
problem_detail_example: ProblemDetails = { | |||
"errors": {"name": "The name field is required."}, | |||
"type": "https://testserver/problems/error/", | |||
"title": "One or more validation errors occurred.", | |||
"title": "One or more permissions errors occurred.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably would change this to "One or more validation or permissions errors occurred."
class RegisterManagerView( | ||
generics.CreateAPIView, | ||
): | ||
permission_classes = (AllowAny,) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be "IsStaffUser" rather than AllowAny?
mixins.CreateModelMixin, | ||
generics.GenericAPIView, | ||
): | ||
permission_classes = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this have "IsStaffUser" s its permission?
mixins.UpdateModelMixin, | ||
generics.GenericAPIView, | ||
): | ||
permission_classes = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this have "IsStaffUser" as its permission?
RetireMixin, | ||
generics.GenericAPIView, | ||
): | ||
permission_classes = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this have "IsStaffUser" as its permission?
depth = 3 | ||
|
||
|
||
class ManagerAuthSerializer(serializers.ModelSerializer): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are going with the permission as "IsStaffUser" we may want to change this to StaffAuthSerializer. Similar with a few other naming conventions further down also.
"password", | ||
"password2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 single password should be sent, password confirmation should be done by the front end
No description provided.