From 0b1619b87401093c17c67bb1463329992797fd6f Mon Sep 17 00:00:00 2001 From: Brad Johnson Date: Sun, 7 Mar 2021 20:59:31 -0600 Subject: [PATCH 1/2] fix(): 404 if a trainer tries to login to a different gym Rather than just returning "permission denied", we can interpret this scenario as a "failure to look up a user with that gym/user ID combo". This should give users the information they need to self-recover, while not leaking any other sensitive details. Fixes: #585 --- .github/contributing.md | 4 +- .github/pull_request_template.md | 2 +- .gitignore | 1 + AUTHORS.rst | 4 +- README.md | 4 +- wger/core/tests/test_user_login.py | 78 ++++++++++++++++++++++++++++++ wger/core/views/user.py | 14 ++++-- 7 files changed, 96 insertions(+), 11 deletions(-) create mode 100644 wger/core/tests/test_user_login.py diff --git a/.github/contributing.md b/.github/contributing.md index fb3bc3c0e..43317d4eb 100644 --- a/.github/contributing.md +++ b/.github/contributing.md @@ -28,9 +28,9 @@ describe a use-case or similar. ## Pull Requests You want to contribute code? Awesome! Here are some tips: -* Make sure the tests are running (``python3 manage.py tests``)... +* Make sure the tests are running (``python3 manage.py test``)... * ... and if you write new code, write new tests -* Lint the code with flake8 (``flake8 --config .github/linters/.flake8``) +* Lint the code with flake8 (``flake8 --config .github/linters/.flake8 ./wger``) and isort (``isort``) * You can expect a response from a maintainer within a week, if you haven’t heard anything by then, feel free to ping the thread. diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index ace2b414f..2328922ad 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -7,7 +7,7 @@ ### Please check that the PR fulfills these requirements - [ ] Tests for the changes have been added (for bug fixes / features) -- [ ] New python code has been linted with with flake8 (``flake8 --config .github/linters/.flake8``) +- [ ] New python code has been linted with with flake8 (``flake8 --config .github/linters/.flake8 ./wger``) and isort (``isort``) - [ ] Added yourself to AUTHORS.rst diff --git a/.gitignore b/.gitignore index a15f12fa3..595a743c2 100644 --- a/.gitignore +++ b/.gitignore @@ -65,6 +65,7 @@ node_modules # Virtual envs venv +venv-wget # Dummy file for translations /wger/i18n.tpl diff --git a/AUTHORS.rst b/AUTHORS.rst index 925d121ad..f42a7f120 100644 --- a/AUTHORS.rst +++ b/AUTHORS.rst @@ -52,7 +52,7 @@ Developers * Aaron Bassett - https://github.com/aaronbassett * Rachael Wright-Munn - https://github.com/ChaelCodes * Will Pearson - https://github.com/qagoose - +* Brad Johnson - https://github.com/bradsk88 Translators ----------- @@ -155,4 +155,4 @@ Translators Exercises --------- -And of course many thanks as well to everyone that submited exercises! +And of course many thanks as well to everyone that submitted exercises! diff --git a/README.md b/README.md index 6b540b880..26ca75014 100644 --- a/README.md +++ b/README.md @@ -53,7 +53,9 @@ persists the database on a volume. From the root folder just call docker-compose -f extras/docker/compose/docker-compose.yml up ```` -For more info, check the README in wger/extras/compose. +For more info, check the [README in wger/extras/docker/compose]( + ./extras/docker/compose/README.md +). #### Local installation (git) diff --git a/wger/core/tests/test_user_login.py b/wger/core/tests/test_user_login.py new file mode 100644 index 000000000..133562365 --- /dev/null +++ b/wger/core/tests/test_user_login.py @@ -0,0 +1,78 @@ +from unittest import mock + +from wger.core.tests.base_testcase import WgerTestCase +from wger.core.views.user import trainer_login + + +def _build_mock_request(user): + request = mock.Mock() + request.session = dict() + request.GET = dict() + request.user = user + return request + + +def _build_mock_user(gym_name, is_trainer=False): + user = mock.Mock() + user.userprofile.gym = gym_name + + def request_has_perm(perm): + if perm in ['gym.gym_trainer', 'gym.manage_gym', 'gym.manage_gyms']: + return is_trainer + return True # Don't care about other permissions for these tests + + user.has_perm.side_effect = request_has_perm + return user + + +class TrainerLoginTestCase(WgerTestCase): + + mock_django_login = None + + @classmethod + def setUpClass(cls): + cls.mock_django_login = mock.patch('wger.core.views.user.django_login').start() + + @classmethod + def tearDownClass(cls): + cls.mock_django_login.stop() + + def test_trainer_is_allowed_to_login_to_non_trainer_in_same_gym(self): + request_user = _build_mock_user('same-gym', is_trainer=True) + request = _build_mock_request(request_user) + user_from_db_lookup = _build_mock_user('same-gym', is_trainer=False) + + with mock.patch('wger.core.views.user.get_object_or_404', return_value=user_from_db_lookup): + resp = trainer_login(request, 'primary-key-not-needed-because-get-object-is-mocked') + + self.assertEqual(302, resp.status_code) + + def test_trainer_is_denied_from_login_to_trainer_in_same_gym(self): + request_user = _build_mock_user('same-gym', is_trainer=True) + request = _build_mock_request(request_user) + user_from_db_lookup = _build_mock_user('same-gym', is_trainer=True) + + with mock.patch('wger.core.views.user.get_object_or_404', return_value=user_from_db_lookup): + resp = trainer_login(request, 'primary-key-not-needed-because-of-mock') + + self.assertEqual(403, resp.status_code) + + def test_trainer_is_denied_from_login_to_trainer_at_different_gym(self): + request_user = _build_mock_user('trainer-gym', is_trainer=True) + request = _build_mock_request(request_user) + user_from_db_lookup = _build_mock_user('other-trainer-gym', is_trainer=True) + + with mock.patch('wger.core.views.user.get_object_or_404', return_value=user_from_db_lookup): + resp = trainer_login(request, 'primary-key-not-needed-because-of-mock') + + self.assertEqual(403, resp.status_code) + + def test_trainer_gets_404_when_trying_to_login_to_non_trainer_in_different_gym(self): + request_user = _build_mock_user('trainer-gym', is_trainer=True) + request = _build_mock_request(request_user) + user_from_db_lookup = _build_mock_user('user-gym', is_trainer=False) + + with mock.patch('wger.core.views.user.get_object_or_404', return_value=user_from_db_lookup): + resp = trainer_login(request, 'primary-key-not-needed-because-of-mock') + + self.assertEqual(404, resp.status_code) diff --git a/wger/core/views/user.py b/wger/core/views/user.py index a233871cc..86ce8e1e4 100644 --- a/wger/core/views/user.py +++ b/wger/core/views/user.py @@ -39,7 +39,8 @@ from django.contrib.auth.views import ( ) from django.http import ( HttpResponseForbidden, - HttpResponseRedirect + HttpResponseRedirect, + HttpResponseNotFound, ) from django.shortcuts import ( get_object_or_404, @@ -175,10 +176,6 @@ def trainer_login(request, user_pk): user = get_object_or_404(User, pk=user_pk) orig_user_pk = request.user.pk - # Changing only between the same gym - if request.user.userprofile.gym != user.userprofile.gym: - return HttpResponseForbidden() - # No changing if identity is not set if not request.user.has_perm('gym.gym_trainer') \ and not request.session.get('trainer.identity'): @@ -191,6 +188,13 @@ def trainer_login(request, user_pk): or user.has_perm('gym.manage_gyms')): return HttpResponseForbidden() + # Changing is only allowed between the same gym + if request.user.userprofile.gym != user.userprofile.gym: + return HttpResponseNotFound( + 'There are no users in gym "{}" with user ID "{}".'.format( + request.user.userprofile.gym, user_pk, + )) + # Check if we're switching back to our original account own = False if (user.has_perm('gym.gym_trainer') From 523e8c1552c107952a0bb75ad9f82a6e82b205c2 Mon Sep 17 00:00:00 2001 From: Brad Johnson Date: Tue, 9 Mar 2021 22:06:54 -0600 Subject: [PATCH 2/2] fix(): Set up test mocks correctly. --- wger/core/tests/test_user_login.py | 19 +++++-------------- wger/gym/tests/test_user.py | 2 +- 2 files changed, 6 insertions(+), 15 deletions(-) diff --git a/wger/core/tests/test_user_login.py b/wger/core/tests/test_user_login.py index 133562365..1c262dcbb 100644 --- a/wger/core/tests/test_user_login.py +++ b/wger/core/tests/test_user_login.py @@ -25,19 +25,10 @@ def _build_mock_user(gym_name, is_trainer=False): return user +@mock.patch('wger.core.views.user.django_login') class TrainerLoginTestCase(WgerTestCase): - mock_django_login = None - - @classmethod - def setUpClass(cls): - cls.mock_django_login = mock.patch('wger.core.views.user.django_login').start() - - @classmethod - def tearDownClass(cls): - cls.mock_django_login.stop() - - def test_trainer_is_allowed_to_login_to_non_trainer_in_same_gym(self): + def test_trainer_is_allowed_to_login_to_non_trainer_in_same_gym(self, _): request_user = _build_mock_user('same-gym', is_trainer=True) request = _build_mock_request(request_user) user_from_db_lookup = _build_mock_user('same-gym', is_trainer=False) @@ -47,7 +38,7 @@ class TrainerLoginTestCase(WgerTestCase): self.assertEqual(302, resp.status_code) - def test_trainer_is_denied_from_login_to_trainer_in_same_gym(self): + def test_trainer_is_denied_from_login_to_trainer_in_same_gym(self, _): request_user = _build_mock_user('same-gym', is_trainer=True) request = _build_mock_request(request_user) user_from_db_lookup = _build_mock_user('same-gym', is_trainer=True) @@ -57,7 +48,7 @@ class TrainerLoginTestCase(WgerTestCase): self.assertEqual(403, resp.status_code) - def test_trainer_is_denied_from_login_to_trainer_at_different_gym(self): + def test_trainer_is_denied_from_login_to_trainer_at_different_gym(self, _): request_user = _build_mock_user('trainer-gym', is_trainer=True) request = _build_mock_request(request_user) user_from_db_lookup = _build_mock_user('other-trainer-gym', is_trainer=True) @@ -67,7 +58,7 @@ class TrainerLoginTestCase(WgerTestCase): self.assertEqual(403, resp.status_code) - def test_trainer_gets_404_when_trying_to_login_to_non_trainer_in_different_gym(self): + def test_trainer_gets_404_when_trying_to_login_to_non_trainer_in_different_gym(self, _): request_user = _build_mock_user('trainer-gym', is_trainer=True) request = _build_mock_request(request_user) user_from_db_lookup = _build_mock_user('user-gym', is_trainer=False) diff --git a/wger/gym/tests/test_user.py b/wger/gym/tests/test_user.py index 65f405481..6392df86b 100644 --- a/wger/gym/tests/test_user.py +++ b/wger/gym/tests/test_user.py @@ -185,7 +185,7 @@ class TrainerLoginTestCase(WgerTestCase): profile.save() self.user_login('admin') response = self.client.get(reverse('core:user:trainer-login', kwargs={'user_pk': 2})) - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 404) self.assertFalse(self.client.session.get('trainer.identity')) def test_gym_trainer(self):