Add missing permission checks in runs/get_case_runs.html #750

ivo0126 · 2019-01-27T10:46:57Z

Fixes #716

codecov-io · 2019-01-27T11:09:49Z

Codecov Report

Merging #750 into master will decrease coverage by 6.44%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #750      +/-   ##
==========================================
- Coverage   73.42%   66.97%   -6.45%     
==========================================
  Files         111      114       +3     
  Lines        4753     5893    +1140     
  Branches      607      782     +175     
==========================================
+ Hits         3490     3947     +457     
- Misses       1041     1706     +665     
- Partials      222      240      +18

Impacted Files	Coverage Δ
tcms/urls.py	`73.33% <0%> (-3.81%)`	⬇️
tcms/testcases/forms.py	`79.06% <0%> (-2.39%)`	⬇️
tcms/settings/common.py	`96.36% <0%> (-2.03%)`	⬇️
tcms/core/templatetags/extra_filters.py	`87.5% <0%> (-1.39%)`	⬇️
tcms/issuetracker/types.py	`35.25% <0%> (-0.6%)`	⬇️
tcms/testplans/models.py	`75.96% <0%> (-0.37%)`	⬇️
tcms/testplans/views.py	`63.67% <0%> (-0.24%)`	⬇️
tcms/kiwi_auth/forms.py	`92.5% <0%> (-0.19%)`	⬇️
tcms/xmlrpc/api/testplan.py	`54.66% <0%> (-0.1%)`	⬇️
tcms/testcases/views.py	`60.42% <0%> (-0.03%)`	⬇️
... and 30 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0473f32...a816615. Read the comment docs.

coveralls · 2019-01-27T11:13:34Z

Pull Request Test Coverage Report for Build 2407

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 64.489%

Totals
Change from base Build 2400:	0.0%
Covered Lines:	4016
Relevant Lines:	5825

💛 - Coveralls

coveralls · 2019-01-27T11:13:36Z

Pull Request Test Coverage Report for Build 3165

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 74.057%

Totals
Change from base Build 3152:	0.0%
Covered Lines:	3712
Relevant Lines:	4753

💛 - Coveralls

atodorov

Fix MissingPermissionsChecker visit_module booliean condition

fix the typo in the commit log
explain why you do this, not only what you have done

atodorov

Fix the commit log for the linter so I can cherry-pick it. The rest will need a lot more work before it is ready.

atodorov · 2019-02-07T21:08:25Z

tcms/testruns/tests/test_views.py

+            username=self.tester.username,
+            password='password')
+
+    def getNewResponse(self):


you don't need a function that has only 2 lines. Repeating them, specially in tests is just fine. It saves you from having to go back and see what this function does.

atodorov · 2019-02-07T21:11:05Z

tcms/testruns/tests/test_views.py

+        self.getNewResponse()
+
+        for html_string in self.menu_html:
+            self.assertContains(self.response, html_string, html=True)


Not very nice. These should be separate test methods so that when something fails we know what failed. Otherwise when this method fails we have to spend the time to debug the entire menu and its permissions to figure out which exactly part failed.

While at it the menu_html list should become separate variables for each part of the menu.

atodorov · 2019-02-07T21:11:34Z

tcms/testruns/tests/test_views.py

+        self.getNewResponse()
+
+        for html_string in self.menu_html:
+            self.assertNotContains(self.response, html_string, html=True)


Same as above, separate test methods so we know what and when fails!

atodorov · 2019-02-07T21:17:35Z

tcms/templates/run/get_case_runs.html

-                            <li><a href="#" class="addBlue9 js-show-commentdialog">{% trans "Add" %}</a></li>
-			</ul>
-		</div>
+		{% if perms.testruns.change_testcaserun %}


testruns.change_testcaserun means to edit the TestCaseRun object, that is to be able to edit the recorded status of a test execution which we don't do.

