hook_node_grants implementations lead to a 'URL Alias' validation error when saving translated nodes.

Created on 16 December 2019, over 4 years ago
Updated 16 May 2024, about 1 month ago

Problem/Motivation

Whenever a value is present in the URL alias field while saving a node or its translation, ValidPathConstraintValidator calls $this->pathValidator->isValid($path). If the site uses any contrib or custom modules that implement hook_node_grants() then the access() method in NodeGrantDatabaseStorage queries the node_access table to check the access to determine if the path is valid or not. In the current logic, core uses the language code of the translation being saved in the db query. Now consider the following scenario

  • Node has English as default language and is published.
  • User tries to create a new translation.

In both cases, the node_access table is queried with the language code of the translation being saved to check the access. However, there won’t be any record for the unpublished translation, causing the validation to fail and resulting in the validation error “Either the path '%link_path' is invalid or you do not have access to it”.

Check comments #122 🐛 Cannot save translated nodes after upgrading to 8.8 due to invalid path Needs review to #125 for more detailed analysis.

Steps to reproduce

  • Install with demo_umami profile
  • Give "Editorial workflow: Use Create New Draft transition" and "Article: Create new content" permissions to the editor role.
  • Check the node_access table. There will be only one entry with nid 0 which is the default record.
  • Create a custom module and implement hook_node_grants(). Install the module.
/**
 * Implements hook_node_grants().
 */
function my_module_node_grants(AccountInterface $account, $operation) {
  $grants = [];

  return $grants;
}
  • Login as admin and visit any admin page. There will be a message to rebuild the permissions. Click on Rebuild permissions to rebuild all permissions.

  • Login with any of the editor accounts (Eg: uid=3) in incognito mode.
  • Add an English article. Give a path in URL alias field and publish it.

  • Try to add spanish translation.

Proposed resolution

There have been 3 suggested solutions based on where the problem is.

  1. : But Node access table gets updated everytime permissions are rebuilt. So, not a robust solution.
  2. , #20 Reported to worķ. #21, #23, #24, #26, #27, #28, #46 and Reported to not work; #44, #45
  3. #79. Reported to work #83, #85. But will not work if the alias field is visiblie in node edit form
  4. Use fallback language in node_access query when the translation is new: #64, #122 to #125: Needs review

Note that In #30, the problem was not reproducible with core only. The suggestion is that access control modules are causing the problem. See #30, #31

Remaining tasks

Create a merge request - Done
Add tests - Done
Review MR 6210
Commit

User interface changes

API changes

Data model changes

N/A

Release notes snippet

🐛 Bug report
Status

Fixed

Version

11.0 🔥

Component
Path 

Last updated 6 days ago

  • Maintained by
  • 🇬🇧United Kingdom @catch
Created by

