Merge branch 'kerl/403_vs_404' into 'master'

Replace some 403 by 404 to avoid trigramme leaking

Closes #224

See merge request klub-dev-ens/gestioCOF!364
This commit is contained in:
Ludovic Stephan 2019-10-05 16:01:46 +02:00
commit b99fd03df2
4 changed files with 134 additions and 11 deletions

View file

@ -1,3 +1,4 @@
- Si on essaie d'accéder au compte que qqn d'autre on a une 404 (et plus une 403)
- On ne peut plus modifier des comptes COF depuis l'interface K-Fêt - On ne peut plus modifier des comptes COF depuis l'interface K-Fêt
- Le champ de paiement BdA se fait au niveau des attributions - Le champ de paiement BdA se fait au niveau des attributions
- Affiche un message d'erreur plutôt que de crasher si échec de l'envoi du mail - Affiche un message d'erreur plutôt que de crasher si échec de l'envoi du mail

View file

@ -36,8 +36,7 @@ class TestStats(TestCase):
client2 = Client() client2 = Client()
client2.login(username="Barfoo", password="barfoo") client2.login(username="Barfoo", password="barfoo")
# 1. FOO should be able to get these pages but BAR receives a Forbidden # 1. FOO should be able to get these pages but BAR receives a 404
# response
user_urls = [ user_urls = [
"/k-fet/accounts/FOO/stat/operations/list", "/k-fet/accounts/FOO/stat/operations/list",
"/k-fet/accounts/FOO/stat/operations?{}".format( "/k-fet/accounts/FOO/stat/operations?{}".format(
@ -57,7 +56,7 @@ class TestStats(TestCase):
resp = client.get(url) resp = client.get(url)
self.assertEqual(200, resp.status_code) self.assertEqual(200, resp.status_code)
resp2 = client2.get(url) resp2 = client2.get(url)
self.assertEqual(403, resp2.status_code) self.assertEqual(404, resp2.status_code)
# 2. FOO is a member of the team and can get these pages but BAR # 2. FOO is a member of the team and can get these pages but BAR
# receives a Redirect response # receives a Redirect response

View file

@ -209,6 +209,25 @@ class AccountReadViewTests(ViewTestCaseMixin, TestCase):
auth_user = "team" auth_user = "team"
auth_forbidden = [None, "user"] auth_forbidden = [None, "user"]
# Users with forbidden access users should get a 404 here, to avoid leaking trigrams
# See issue #224
def test_forbidden(self):
for user in self.auth_forbidden:
self.assertRedirectsToLoginOr404(user, self.url_expected)
self.assertRedirectsToLoginOr404(user, "/k-fet/accounts/NEX")
def assertRedirectsToLoginOr404(self, user, url):
client = Client()
if user is None:
response = client.get(url)
self.assertRedirects(
response, "/login?next={}".format(url), fetch_redirect_response=False
)
else:
client.login(username=user, password=user)
response = client.get(url)
self.assertEqual(response.status_code, 404)
def get_users_extra(self): def get_users_extra(self):
return {"user1": create_user("user1", "001")} return {"user1": create_user("user1", "001")}
@ -296,6 +315,29 @@ class AccountUpdateViewTests(ViewTestCaseMixin, TestCase):
"team1": create_team("team1", "101", perms=["kfet.change_account"]), "team1": create_team("team1", "101", perms=["kfet.change_account"]),
} }
# Users with forbidden access users should get a 404 here, to avoid leaking trigrams
# See issue #224
def test_forbidden(self):
for method in ["get", "post"]:
for user in self.auth_forbidden:
self.assertRedirectsToLoginOr404(user, method, self.url_expected)
self.assertRedirectsToLoginOr404(
user, method, "/k-fet/accounts/NEX/edit"
)
def assertRedirectsToLoginOr404(self, user, method, url):
client = Client()
meth = getattr(client, method)
if user is None:
response = meth(url)
self.assertRedirects(
response, "/login?next={}".format(url), fetch_redirect_response=False
)
else:
client.login(username=user, password=user)
response = meth(url)
self.assertEqual(response.status_code, 404)
def test_get_ok(self): def test_get_ok(self):
r = self.client.get(self.url) r = self.client.get(self.url)
self.assertEqual(r.status_code, 200) self.assertEqual(r.status_code, 200)
@ -375,7 +417,7 @@ class AccountDeleteViewTests(ViewTestCaseMixin, TestCase):
if Account.objects.get(trigramme=trigramme).readable: if Account.objects.get(trigramme=trigramme).readable:
expected_code = 200 expected_code = 200
else: else:
expected_code = 403 expected_code = 404
r = self.client.post( r = self.client.post(
reverse(self.url_name, kwargs={"trigramme": trigramme}), {} reverse(self.url_name, kwargs={"trigramme": trigramme}), {}
) )
@ -555,6 +597,27 @@ class AccountStatOperationListViewTests(ViewTestCaseMixin, TestCase):
def get_users_extra(self): def get_users_extra(self):
return {"user1": create_user("user1", "001")} return {"user1": create_user("user1", "001")}
# Users with forbidden access users should get a 404 here, to avoid leaking trigrams
# See issue #224
def test_forbidden(self):
for user in self.auth_forbidden:
self.assertRedirectsToLoginOr404(user, self.url_expected)
self.assertRedirectsToLoginOr404(
user, "/k-fet/accounts/NEX/stat/operations/list"
)
def assertRedirectsToLoginOr404(self, user, url):
client = Client()
if user is None:
response = client.get(url)
self.assertRedirects(
response, "/login?next={}".format(url), fetch_redirect_response=False
)
else:
client.login(username=user, password=user)
response = client.get(url)
self.assertEqual(response.status_code, 404)
def test_ok(self): def test_ok(self):
r = self.client.get(self.url) r = self.client.get(self.url)
self.assertEqual(r.status_code, 200) self.assertEqual(r.status_code, 200)
@ -616,6 +679,27 @@ class AccountStatOperationViewTests(ViewTestCaseMixin, TestCase):
auth_user = "user1" auth_user = "user1"
auth_forbidden = [None, "user", "team"] auth_forbidden = [None, "user", "team"]
# Users with forbidden access users should get a 404 here, to avoid leaking trigrams
# See issue #224
def test_forbidden(self):
for user in self.auth_forbidden:
self.assertRedirectsToLoginOr404(user, self.url_expected)
self.assertRedirectsToLoginOr404(
user, "/k-fet/accounts/NEX/stat/operations"
)
def assertRedirectsToLoginOr404(self, user, url):
client = Client()
if user is None:
response = client.get(url)
self.assertRedirects(
response, "/login?next={}".format(url), fetch_redirect_response=False
)
else:
client.login(username=user, password=user)
response = client.get(url)
self.assertEqual(response.status_code, 404)
def get_users_extra(self): def get_users_extra(self):
return {"user1": create_user("user1", "001")} return {"user1": create_user("user1", "001")}
@ -632,6 +716,27 @@ class AccountStatBalanceListViewTests(ViewTestCaseMixin, TestCase):
auth_user = "user1" auth_user = "user1"
auth_forbidden = [None, "user", "team"] auth_forbidden = [None, "user", "team"]
# Users with forbidden access users should get a 404 here, to avoid leaking trigrams
# See issue #224
def test_forbidden(self):
for user in self.auth_forbidden:
self.assertRedirectsToLoginOr404(user, self.url_expected)
self.assertRedirectsToLoginOr404(
user, "/k-fet/accounts/NEX/stat/balance/list"
)
def assertRedirectsToLoginOr404(self, user, url):
client = Client()
if user is None:
response = client.get(url)
self.assertRedirects(
response, "/login?next={}".format(url), fetch_redirect_response=False
)
else:
client.login(username=user, password=user)
response = client.get(url)
self.assertEqual(response.status_code, 404)
def get_users_extra(self): def get_users_extra(self):
return {"user1": create_user("user1", "001")} return {"user1": create_user("user1", "001")}
@ -677,6 +782,25 @@ class AccountStatBalanceViewTests(ViewTestCaseMixin, TestCase):
auth_user = "user1" auth_user = "user1"
auth_forbidden = [None, "user", "team"] auth_forbidden = [None, "user", "team"]
# Users with forbidden access users should get a 404 here, to avoid leaking trigrams
# See issue #224
def test_forbidden(self):
for user in self.auth_forbidden:
self.assertRedirectsToLoginOr404(user, self.url_expected)
self.assertRedirectsToLoginOr404(user, "/k-fet/accounts/NEX/stat/balance")
def assertRedirectsToLoginOr404(self, user, url):
client = Client()
if user is None:
response = client.get(url)
self.assertRedirects(
response, "/login?next={}".format(url), fetch_redirect_response=False
)
else:
client.login(username=user, password=user)
response = client.get(url)
self.assertEqual(response.status_code, 404)
def get_users_extra(self): def get_users_extra(self):
return {"user1": create_user("user1", "001")} return {"user1": create_user("user1", "001")}

View file

@ -10,7 +10,6 @@ from django.contrib.auth.decorators import login_required, permission_required
from django.contrib.auth.mixins import PermissionRequiredMixin from django.contrib.auth.mixins import PermissionRequiredMixin
from django.contrib.auth.models import Permission, User from django.contrib.auth.models import Permission, User
from django.contrib.messages.views import SuccessMessageMixin from django.contrib.messages.views import SuccessMessageMixin
from django.core.exceptions import PermissionDenied
from django.db import transaction from django.db import transaction
from django.db.models import Count, F, Prefetch, Sum from django.db.models import Count, F, Prefetch, Sum
from django.forms import formset_factory from django.forms import formset_factory
@ -303,7 +302,7 @@ def account_read(request, trigramme):
if not account.readable or ( if not account.readable or (
not request.user.has_perm("kfet.is_team") and request.user != account.user not request.user.has_perm("kfet.is_team") and request.user != account.user
): ):
raise PermissionDenied raise Http404
addcosts = ( addcosts = (
OperationGroup.objects.filter(opes__addcost_for=account, opes__canceled_at=None) OperationGroup.objects.filter(opes__addcost_for=account, opes__canceled_at=None)
@ -327,7 +326,7 @@ def account_update(request, trigramme):
# Checking permissions # Checking permissions
if not request.user.has_perm("kfet.is_team") and request.user != account.user: if not request.user.has_perm("kfet.is_team") and request.user != account.user:
raise PermissionDenied raise Http404
user_info_form = UserInfoForm(instance=account.user) user_info_form = UserInfoForm(instance=account.user)
@ -2226,7 +2225,7 @@ class AccountStatBalanceList(PkUrlMixin, SingleResumeStat):
def get_object(self, *args, **kwargs): def get_object(self, *args, **kwargs):
obj = super().get_object(*args, **kwargs) obj = super().get_object(*args, **kwargs)
if self.request.user != obj.user: if self.request.user != obj.user:
raise PermissionDenied raise Http404
return obj return obj
@method_decorator(login_required) @method_decorator(login_required)
@ -2345,7 +2344,7 @@ class AccountStatBalance(PkUrlMixin, JSONDetailView):
def get_object(self, *args, **kwargs): def get_object(self, *args, **kwargs):
obj = super().get_object(*args, **kwargs) obj = super().get_object(*args, **kwargs)
if self.request.user != obj.user: if self.request.user != obj.user:
raise PermissionDenied raise Http404
return obj return obj
@method_decorator(login_required) @method_decorator(login_required)
@ -2376,7 +2375,7 @@ class AccountStatOperationList(PkUrlMixin, SingleResumeStat):
def get_object(self, *args, **kwargs): def get_object(self, *args, **kwargs):
obj = super().get_object(*args, **kwargs) obj = super().get_object(*args, **kwargs)
if self.request.user != obj.user: if self.request.user != obj.user:
raise PermissionDenied raise Http404
return obj return obj
@method_decorator(login_required) @method_decorator(login_required)
@ -2439,7 +2438,7 @@ class AccountStatOperation(ScaleMixin, PkUrlMixin, JSONDetailView):
def get_object(self, *args, **kwargs): def get_object(self, *args, **kwargs):
obj = super().get_object(*args, **kwargs) obj = super().get_object(*args, **kwargs)
if self.request.user != obj.user: if self.request.user != obj.user:
raise PermissionDenied raise Http404
return obj return obj
@method_decorator(login_required) @method_decorator(login_required)