Only the 'Update' and 'Assignee' actions in this menu are controlled by this permission so it is wrong to surround the entire sub-menu with it.

atodorov · 2019-02-07T21:19:47Z

tcms/templates/run/get_case_runs.html

+					{% if perms.testruns.delete_testcaserun %}
+	                                <li><a href="#" title="Remove selected cases form this test run" data-param="{{ test_run.run_id }}" class="removeBlue9 js-del-case">{% trans "Remove" %}</a></li>
+					{% endif %}
+	                <li><a href="#" title="Update the IDLE case runs to newest case text" href="javascript:void(0)" data-param="{% url 'testruns-update_case_run_text' test_run.pk %}" class="updateBlue9 js-update-case">{% trans "Update" %}</a></li>


Now this one should be controlled via testruns.change_testcaserun. Also the view handling this request is missing the permissions required decorator so you will have to create tests for it and add the missing decorator before continuing with the HTML template (better use a separate pull request)

atodorov · 2019-02-07T21:20:51Z

tcms/templates/run/get_case_runs.html

+	                                <li><a href="#" title="Remove selected cases form this test run" data-param="{{ test_run.run_id }}" class="removeBlue9 js-del-case">{% trans "Remove" %}</a></li>
+					{% endif %}
+	                <li><a href="#" title="Update the IDLE case runs to newest case text" href="javascript:void(0)" data-param="{% url 'testruns-update_case_run_text' test_run.pk %}" class="updateBlue9 js-update-case">{% trans "Update" %}</a></li>
+	                <li><a href="#" title="Assignee this case(s) to other people" class="assigneeBlue9 js-change-assignee">{% trans "Assignee" %}</a></li>


This one is also controlled by testruns.change_testcaserun so group it with the previous one. This time the backend portion has the permission decorator applied.

atodorov · 2019-02-07T21:25:36Z

tcms/templates/run/get_case_runs.html

+				</ul>
+			</div>
+			<div class="btnBlueCaserun">
+	                    <span>{% trans "Status" %}</span>


testruns.change_testcaserun

atodorov · 2019-02-07T21:29:05Z

tcms/templates/run/get_case_runs.html

+	                    <span>{% trans "Bugs" %}</span>
+				<ul>
+					{% if perms.testcases.add_bug %}
+	                            <li><a href="javascript:void(0);" class="addBlue9 js-add-bugs">{% trans "Add" %}</a></li>


The backend ajax function doesn't use any permissions and should be removed in favor of existing JSON-RPC

atodorov · 2019-02-07T21:29:59Z

tcms/templates/run/get_case_runs.html

+	                            <li><a href="javascript:void(0);" class="addBlue9 js-add-bugs">{% trans "Add" %}</a></li>
+	                {% endif %}
+					{% if perms.testcases.delete_bug %}
+	                            <li><a href="javascript:void(0);" class="removeBlue9 js-remove-bugs">{% trans "Remove" %}</a></li>


Same thing here, refactor to JSON-RPC first.

atodorov · 2019-02-07T21:33:46Z

tcms/templates/run/get_case_runs.html

+			<div class="btnBlueCaserun">
+	                    <span>{% trans "Comment" %}</span>
+				<ul>
+	                            <li><a href="#" class="addBlue9 js-show-commentdialog">{% trans "Add" %}</a></li>


Same thing here. No permissions used on the backend, must be refactored first to use JSON-RPC (TestCaseRun.add_comment). On the backend no permissions are enforced when adding comments and there are 2 duplicate helpers that do this.

atodorov · 2019-02-11T18:23:23Z

tcms/templates/run/get_case_runs.html

-                            <li><a href="javascript:void(0);" class="addBlue9 js-add-bugs">{% trans "Add" %}</a></li>
-                            <li><a href="javascript:void(0);" class="removeBlue9 js-remove-bugs">{% trans "Remove" %}</a></li>
+                        <li><a href="javascript:void(0);" class="addBlue9 js-add-bugs">{% trans "Add" %}</a></li>
+                        <li><a href="javascript:void(0);" class="removeBlue9 js-remove-bugs">{% trans "Remove" %}</a></li>


