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.

CommentFileSizeAuthor
#1 683630-node-remove-stupid-D7.patch2.64 KBdave reid

Comments

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Active » Needs review
StatusFileSize
new 683630-node-remove-stupid-D7.patch2.64 KB

Removes the check for 'administer nodes' permission. Let's see how the bot likes it.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Issue tags: +D7DX

Yes, please.

In fact, we should probably open a meta-issue to remove all access checks from validate and submit functions.

moshe weitzman’s picture

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

Indeed. These checks are pure cruft.

drewish’s picture

drewish commented

subscribing

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

See #683778: Remove permission checks from _validate() and _submit() handlers for the meta issue.

dww’s picture

dww
we/he/they
commented

<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>

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Looks good to me. Committed to CVS HEAD.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

Arg 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.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

So 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.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Priority: Normal » Critical
Status: Fixed » Needs work
Issue tags: +Needs tests
dww’s picture

dww
we/he/they
commented

@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...

int’s picture

int commented

2 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"

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Priority: Critical » Normal
Status: Needs work » Fixed
Issue tags: -Needs tests

We'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.

dww’s picture

dww
we/he/they
commented

@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

Status: Fixed » Closed (fixed)
Issue tags: -D7DX

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

Issue tags: +D7DX

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.