Problem/Motivation

When attempting to access the administrative UI pages as a user with the appropriate permission, a 403 error is encountered.

Proposed resolution

Correct the access checks defined in simplesamlphp_auth.routing.yml. They need to be lower-case in order to match the

Remaining tasks

Needs review.

User interface changes

None.

API changes

None. This permission only seems to be utilized to control access to the administrative pages in the UI.

Data model changes

None.

CommentFileSizeAuthor
#34 2907182-34.patch4.29 KBidebr
#34 interdiff-33-34.txt476 bytesidebr
#33 2907182-33.patch4.29 KBidebr
#33 interdiff-31-33.txt5.99 KBidebr
#31 simplesamlphp_auth-admin_ui_pages-2907182-30.patch4.63 KBidebr
#19 simplesamlphp_auth-admin_ui_pages-2907182-19.patch5.03 KBkerasai
#17 simplesamlphp_auth-admin_ui_pages-2907182-17.patch3.82 KBkerasai
#12 simplesamlphp_auth-admin_ui_pages-2907182-12.patch4.98 KBkerasai
#10 simplesamlphp_auth-admin_ui_pages-2907182-10.patch4.8 KBkerasai
#8 simplesamlphp_auth-admin_ui_pages-2907182-8.patch4.2 KBkerasai
#6 simplesamlphp_auth-admin_ui_pages-2907182-6.patch2.62 KBkerasai
#4 simplesamlphp_auth-admin_ui_pages-2907182-4.patch2.08 KBkerasai
#2 simplesamlphp_auth-admin_ui_pages-2907182-2.patch2.04 KBkerasai

Comments

kerasai created an issue. See original summary.

kerasai’s picture

kerasai commented
Status: Active » Needs review
StatusFileSize
new simplesamlphp_auth-admin_ui_pages-2907182-2.patch2.04 KB

Posting a patch with tests to verify bug.

Status: Needs review » Needs work

The last submitted patch, 2: simplesamlphp_auth-admin_ui_pages-2907182-2.patch, failed testing. View results

kerasai’s picture

kerasai commented
Status: Needs work » Needs review
StatusFileSize
new simplesamlphp_auth-admin_ui_pages-2907182-4.patch2.08 KB

Added group annotation to test.

Status: Needs review » Needs work

The last submitted patch, 4: simplesamlphp_auth-admin_ui_pages-2907182-4.patch, failed testing. View results

kerasai’s picture

kerasai commented
Status: Needs work » Needs review
StatusFileSize
new simplesamlphp_auth-admin_ui_pages-2907182-6.patch2.62 KB

Updated patch to include config schema to allow functional tests to install without errors in UI.

Status: Needs review » Needs work

The last submitted patch, 6: simplesamlphp_auth-admin_ui_pages-2907182-6.patch, failed testing. View results

kerasai’s picture

kerasai commented
Status: Needs work » Needs review
StatusFileSize
new simplesamlphp_auth-admin_ui_pages-2907182-8.patch4.2 KB

Another update to config schema.

Status: Needs review » Needs work

The last submitted patch, 8: simplesamlphp_auth-admin_ui_pages-2907182-8.patch, failed testing. View results

kerasai’s picture

kerasai commented
Status: Needs work » Needs review
StatusFileSize
new simplesamlphp_auth-admin_ui_pages-2907182-10.patch4.8 KB

Another update to the test so set a simpleSAMLphp config file.

Status: Needs review » Needs work

The last submitted patch, 10: simplesamlphp_auth-admin_ui_pages-2907182-10.patch, failed testing. View results

kerasai’s picture

kerasai commented
StatusFileSize
new simplesamlphp_auth-admin_ui_pages-2907182-12.patch4.98 KB

Adjustment to writing the config file.

kerasai’s picture

kerasai commented
Issue summary: View changes
kerasai’s picture

kerasai commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 12: simplesamlphp_auth-admin_ui_pages-2907182-12.patch, failed testing. View results

kerasai’s picture

kerasai commented
Status: Needs work » Needs review

One last adjustment to the test coverage.

kerasai’s picture

kerasai commented
StatusFileSize
new simplesamlphp_auth-admin_ui_pages-2907182-17.patch3.82 KB

Missed the upload, here goes.

Status: Needs review » Needs work

The last submitted patch, 17: simplesamlphp_auth-admin_ui_pages-2907182-17.patch, failed testing. View results