Do not change spaces if you are not changing anything else. It makes the PR harder to read/review and doesn't bring anything useful.

Not resolved, revert white-space changes if nothing else changed.

atodorov · 2019-02-11T18:24:08Z

tcms/testruns/tests/test_views.py

+class TestCaserunEditMenus(BaseCaseRun):
+    @classmethod
+    def setUpTestData(cls):
+        super().setUpTestData()


Method that only calls super() is useless. It doesn't do anything on its own and directly delegates to the parent class as if it didn't exist.

atodorov · 2019-02-11T18:25:13Z

tcms/testruns/tests/test_views.py

+    def setUpTestData(cls):
+        super().setUpTestData()
+
+    def setUp(self):


setUp is executed before every test method, while setUpTestData is executed only once. Given that your testing data doesn't change between the various test methods you probably want this executed only once.

atodorov · 2019-02-11T18:26:14Z

tcms/testruns/tests/test_views.py

+
+        self.client.login(  # nosec:B106:hardcoded_password_funcarg
+            username=self.tester.username,
+            password='password')


I believe this is already done in one of the parent classes. Why do we need it?

atodorov · 2019-02-11T18:29:11Z

tcms/testruns/tests/test_views.py

+        self.unauthorized.save()
+
+        for perm_string in self.menu_perms:
+            remove_perm_from_user(self.unauthorized, perm_string)


It would be best to have permissions assigned inside the test method and only assign/remove permissions for which we are testing. Nothing else! This will mean the tests are in their most strict form and will catch possible errors in the future where we mess up permissions.

atodorov

It is getting there, but there is still work to be done which will be very hard in such a big PR. You will have to start splitting this into smaller pieces so we can make progress on it:

For the Cases menu, there are permission label checks inside the template from before. However Rename model TestCaseRun to TestExecution #848 renames them. Do the following: Checkout PR 848 into a separate branch (git fetch upstream pull/848/head:pr848) and create a new branch on top of it. Cherry-pick only the tests commit and apply modifications to it:

update the permission label names
remove other test scenarios for the other menus
Rename test class to reference only the Cases menu
Rename test methods to not include the _get_ part

Then submit a new PR which we can review first!

