Stop volunteers from accessing each other's urls. #408
Conversation
necessary129
force-pushed
the
vol-acc-fix
branch
from
10f53d1 to
cf35353
Compare
|
Coverage increased (+0.2%) to 91.557% when pulling cf35353b140089e2e4cdfc88520c98da5a2347d1 on necessary129:vol-acc-fix into 3804832 on systers:develop. |
| def vol_id_check(func): | ||
| @wraps(func) | ||
| def wrapped_view(request, volunteer_id): | ||
| vol = getattr(request.user, 'volunteer', hasattr(request.user, 'administrator')) |
There was a problem hiding this comment.
@necessary129 Why use a default value here?
There was a problem hiding this comment.
@smarshy Because it also checks if the user is an admin. if the user is a volunteer,it returns the volunteer object, if an admin, it returns true, and if not both, false
There was a problem hiding this comment.
@necessary129 Yes, but what I meant was why not simply check if the user has volunteer attribute. If they don't , then simply display the no rights page. And in case he/she is a volunteer, the ids are checked. Did you intend to allow the administrator to view these pages? Because currently, they are able to and you haven't handled the case where vol=True (i.e. he/she is an administrator)
Don't make changes yet for this, I just want to know your views on it. :)
There was a problem hiding this comment.
Aah, I thought the admin also needed to access this, heh :P
| return render(request, 'vms/no_volunteer_access.html', status=403) | ||
| elif vol != True: | ||
| volunteer = get_volunteer_by_id(volunteer_id) | ||
| if not volunteer: |
There was a problem hiding this comment.
@necessary129 Why have this? It reveals to the user the number of volunteers present in the system. Why not simply display a no volunteers page if the ids don't match?
| vol = getattr(request.user, 'volunteer', hasattr(request.user, 'administrator')) | ||
| if not vol: | ||
| return render(request, 'vms/no_volunteer_access.html', status=403) | ||
| elif vol != True: |
There was a problem hiding this comment.
@necessary129 Could we not have a simple else clause here?
There was a problem hiding this comment.
see the reply to the default value one
| @wraps(func) | ||
| def wrapped_view(request, volunteer_id): | ||
| vol = getattr(request.user, 'volunteer', hasattr(request.user, 'administrator')) | ||
| if not vol: |
There was a problem hiding this comment.
@necessary129 You have this check to ensure if the user is logged in, right?
There was a problem hiding this comment.
Yes, but also if the user was added through the admin interface, where they aren't an admin or a volunteer, it will display a no rights page
There was a problem hiding this comment.
@necessary129 I did not understand how this is possible. A user can be added through the admin interface by another admin. If a user is added to the system they are necessarily either an admin or a volunteer.
There was a problem hiding this comment.
Well.. in my local db, there is a user who is neither an admin nor a volunteer. ie, only a django.contrib.auth.models.User. It happens when you add a user by python manage.py createsuperuser or like accessing the /admin page and adding only a user.
There was a problem hiding this comment.
@necessary129 Ok, don't change this yet
necessary129
force-pushed
the
vol-acc-fix
branch
from
cf35353 to
23427e5
Compare
|
@necessary129 One test fails. Most probably, the test would be updated rather than your code to fix that. Also, I need to verify whether admins should be able to view these pages or not. So, there could be possible changes required later to get this merged. |
necessary129
force-pushed
the
vol-acc-fix
branch
from
23427e5 to
e1ae179
Compare
|
Coverage increased (+0.2%) to 91.557% when pulling e1ae1799a207085d2b5a32c7458b6c1fc633a55a on necessary129:vol-acc-fix into 282b3b2 on systers:develop. |
|
@smarshy Fixed the tests! |
|
@necessary129 Could you please change 'fixes' to 'related to' or something similar in the description and commit message? A few more urls have been added to the issue which haven't been handled in this PR. |
Related to #326 also fix the test.
necessary129
force-pushed
the
vol-acc-fix
branch
from
e1ae179 to
d750aa8
Compare
|
Aye aye! |
Related to #326