🇩🇪Germany rgpublic Düsseldorf 🇩🇪 🇪🇺

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • 🇬🇧United Kingdom nicrodgers Monmouthshire, UK

    Setting back to Needs Work, as the patch in 99 failed, and the test-only patch passed but should have failed.

  • 🇧🇪Belgium flyke

    3101344-99.patch doesnt fix it for me.

    My project is currently on Drupal 9.5.3.
    The patch applies without problem.

    The project has 3 languages:
    - NL (Dutch) -> is the default language
    - FR (French)
    - EN (English)

    The client/content editor created a node in FR (so not the default language) without a problem and published it.
    Now he tries to create an EN translation of that node that is unpublished.
    If he tries to save that unpublished EN translation he gets the error
    Either the path '/node/123' is invalid or you do not have access to it

    The other patches here also did not fix this error.
    I also tried this but that did not fix it either.

  • 🇧🇪Belgium flyke

    In my case, editing a translation via 'Edit translations' does not work, which has this path structure:
    /node/123/translations/edit/en
    But editing the translation directly via admin/content does work, which has this path structure:
    /node/123/edit?language_content_entity=en

    Even as admin user, the /translations/edit link does not work while the ?language_content_entity edit link does.

  • 🇧🇪Belgium daften

    In our case, the patches that work on \Drupal\path\Plugin\Field\FieldWidget\PathWidget::validateFormElement don't work, because the widget is active. Having the widget active should not keep this bug active. I don't agree Alex, this is not a problem present because of a widget, is an underlying issue in the node_grants system.
    The patch from #64 fixes it for us.

  • 🇺🇸United States jasloe

    FWIW, #64 🐛 Cannot save translated nodes after upgrading to 8.8 due to invalid path Needs review resolves this issue for me on a multilingual Drupal 10.0 site.

    Before applying the patch, users with correct node editing and translation permissions attempting to create a new translation receive the following message...

    Either the path '/node/[some-nid]' is invalid or you do not have access to it.

    ...no message is written to the error log.

  • 🇮🇪Ireland frankdesign

    Patch at #64 fixes for me with 10.1.2. I also have View Unpublished installed.

  • 🇳🇿New Zealand quietone New Zealand

    I tried to reproduce this Drupal 10.1.x using the steps in the Issue Summary (again) and in #98 and was not able to.

    I did update the patch, making the change asked for in #88. Also made changes to comments and switched to an MR.

  • Status changed to Needs review 8 months ago
  • @quietone opened merge request.
  • Status changed to Postponed: needs info 8 months ago
  • 🇺🇸United States smustgrave

    Also have not been able to reproduce. Could someone confirm this is still an issue in D10?

  • Status changed to Needs review 8 months ago
  • Still an issue on D10 (10.1.6).

    Not sure if it was mentioned before but for reproducibility, in my case this bug only manifests itself when creating translation from interface language different from original node language. For example, while this /en/node/276/translations/add/en/cs works (adding translation for originally EN node from EN interface), this does not /cs/node/276/translations/add/en/cs (adding translation for originally EN node from CS interface - and this path where button to add translation uses).

  • Status changed to Needs work 8 months ago
  • 🇺🇸United States smustgrave

    Confirmed steps should be added to the IS.

    Alsolast MR has test failures.

  • Updated steps to reproduce in IS with point mentioning interface language being important when trying to reproduce bug.

  • 🇧🇪Belgium borisson_ Mechelen, 🇧🇪

    The steps to reproduce that are in this issue are not sufficient. I can't reproduce the problem with these steps, I tested this on 11.x

    1. Use sth. other than the administrator account. You should have the usual editor permissions to create nodes and create/update translations and translate any entity, of course. You should not have any url alias permissions.
    2. Create a new node in the original language (in my case: German)
    3. Save the node
    4. Edit the node again
    5. Click on translation, add translation (English)
    6. Site interface of translation form should be now in target (English) language (e.g. path /en/node/123/translations/add/de/en)
    7. Try to save
  • 🇧🇪Belgium borisson_ Mechelen, 🇧🇪

    I just had @Mschudders reproduce this, as we are sitting next to each other at the moment. They can reproduce it on their instal without issues, but that is not a core checkout but rather a site with a bunch of contrib modules enabled.
    I think this is only a problem with a certain set of contrib modules?

  • 🇧🇪Belgium Mschudders

    Adding screenshot which will help in debugging/finding the issue:

  • 🇧🇪Belgium borisson_ Mechelen, 🇧🇪

    Even with pathauto installed and a configuration for articles, I can't trigger the error.

  • 🇳🇱Netherlands tinto Amsterdam

    I've seen this problem on one of our client sites too. Perhaps @Mschudders and I could compare a list of contrib modules installed so we can cross-check?

  • 🇮🇳India Akhil Babu Chengannur

    Here is how I reproduced this issue in Drupal 10.2

    • Install with demo_umami profile
    • Give "Editorial workflow: Use Create New Draft transition" and "Article: Create new content" permissions to the editor role.
    • Check the node_access table. There will be only one entry with nid 0 which is the default record.
    • Create a custom module and implement hook_node_grants(). Install the module.
    /**
     * Implements hook_node_grants().
     */
    function my_module_node_grants(AccountInterface $account, $operation) {
      $grants = [];
    
      return $grants;
    }
    
    • Login as admin and visit any admin page. There will be a message to rebuild the permissions. Click on Rebuild permissions to rebuild all permissions.
    • Login with any of the editor accounts (Eg: uid=3) in incognito mode.
    • Add an English article. Give a path in URL alias field and publish it.
    • Try to add spanish translation.
  • 🇮🇳India Akhil Babu Chengannur
  • 🇮🇳India Akhil Babu Chengannur

    Patch #99 will only work if "URL alias" field is not rendered in the edit form (ie: User does not have 'Create URL alias" permission) as it skips the validation completely. It wont work if the field is accessible to the user.
    I think patch #64 is the best way to fix this bug and here is why:-

    validateFormElement() method in Drupal\path\Plugin\Field\FieldWidget\PathWidget invokes validations whenever there is an entry in the "URL alias" field. This eventually calls access() method in NodeGrantDatabaseStorage. This method checks if any modules have 'hook_node_grants' implementations and, if so, it checks the node_access table for access grants. The db query uses current language code of the node as a condition.

    The "node_access" table is updated only when nodes are published. If you check the node access table now, there will be only one entry for node with id '20' (The one created in previous step) and language code will be 'en'. When we try to save the spanish translation, code checks the table with 'es' language code. There won't be any entry and access() method returns AccessResult::neutral()

    This result is eventually passed to Drupal\Core\Routing\AccessAwareRouter checkAccess(). This method throws AccessDeniedHttpException if access is not allowed which causes the validation error.

  • 🇮🇳India Akhil Babu Chengannur
  • 🇮🇳India Akhil Babu Chengannur

    Translation can be added without any errors if the original English content is kept in draft state. In this scenario also, there won't be an entry in node_access table for the new node in both 'en' end 'es' languages. So access() method in NodeGrantDatabaseStorage would stll return neutral access. But access checks also invoke 'content_moderation_entity_access' and it returns AccessResult::allowedIfHasPermission($account, 'view any unpublished content'). So user with 'view any unpublished content' permission would be able to save the translation. Later both translations can be published and updated without any errors.

  • 🇮🇳India Akhil Babu Chengannur

    Patch #64 fixes the error by checking the fallback language if the translation is new.
    Committing that patch as an MR. Will also add a test to show the bug.

  • Pipeline finished with Failed
    5 months ago
    #78666
  • Pipeline finished with Failed
    5 months ago
    Total: 169s
    #78668
  • 🇮🇳India Akhil Babu Chengannur
  • Pipeline finished with Failed
    5 months ago
    Total: 162s
    #78800
  • Status changed to Needs review 5 months ago
  • 🇮🇳India Akhil Babu Chengannur
  • 🇮🇳India Akhil Babu Chengannur

    Akhil Babu changed the visibility of the branch 11.x to hidden.

  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7 updated deps
    last update 5 months ago
    Composer error. Unable to continue.
  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.2 & sqlite-3.34
    last update 5 months ago
    CI aborted
  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MariaDB 10.3.22
    last update 5 months ago
    29,235 pass, 26 fail
  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.2 & pgsql-14.1
    last update 5 months ago
    CI aborted
  • 🇮🇳India Akhil Babu Chengannur
  • 🇮🇳India Akhil Babu Chengannur
  • 🇮🇳India Akhil Babu Chengannur
  • 🇮🇳India Akhil Babu Chengannur
  • 🇩🇪Germany diqidoq Berlin | Hamburg | New York | London | Paris

    The issue is full of very useful comments and brainstorming hooks helping to get a good picture of why it can't be reproduced every time, etc. Great work in here and Thanks @Akhil Babu! +1 for your further steps and information provided in the last comments. Wake up call for all of us.

  • Pipeline finished with Success
    5 months ago
    Total: 770519s
    #78802
  • 🇺🇸United States smustgrave

    Just fyi helps to hide the old patches and MRs. There is a new section in the standard template for which MR to review but its new so not surprised it isn't here. Have hidden the patches for clarity

    Ran the test-only features

    1) Drupal\Tests\path\Functional\PathWithNodeAccessGrantsTest::testAliasTranslation
    Behat\Mink\Exception\ResponseTextException: The text "Basic page test has been updated." was not found anywhere in the text of the current page.
    /builds/issue/drupal-3101344/vendor/behat/mink/src/WebAssert.php:811
    /builds/issue/drupal-3101344/vendor/behat/mink/src/WebAssert.php:262
    /builds/issue/drupal-3101344/core/modules/path/tests/src/Functional/PathWithNodeAccessGrantsTest.php:102
    /builds/issue/drupal-3101344/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
    ERRORS!
    Tests: 1, Assertions: 22, Errors: 1.
    

    Left 2 small comments on the MR but overall looks good.

  • Pipeline finished with Success
    5 months ago
    Total: 570s
    #84080
  • 🇩🇪Germany quotientix

    Patch in #64 works for me in 10.2.2 - thanks for the team effort here!

  • Status changed to RTBC 5 months ago
  • 🇺🇸United States smustgrave

    Feedback appears to have been addressed.

  • Status changed to Needs work 5 months ago
  • 🇬🇧United Kingdom catch

    One question on the MR.

  • Pipeline finished with Canceled
    5 months ago
    Total: 224s
    #85213
  • Pipeline finished with Canceled
    5 months ago
    Total: 101s
    #85216
  • Pipeline finished with Failed
    5 months ago
    Total: 547s
    #85218
  • Pipeline finished with Canceled
    5 months ago
    Total: 139s
    #85242
  • Pipeline finished with Success
    5 months ago
    Total: 615s
    #85243
  • Status changed to Needs review 5 months ago
  • 🇮🇳India Akhil Babu Chengannur

    Feedback addressed. Moving back to needs review.

  • Status changed to Needs work 5 months ago
  • 🇺🇸United States smustgrave

    If going to do that way probably don't need the $langcode variable anymore.

  • Pipeline finished with Success
    5 months ago
    Total: 505s
    #85798
  • Status changed to Needs review 5 months ago
  • 🇮🇳India Akhil Babu Chengannur

    Sure. $language variable was unnecessary, so I've removed it.

  • Status changed to RTBC 5 months ago
  • 🇺🇸United States smustgrave

    Feedback appears to be addressed.

    • catch committed e6fe4180 on 10.2.x
      Issue #3101344 by Akhil Babu, Alex Bukach, ravi.shankar, quietone,...
    • catch committed f665483c on 11.x
      Issue #3101344 by Akhil Babu, Alex Bukach, ravi.shankar, quietone,...
  • 🇬🇧United Kingdom catch

    Good to see this one ready. I did my best with issue credit but there are a lot of comments on this issue so apologies if anyone got overlooked.

    Committed/pushed to 11.x and cherry-picked to 10.2.x, thanks!

  • Status changed to Fixed 5 months ago
  • 🇬🇧United Kingdom catch
  • Automatically closed - issue fixed for 2 weeks with no activity.

  • First commit to issue fork.
Production build 0.69.0 2024