For the Status menu permission labels seem to be OK, tests need some changes. This is independent of 1). Also the tests must be in their own class to avoid making the test class too big.
Bugs menu: you have extra white-space (tabs vs. space) in the template without any other changes. No permission labels in the template, no tests.
Comments menu: extra white space, missing permission label checks in template, no tests (there is the add_comment_html variable but it is unused.

So we're looking at 4 separate pull requests.

atodorov · 2019-03-24T21:42:57Z

tcms/testruns/tests/test_views.py

+        response = self.client.get(self.url)
+        self.assertEqual(HTTPStatus.OK, response.status_code)
+
+        for tcrs in TestCaseRunStatus.objects.all():


You have to iterate over the html menu options, not the status objects in the database.

atodorov · 2019-03-24T21:48:22Z

tcms/testruns/tests/test_views.py

+    def test_get_change_assignee_without_permissions(self):
+        remove_perm_from_user(self.tester, 'testruns.change_testcaserun')
+        response = self.client.get(self.url)
+        self.assertEqual(HTTPStatus.OK, response.status_code)


This is not needed b/c assertContains will check if the status is OK

atodorov · 2019-03-24T21:48:39Z

tcms/testruns/tests/test_views.py

+        self.assertEqual(HTTPStatus.OK, response.status_code)
+        self.assertNotContains(response, self.update_case_run_text_html, html=True)
+
+    def test_get_change_assignee_without_permissions(self):


All test names must end without the extra s.

atodorov · 2019-03-24T21:49:23Z

tcms/templates/run/get_case_runs.html

-                            <li><a href="javascript:void(0);" class="addBlue9 js-add-bugs">{% trans "Add" %}</a></li>
-                            <li><a href="javascript:void(0);" class="removeBlue9 js-remove-bugs">{% trans "Remove" %}</a></li>
+                        <li><a href="javascript:void(0);" class="addBlue9 js-add-bugs">{% trans "Add" %}</a></li>
+                        <li><a href="javascript:void(0);" class="removeBlue9 js-remove-bugs">{% trans "Remove" %}</a></li>


Not resolved, revert white-space changes if nothing else changed.

Fixes kiwitcms#716

ivo0126 force-pushed the run_permission_check branch 2 times, most recently from 3ab16c5 to 2f94d5c Compare February 7, 2019 16:35

atodorov requested changes Feb 7, 2019

View reviewed changes

ivo0126 force-pushed the run_permission_check branch 2 times, most recently from eb66c05 to 698316a Compare February 11, 2019 15:20

atodorov requested changes Feb 11, 2019

View reviewed changes

ivo0126 force-pushed the run_permission_check branch 2 times, most recently from 323f58c to 1e9c3b3 Compare February 21, 2019 15:17

ivo0126 force-pushed the run_permission_check branch from 1e9c3b3 to 0318481 Compare March 24, 2019 16:59

atodorov requested changes Mar 24, 2019

View reviewed changes

ivo0126 force-pushed the run_permission_check branch from 0318481 to bcaffc6 Compare May 11, 2019 08:16

ivo0126 added 2 commits May 11, 2019 13:50

Add missing permission checks in runs/get_case_runs.html

ab13e15

Fixes kiwitcms#716

Add tests for edit-menu permission-checks in testruns

a816615

ivo0126 force-pushed the run_permission_check branch from bcaffc6 to a816615 Compare May 11, 2019 11:16

atodorov closed this May 18, 2019

ivo0126 deleted the run_permission_check branch May 19, 2019 09:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing permission checks in runs/get_case_runs.html #750

Add missing permission checks in runs/get_case_runs.html #750

ivo0126 commented Jan 27, 2019

codecov-io commented Jan 27, 2019 •

edited

Loading

coveralls commented Jan 27, 2019 •

edited

Loading

coveralls commented Jan 27, 2019 •

edited

Loading

atodorov left a comment

atodorov left a comment

atodorov Feb 7, 2019

atodorov Feb 7, 2019

atodorov Feb 7, 2019

atodorov Feb 7, 2019

atodorov Feb 7, 2019

atodorov Feb 7, 2019

atodorov Feb 7, 2019

atodorov Feb 7, 2019

atodorov Feb 7, 2019

atodorov Feb 7, 2019

atodorov Feb 11, 2019

atodorov Mar 24, 2019

atodorov Feb 11, 2019

atodorov Feb 11, 2019

atodorov Feb 11, 2019

atodorov Feb 11, 2019

atodorov left a comment

atodorov Mar 24, 2019

atodorov Mar 24, 2019

atodorov Mar 24, 2019

atodorov Mar 24, 2019

Add missing permission checks in runs/get_case_runs.html #750

Add missing permission checks in runs/get_case_runs.html #750

Conversation

ivo0126 commented Jan 27, 2019

codecov-io commented Jan 27, 2019 • edited Loading

Codecov Report

coveralls commented Jan 27, 2019 • edited Loading

Pull Request Test Coverage Report for Build 2407

💛 - Coveralls

coveralls commented Jan 27, 2019 • edited Loading

Pull Request Test Coverage Report for Build 3165

💛 - Coveralls

atodorov left a comment

Choose a reason for hiding this comment

atodorov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atodorov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 27, 2019 •

edited

Loading

coveralls commented Jan 27, 2019 •

edited

Loading

coveralls commented Jan 27, 2019 •

edited

Loading