Problem/Motivation

In the UserIntegritySensorPlugin we are showing in the verbose output the title "List of roles with restricted permissions." even if there are not role to list.

Proposed resolution

Assert if there are any restricted role and then show the list.

CommentFileSizeAuthor
#19 2598216-19.patch1.39 KBgiancarlosotelo
#15 show_title_of_list_of-2598216-14.patch713 bytesvisabhishek
#11 no-roles-2598216-11.patch771 bytesanil280988
#8 no-roles-2598216-8.patch1.09 KBanil280988
#6 no-roles-2598216-6.patch1.09 KBanil280988
#3 no-roles-2598216-3.patch1.09 KBsdstyles

Comments

giancarlosotelo created an issue. See original summary.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Please revert the logic: Always show the title and add an explicit statement: "NONE" if there are none!

sdstyles’s picture

sdstyles commented
Status: Active » Needs review
StatusFileSize
new no-roles-2598216-3.patch1.09 KB
miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented 
+++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
@@ -366,7 +366,6 @@ class UserIntegritySensorPlugin extends SensorPluginBase implements ExtendedInfo
-    $roles_list = [];

I think we can keep this line in combination with empty().

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

I think in tests we should also execute the sensor once without privileged users and check this None output.

anil280988’s picture

anil280988 commented
StatusFileSize
new no-roles-2598216-6.patch1.09 KB

Added the line back

Status: Needs review » Needs work

The last submitted patch, 6: no-roles-2598216-6.patch, failed testing.

anil280988’s picture

anil280988 commented
Status: Needs work » Needs review
StatusFileSize
new no-roles-2598216-8.patch1.09 KB

Added back the line.

Status: Needs review » Needs work

The last submitted patch, 8: no-roles-2598216-8.patch, failed testing.

giancarlosotelo’s picture

giancarlosotelo commented

Patch is not applying because you are adding the line manually, so instead of doing that can you generate a new patch?

And it would be nice if you can add test explained in #5 maybe in MonitoringUITest::testSensorDetailPage() where the list of roles is tested so you can remove the permission and assert the none output.

anil280988’s picture

anil280988 commented
Status: Needs work » Needs review
StatusFileSize
new no-roles-2598216-11.patch771 bytes

Created the Patch

Status: Needs review » Needs work

The last submitted patch, 11: no-roles-2598216-11.patch, failed testing.

visabhishek’s picture

visabhishek commented
Status: Needs work » Needs review

I am re-rolling the patch #11.

@giancarlosotelo : Please give me some more idea to add test explained in #5.

The last submitted patch, 6: no-roles-2598216-6.patch, failed testing.

visabhishek’s picture

visabhishek commented
StatusFileSize
new show_title_of_list_of-2598216-14.patch713 bytes

forget to upload the patch

The last submitted patch, 8: no-roles-2598216-8.patch, failed testing.

The last submitted patch, 11: no-roles-2598216-11.patch, failed testing.

giancarlosotelo’s picture

giancarlosotelo commented

In the function I said before we create a user with privilege permissions and we grant privilege permissions to the 'authenticated' role so before that code you can check the output in the test and assert that we have the 'None' text because we don't have any restricted role with privilege permissions yet.

giancarlosotelo’s picture

giancarlosotelo commented
StatusFileSize
new 2598216-19.patch1.39 KB

Since nobody seems to want to work on tests, here is a patch.

Status: Needs review » Needs work

The last submitted patch, 19: 2598216-19.patch, failed testing.

giancarlosotelo’s picture

giancarlosotelo commented

Unrelated fails

giancarlosotelo’s picture

giancarlosotelo commented
Status: Needs work » Needs review
miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Fixed

Committed, thx!

The last submitted patch, 3: no-roles-2598216-3.patch, failed testing.

The last submitted patch, 6: no-roles-2598216-6.patch, failed testing.

The last submitted patch, 8: no-roles-2598216-8.patch, failed testing.

The last submitted patch, 11: no-roles-2598216-11.patch, failed testing.

The last submitted patch, 15: show_title_of_list_of-2598216-14.patch, failed testing.

Status: Fixed » Needs work

The last submitted patch, 19: 2598216-19.patch, failed testing.

giancarlosotelo’s picture

giancarlosotelo commented
Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.