Skip to content
This repository was archived by the owner on Jan 26, 2021. It is now read-only.

Stop volunteers from accessing each other's urls. #408

Merged
merged 1 commit into from
Jan 26, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions vms/event/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
from django.utils.decorators import method_decorator
from django.shortcuts import render_to_response
from django.http import Http404
from volunteer.utils import vol_id_check


class AdministratorLoginRequiredMixin(object):
Expand Down Expand Up @@ -122,6 +123,7 @@ def get_queryset(self):


@login_required
@vol_id_check
def list_sign_up(request, volunteer_id):
if request.method == 'POST':
form = EventDateForm(request.POST)
Expand Down
2 changes: 1 addition & 1 deletion vms/shift/tests/test_viewVolunteerShift.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ def test_access_another_existing_volunteer_view(self):
def test_access_another_nonexisting_volunteer_view(self):
upcoming_shift_page = self.upcoming_shift_page
upcoming_shift_page.get_page(self.live_server_url, upcoming_shift_page.view_shift_page + '65459')
found = re.search('Not Found', self.driver.page_source)
found = re.search('You don\'t have the necessary rights to access this page', self.driver.page_source)
self.assertNotEqual(found, None)

def test_view_without_any_assigned_shift(self):
Expand Down
41 changes: 12 additions & 29 deletions vms/shift/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
from django.views.generic import ListView
from django.utils.decorators import method_decorator
from django.core.urlresolvers import reverse_lazy

from volunteer.utils import vol_id_check

class AdministratorLoginRequiredMixin(object):

Expand Down Expand Up @@ -568,6 +568,10 @@ def sign_up(request, shift_id, volunteer_id):
class ViewHoursView(LoginRequiredMixin, FormView, TemplateView):
template_name = 'shift/hours_list.html'

@method_decorator(vol_id_check)
def dispatch(self, *args, **kwargs):
return super(ViewHoursView, self).dispatch(*args, **kwargs)

def get_context_data(self, **kwargs):
context = super(ViewHoursView, self).get_context_data(**kwargs)
volunteer_id = self.kwargs['volunteer_id']
Expand All @@ -577,36 +581,15 @@ def get_context_data(self, **kwargs):


@login_required
@vol_id_check
def view_volunteer_shifts(request, volunteer_id):
user = request.user
vol = None

try:
vol = user.volunteer
except ObjectDoesNotExist:
pass
shift_list = get_unlogged_shifts_by_volunteer_id(volunteer_id)
return render(
request,
'shift/volunteer_shifts.html',
{'shift_list': shift_list, 'volunteer_id': volunteer_id, }
)

# check that a volunteer is logged in
if vol:
if volunteer_id:
volunteer = get_volunteer_by_id(volunteer_id)
if volunteer:
user = request.user
if int(user.volunteer.id) == int(volunteer_id):
shift_list = get_unlogged_shifts_by_volunteer_id(volunteer_id)
return render(
request,
'shift/volunteer_shifts.html',
{'shift_list': shift_list, 'volunteer_id': volunteer_id, }
)
else:
return HttpResponse(status=403)
else:
raise Http404
else:
raise Http404
else:
return HttpResponse(status=403)


class VolunteerSearchView(AdministratorLoginRequiredMixin, FormView):
Expand Down
22 changes: 22 additions & 0 deletions vms/vms/templates/vms/no_volunteer_access.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
{% extends "vms/base.html" %}

{% load i18n %}

{% block content %}
<div class="spacer"></div>

{% csrf_token %}
<div class="panel panel-danger">
<div class="panel-heading">
<h3 class="panel-title">{% trans "No Access" %}</h3>
</div>
<div class="panel-body">
<br>
{% trans "You don't have the necessary rights to access this page." %}
<br>
<br>
<input type="button" class="btn btn-default" value="{% blocktrans %}Return to Previous Page{% endblocktrans %}" onClick="javascript:history.go(-1);">
</div>
</div>

{% endblock %}
19 changes: 19 additions & 0 deletions vms/volunteer/utils.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
from functools import wraps
from django.shortcuts import render
from django.http import Http404
from volunteer.services import get_volunteer_by_id

def vol_id_check(func):
@wraps(func)
def wrapped_view(request, volunteer_id):
vol = getattr(request.user, 'volunteer', hasattr(request.user, 'administrator'))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@necessary129 Why use a default value here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aah, I thought the admin also needed to access this, heh :P

if not vol:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@necessary129 You have this check to ensure if the user is logged in, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@smarshy smarshy Dec 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@necessary129 Ok, don't change this yet

return render(request, 'vms/no_volunteer_access.html', status=403)
elif vol != True:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@necessary129 Could we not have a simple else clause here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see the reply to the default value one

volunteer = get_volunteer_by_id(volunteer_id)
if not volunteer:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing it!

return render(request, 'vms/no_volunteer_access.html', status=403)
if not int(volunteer.id) == vol.id:
return render(request, 'vms/no_volunteer_access.html', status=403)
return func(request, volunteer_id=volunteer_id)
return wrapped_view
17 changes: 12 additions & 5 deletions vms/volunteer/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@
from volunteer.validation import validate_file
from django.views.generic import View
from django.core.urlresolvers import reverse_lazy

from django.utils.decorators import method_decorator
from volunteer.utils import vol_id_check

@login_required
def download_resume(request, volunteer_id):
Expand Down Expand Up @@ -108,13 +109,15 @@ def form_valid(self, form):
class ProfileView(LoginRequiredMixin, DetailView):
template_name = 'volunteer/profile.html'

@method_decorator(vol_id_check)
def dispatch(self, *args, **kwargs):
return super(ProfileView, self).dispatch(*args, **kwargs)

def get_object(self, queryset=None):
volunteer_id = self.kwargs['volunteer_id']
obj = Volunteer.objects.get(id=self.kwargs['volunteer_id'])
if obj:
return obj
else:
return HttpResponse(status=403)
return obj


'''
The view generate Report.
Expand All @@ -123,6 +126,10 @@ def get_object(self, queryset=None):

class GenerateReportView(LoginRequiredMixin, View):

@method_decorator(vol_id_check)
def dispatch(self, *args, **kwargs):
return super(GenerateReportView, self).dispatch(*args, **kwargs)

def get(self, request, *args, **kwargs):
view = ShowFormView.as_view()
return view(request, *args,**kwargs)
Expand Down