Stop volunteers from accessing admin urls. #406
Conversation
There was a problem hiding this comment.
@necessary129 I get a no admin rights page now, so good job on that front! The only issue is with the change breaking tests. My guess is that it is because of the inability to access administrator sign up without being logged in. That is causing all the Admin sign up tests to fail.
Could you please remove the changes related to the last url? (/registration/signup_administrator/)
Also, don't create any commit for that. You can try rebasing or create another PR with the required changes, if it seems too complex.
necessary129
force-pushed
the
adm-acc-fix
branch
from
deecbd4 to
0c02af8
Compare
|
@smarshy Is this what you needed? (0c02af8) |
| @@ -16,7 +16,7 @@ | |||
| from administrator.models import * | |||
|
|
|||
|
|
|||
| class AdministratorSignupView(TemplateView): | |||
| class AdministratorSignupView(AnonymousRequiredMixin, TemplateView): | |||
There was a problem hiding this comment.
@necessary129 Is there a reason why this mixin should be added here?
There was a problem hiding this comment.
@smarshy Well... It is the default Mixin used to prevent logged in users to access that view; i thought the task said to make that view inaccessible to logged in users, so did that.
There was a problem hiding this comment.
@necessary129 No, that view should be accessible to the admin and not the volunteer. A logged in admin should be able to access. Please remove the mixin .
|
Coverage increased (+0.06%) to 91.413% when pulling 0c02af8fb0eab2768d15e0d076f7d45efc164a34 on necessary129:adm-acc-fix into 3804832 on systers:develop. |
There was a problem hiding this comment.
@necessary129 If you check on your dashboard now, the admin themselves are not able to create another admin account. This is incorrect behaviour. What is needed is that the volunteer should not be able to access the create admin tab while the admin can do so.
Hence, please remove the changes for 'registration/signup_administrator'. This url access can be fixed only when 'create admin account' redirects to a different url and view, where AdministratorLoginRequiredMixin can be used.
| @@ -81,6 +77,7 @@ def post(self, request, *args, **kwargs): | |||
|
|
|||
|
|
|||
| @login_required | |||
| @admin_required | |||
| def settings(request): | |||
| user = request.user | |||
| admin = None | |||
There was a problem hiding this comment.
@necessary129 Do we need to check if the user is admin in the settings view now? I mean after putting up the decorator, is there a need of try except block?
There was a problem hiding this comment.
@mayank-agarwal-96 Good catch!
necessary129
force-pushed
the
adm-acc-fix
branch
from
0c02af8 to
9e0293f
Compare
|
@smarshy What about now? |
necessary129
force-pushed
the
adm-acc-fix
branch
from
9e0293f to
3bc4ea5
Compare
There was a problem hiding this comment.
@necessary129 The tests are failing again and it is due to the same reason as last time. Remove the mixin from the previous PR, take into account suggestions by @mayank-agarwal-96 and we are good to go.
I will open a separate issue for the create admin account. If you do want to fix that -
- either map that tab to a new url, view where you can use the mixin
- or code logic such that no admin rights page is displayed when he user is logged in but is not an administrator.
necessary129
force-pushed
the
adm-acc-fix
branch
from
3bc4ea5 to
471770b
Compare
|
Coverage increased (+0.02%) to 91.373% when pulling 471770ba314e604bc35f08045306307f014cb355 on necessary129:adm-acc-fix into 3804832 on systers:develop. |
|
@smarshy Fixed that, and also did the no.2 suggestion you made 😄 |
|
@necessary129 I will review this by the evening |
| @@ -31,6 +31,9 @@ class AdministratorSignupView(TemplateView): | |||
| phone_error = False | |||
|
|
|||
| def get(self, request): | |||
| if request.user.is_authenticated(): | |||
| if not hasattr(request.user, 'administrator'): | |||
There was a problem hiding this comment.
@necessary129 Are you trying to protect get and post method for admin? You can have a look at this link for decorating class based views : https://docs.djangoproject.com/en/1.10/topics/class-based-views/intro/#decorating-class-based-views
There was a problem hiding this comment.
@mayank-agarwal-96 Well... I thought as it is only needed for this view, i would just protect it by adding code in both the functions.
There was a problem hiding this comment.
I hope you understand that it leads to redundant code.
necessary129
force-pushed
the
adm-acc-fix
branch
from
471770b to
a830118
Compare
|
Coverage increased (+0.1%) to 91.461% when pulling a8301187ee35470b54913c0a60e371f17a7db715 on necessary129:adm-acc-fix into 3804832 on systers:develop. |
There was a problem hiding this comment.
@necessary129 This is working fine now. Thanks! :) I have added another url for which access needs to be denied in the issue. If possible, please include that too in this PR.
@mayank-agarwal-96 The PR has been updated. Any other improvements that could be made?
necessary129
force-pushed
the
adm-acc-fix
branch
from
a830118 to
e440df8
Compare
|
@smarshy Did that too. |
Fixes #325