Problem

The access check for the /user/login path is performed as the user that's changing the settings. This is problematic because a logged-in user may not have access to the login screen but may be allowed to change basic site settings. This could also cause the login path to be set to something that the logged in user has access to but an anonymous user does not.

This can be reproduced by installing the latest version of r4032login in a clean Drupal 8 install and creating a new role that has the "Administer basic site settings" permission. Assign this role to a non-admin user and attempt to change the basic site settings with the login path set to /user/login. You'll receive a form error that you're not allowed to access /user/login.

Solution

Perform the access check for the chosen login screen path as the anonymous user.

CommentFileSizeAuthor
#10 interdiff_8-10.txt383 bytesnixou
#10 3010747-access-check-10.patch3.85 KBnixou
#8 interdiff_5-8.txt478 bytesyarik.lutsiuk
#8 3010747-access-check-8.patch3.77 KByarik.lutsiuk
#5 interdiff_3_5.txt2.2 KBnixou
#5 3010747-5-perform-access-check-as-an-user.patch3.76 KBnixou
#3 interdiff-2-3.txt684 byteskingdutch
#3 3010747-3-perform-access-check-as-an-user.patch1.44 KBkingdutch
#2 3010747-2-perform-access-check-as-an-user.patch1.26 KBkingdutch

Comments

Kingdutch created an issue. See original summary.

kingdutch’s picture

kingdutch
he/him
Primary language English
commented
Assigned: kingdutch » Unassigned
Status: Active » Needs review
StatusFileSize
new 3010747-2-perform-access-check-as-an-user.patch1.26 KB
kingdutch’s picture

kingdutch
he/him
Primary language English
commented
StatusFileSize
new 3010747-3-perform-access-check-as-an-user.patch1.44 KB
new interdiff-2-3.txt684 bytes

Also changed the error message to reflect the changed expectation.

robertragas’s picture

robertragas commented
Status: Needs review » Reviewed & tested by the community

RTBC

nixou’s picture

nixou commented
StatusFileSize
new 3010747-5-perform-access-check-as-an-user.patch3.76 KB
new interdiff_3_5.txt2.2 KB

Ok I added a test to cover that but I can't make it pass with your patch, not sure why.

I upload the patch so we can see what the testbot says.

nixou’s picture

nixou commented
Status: Reviewed & tested by the community » Needs work
phily’s picture

phily
he/him
Primary language French
Location 🇪🇺🇫🇷 Paris, France
commented

Patch #5 works for me using Drupal 8.8.3 and r4032login 8.x-1.1
Thanks

yarik.lutsiuk’s picture

yarik.lutsiuk commented
Status: Needs work » Needs review
StatusFileSize
new 3010747-access-check-8.patch3.77 KB
new interdiff_5-8.txt478 bytes

Hello,

patch with updated test.

Cheers

solideogloria’s picture

solideogloria commented
Version: 8.x-1.x-dev » 2.x-dev
nixou’s picture

nixou commented
StatusFileSize
new 3010747-access-check-10.patch3.85 KB
new interdiff_8-10.txt383 bytes

Added default theme for tests under D9.

  • Nixou committed bab40b2 on 2.x authored by Kingdutch 
    Issue #3010747 by Nixou, Kingdutch, yarik.lutsiuk: r4032login should...
nixou’s picture

nixou commented
Status: Needs review » Fixed

Committed to 2.x-dev, will be released in next 2.1.0.

Thanks you all !

kingdutch’s picture

kingdutch
he/him
Primary language English
commented

I'm happy this got merged! However, I do think a follow up is needed.

The change to the test in #8 no longer tests the issue that was reported here. The test user can now link to any paths, so even with the old code the test would now pass.

I suspect the test failure was caused by one of the other path fields' validation failing (specifically I'm not sure if https://drupal.org/ is valid for the authenticated user redirect), but I don't currently have a set-up where I can verify that hypothesis.

No need to revert the code from this issue, but to avoid regressions in the future we should probably figure out how the test can work without the link to any page permission.

nixou’s picture

nixou commented

Indeed, thanks for the catch.

I looked again into this and I found that the test fail with an error "Either the path '/user/login' is invalid or you do not have access to it." which is not thrown by our code.

The point is that this test submit the form at /admin/config/system/site-information without filling the "front page" field.
When we look at core/modules/system/src/Form/SiteInformationForm.php we can see this :

/**
 * {@inheritdoc}
 */
public function validateForm(array &$form, FormStateInterface $form_state) {
  // Check for empty front page path.
  if ($form_state->isValueEmpty('site_frontpage')) {
    // Set to default "user/login".
    $form_state->setValueForElement($form['front_page']['site_frontpage'], '/user/login');
  }

So the core will fill the frontpage field with the "/user/login" path and then send an error because the user is authenticated and cannot access to this path.

This is exactly the same problem as ours but coming from the core.

As this test should not be about testing the core, we can just update it to fill the frontpage field with the path /admin/config/system/site-information.

While debugging this I also seen that there is a problem with the new validation of the path coming with this patch : an invalid path such as /abcd will not be considere as invalid.

So I'll create a follow up as you suggested to update the following things :

  1. Filling the site_frontpage field while testing
  2. Fixing the validation of the path
  3. Removing the "link to any page" permission for the test user
  4. Improving the test to check the correct behavior for incorrect and unaccessible path
solideogloria’s picture

solideogloria commented

#3191522: Incorrect validation and test for the "Path to user login form" field

Status: Fixed » Closed (fixed)

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