Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Currently we hard-code some checks for authored by and authored on node fields by surrounding their validation and processing in node_validate() and node_submit() with user_access('administer nodes'). If users don't have access to those fields, it should be impossible to submit invalid data, so why should we bother skipping it. This only hurts modules like Override node options that alter core node form elements to make them accessible by individual permissions.
| Comment | File | Size | Author |
|---|---|---|---|
| #1 | 683630-node-remove-stupid-D7.patch | 2.64 KB | Dave Reid |
Comments
Comment #1
Dave ReidRemoves the check for 'administer nodes' permission. Let's see how the bot likes it.
Comment #2
agentrickardYes, please.
In fact, we should probably open a meta-issue to remove all access checks from validate and submit functions.
Comment #3
moshe weitzman CreditAttribution: moshe weitzman commentedIndeed. These checks are pure cruft.
Comment #4
drewish CreditAttribution: drewish commentedsubscribing
Comment #5
agentrickardSee #683778: Remove permission checks from _validate() and _submit() handlers for the meta issue.
Comment #6
dww<hat class="security-team">Yes, definitely RTBC. This patch is making core more secure since we're now *always* validating these fields, not just for folks with 'administer nodes'.
</hat>Comment #7
Dries CreditAttribution: Dries commentedLooks good to me. Committed to CVS HEAD.
Comment #8
Dave ReidArg we didn't have any test coverage to know this was broken, but it is even before this patch landed. You can't change node authors. Working on a quick test.
Comment #9
Dave ReidSo the check for !isset($node->uid) is pointless because it's *always* going to be defined. We should just check of $node->name is set.
Comment #10
Dave ReidComment #11
dww@Dave Reid: #8 implies this is a completely distinct bug, separate from this change. I'm not sure why we're piling it on here. Why not open a new critical bug and mark this one fixed? This didn't have anything to do with the problem...
Comment #12
int CreditAttribution: int commented2 Issues for one bug, is too much.. We need a new Status Message, instead of one tag Needs tests, I think is better to know this Issue is fixed, and only "Needs tests"
Comment #13
Dave ReidWe've got an existing critical issue open at #492186: Authoring information is never updated. for not being able to update node names. Reverting meta-data and marking as fixed.
Comment #14
dww@int: I think you misunderstand me. I'm not proposing 2 issues for 1 bug. There are two completely unrelated bugs. This bug is about pointless access checks that make life difficult for contrib modules. The other bug is about not being able to change the owner of a node when you edit it. They just happen to touch nearly the same lines of code... My point was that it's silly to keep talking about the 2nd bug in the issue that already fixed the first.
@Dave Reid: Thanks. That's exactly what I meant. ;)
Cheers,
-Derek