kerasai’s picture

kerasai commented
Status: Needs work » Needs review
StatusFileSize
new simplesamlphp_auth-admin_ui_pages-2907182-19.patch5.03 KB

Attached patch with updates to permission used in simplesamlphp_auth.routing.yml.

Status: Needs review » Needs work

The last submitted patch, 19: simplesamlphp_auth-admin_ui_pages-2907182-19.patch, failed testing. View results

kerasai’s picture

kerasai commented
Status: Needs work » Needs review

This is ready for a maintainer to review.

The patch in #17 is just test coverage which demonstrates this issue. See the failure for AdminUiPageTest in the test results to see that the user with the applicable permission is served a 403 access denied response when accessing the administrative configuration page.

The patch in #19 includes the functional fix. The access permissions in simplesamlphp_auth.routing.yml are updated appropriately and the user is now able to access the pages.

Note regarding the test coverage: The test method writes a config file (if needed) into the simpleSAMLphp library. This is not ideal, but alternative is a fairly large change to allow the config file path to be specified. This could cause issues if the process running the test does not have access to write this file, but it works locally for me as well as in the drupal-ci environment. Also, the test cleans up the config file if it wrote it.

idebr’s picture

idebr commented

@kerasai Thanks for working on this issue. I have reused your code to fix the simpletest to create a patch for #2898517: Fix Tests for 8.x-3.x-dev branch

snufkin’s picture

snufkin commented

Would it make sense to break the patch up? I think introducing permission control is a no brainer fix, and thanks for highlighting the issue. I also see the point in the remaining changes, but it serves a different purpose and raises continuity questions (e.g. will existing sites break as a result, do we need to think about a migration path).

kerasai’s picture

kerasai commented

Recalling this from (shaky) memory, it's been a while since I wrote this...

The primary change is the fix to permissions, to allow users to navigate to admin pages.

The secondary fix is only to get the tests to run. Specifically, config schema added to a few previously un-schema'd config values. The tests bomb during the module install without that change, as I believe the test bot is using a more-strict error tolerance than us normal folk. Assuming the additional schema is correct, these changes should have no affect on currently working environments and should fix environments where the installation is broken.

dakku’s picture

dakku commented

looks good to me, unless anyone has objections, we can merge this?

idebr’s picture

idebr commented

I would suggest fixing the existing tests first at #2898517: Fix Tests for 8.x-3.x-dev branch and then reroll this patch. Then we can check if the added test coverage correctly checks if the configuration page is available when the user has the 'administer simplesamlphp authentication' permission.

dakku’s picture

dakku commented

Hi Ide,
I can look to committing ticket #2898517. Would you be willing to re-roll this patch?

idebr’s picture

idebr commented

Yes sure, I can do that :)

dakku’s picture

dakku commented

Perfect, thanks Idq, tests are now committed. Lets see if we can re-roll this patch please!

idebr’s picture

idebr commented

I rerolled the patch to remove the parts that were committed in #2898517: Fix Tests for 8.x-3.x-dev branch

Locally the test fails with the error:

Warning: include_once(/lib/_autoload.php): failed to open stream: No such file or directory
include_once()() (Line: 218)

Perhaps kerasai can have another look at the test, now that the existing tests are working again?

idebr’s picture

idebr commented
StatusFileSize
new simplesamlphp_auth-admin_ui_pages-2907182-30.patch4.63 KB
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

Looks like this is still an issue. The module now has a working test that uses a test module to fake the auth manager, should make it much easier to add checks for accessing those settings pages in that existing test.

idebr’s picture

idebr commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-31-33.txt5.99 KB
new 2907182-33.patch4.29 KB

#32 Moved the test to the existing \Drupal\Tests\simplesamlphp_auth\Functional\SimplesamlphpAuthTest

idebr’s picture

idebr commented
StatusFileSize
new interdiff-33-34.txt476 bytes
new 2907182-34.patch4.29 KB

Switched the base theme to classy, since the existing test relies on its html markup.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Fixed

Thanks, looks good and as a bonus, gets rid of the last remaining deprecation message when running tests, great.

  • Berdir committed d8ca723 on 8.x-3.x authored by idebr 
    Issue #2907182 by kerasai, idebr: Admin UI Pages not Accessible via...

Status: Fixed » Closed (fixed)

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