Currently the site information Basic Site Settings Front page validates the user has access to the route and prevents saving if this is the case.
Additionally if the user has administrative permission (i.e. user 1) and you try and add a route that you don't have permission to - user/login for example the for saves successfully but wipes the value leaving it black.
1) The form should not wipe values when the routing validation fails when submitted from an admin user.
2) The form validation as to whether the route is a valid route should be separate from whether the user submitting the form has permission over the route.
On submission we should validate that the path is a valid path.
1) As user/1 submit the admin/config/system/site-information form with a valid route
2) As user/1 attempt to submit the admin/config/system/site-information form with /user/login as the homepage.
Prevent setting the following paths:
- admin/*
- /user/*/cancel
- /node/*/delete
- other additional routes?
Validate that the route exists - but not necessary that the current user has permission.
Confirm which routes we want to prevent setting
Change the validation such that it is not user dependent.
Consider how to handle the case when the existing saved value no longer validates on form submission.
Note:
The above is a problem, because in #2288911: Use route name instead of system path in user maintenance mode subscriber, we have made the /user/login path accessible only to anonymous users, so if someone wants to use that for the default front page, then it will prevent the form from being used by any authenticated user.
Comments
Comment #1
effulgentsia commentedDiscussed with chx and his recommendation is to set #access on the element to FALSE if it is currently set to something the user can't access. That would increase security of the element, but stop that element from preventing the submission of the entire form.
That would also require moving the element validation from the form's validate method to a #element_validate.
Comment #2
andypostAlso it would be great to check that both kinds of user roles (anonymous and authenticated) are have access allowed for the path in element validation
Comment #3
chx commentedWhy? There's no reason anything but the login page should be access allowed.
Comment #14
larowlanComment #15
avpadernoI understand why the code is verifying the front page path is accessible to the user who is changing the values in that form: The front page should be accessible to every user, including the user who is editing the values in that form.
What normally happens is that users who can access the form on /admin/config/system/site-information have other permissions that allow them to access more pages than normal users. The code should not take that, since the page they set as front page is accessible from them, the front page is accessible to every user. The code should check authenticated users and anonymous users have access to that page.
As for not allowing to submit the form, I would show a warning, but allow the form to be submitted.
Comment #17
anjali rathodComment #18
anjali rathodUnassigning as the issue is still under discussion.
Comment #19
avpadernoTalking of the steps to reproduce the bug given in the issue summary, isn't /node/add/article a path probably nobody would use for the front page?
I understand the issue is that Drupal checks the path used for the front page can be accessed from the user who is changing the settings, but a path that can only be accessed from anonymous users is probably a bad example of the bug to fix. The front page should be accessible from anonymous users and logged-in users, not just anonymous users. (I would worry if logged-in users cannot see the front page, but a search bot that isn't allowed to crawl my site would be an issue too.)
I would not check the page is accessible from all the user roles set for the site, but I would check the front page is accessible to anonymous users or users with just the authenticated role. In this case, what should the form validation handler do when the path is accessible only to anonymous users or, vice versa, when it's not accessible to anonymous users? Should one of those case cause a validation error and the other just a warning?
Comment #20
anjali rathod@apaderno there are sites that keep login form on the homepage, and I do agree with the fact that homepage should be accessible to all the users, but in the issue context, the path is to create an article, I doubt if it should be accessible to anonymous user. I feel the form should validate the fact that the path shouldn't be admin path, if not then it should save any path provided in the site config by the user.
Comment #21
avpaderno@Anjali That's the point: The issue isn't with a single path that probably nobody would choose as path for the front page, but with many paths that should not be used. For example, as you said, any /admin/* path should not be used either, nor should /user/*/cancel.
We should decide which paths could not be used and in which way they should be checked. In comment #19, I suggested to only accept paths accessible from anonymous users and users with only the authenticated role, but other methods could be used to validate the path used for the front page.
The issue summary should be then updated to reflect what the effective task is. This isn't anymore a novice task, since there are decisions to take.
(Given the issue summary needs to be update, the correct status is probably Needs work.)
Comment #23
luke.stewart commentedUpdated the version target.
I've updated the issue summary based on https://www.drupal.org/project/drupal/issues/2371863#comment-14607943
Given the potential for data loss perhaps this should be updated from Minor?
Comment #25
mstrelan commentedI don't think there should be any access checks on the paths. When combined with a module like r4032login it absolutely makes sense to allow an access restricted page to be the front page. Even admin paths, e.g. for a decoupled Drupal site that might only be used for administering content, it would make sense to put
/admin/contentas the front page.