if you go directly to /admin/access/roles/edit/1 or /admin/access/roles/edit/2 you can edit and delete the roles "anonymous user" and "authenticated user"

CommentFileSizeAuthor
#9 protect-default-roles_2.patch702 bytesrstamm
#7 protect-default-roles.patch959 bytesrstamm
#5 user.module_48.patch950 bytesprofix898
user.module_47.patch1.01 KBrstamm

Comments

pfaocle’s picture

pfaocle
He/Him
Primary language English
commented
Version: 4.7.0-beta6 » x.y.z

Confirmed here: could be a nasty one. Haven't tested patch.

rstamm’s picture

rstamm commented
Priority: Normal » Critical

I think its a critical bug

wulff’s picture

wulff commented

The patch solves the problem, but wouldn't it make sense to place the if block before the database query?

rstamm’s picture

rstamm commented

You are right.

It's only a simple and quick solution.

profix898’s picture

profix898 commented
Status: Active » Needs review
StatusFileSize
new user.module_48.patch950 bytes

+1 for Flanker's patch, works for me.

I think wulff is right, we should check before loading role object from db.
Corrected patch attached.

moshe weitzman’s picture

moshe weitzman commented
Priority: Critical » Minor

there are a lot of imaginary urls which don't behave properly. do we actually present this link anywhere? how does a user get to this url? this sort of defensiveness juts clutters the code IMO.

rstamm’s picture

rstamm commented
StatusFileSize
new protect-default-roles.patch959 bytes

rerolled patch

Kjartan’s picture

Kjartan commented
Status: Needs review » Needs work

Patch fails to apply.

rstamm’s picture

rstamm commented
Status: Needs work » Needs review
StatusFileSize
new protect-default-roles_2.patch702 bytes

re-rolled

AmrMostafa’s picture

AmrMostafa commented

Applied against latest CVS, works as expected.
While I agree with moshe in principle, I think we should fix imaginary URLs that could break Drupal, like this one. The fix ain't that big too ;-).

Very minor, may be unnecessary comment..
For code consistency, I think you should use in_array() like in line 1948:

if (!in_array($rid, array(DRUPAL_ANONYMOUS_RID, DRUPAL_AUTHENTICATED_RID))) {
chx’s picture

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

Most imaginary URLs are already protected by menu or other subsystem. Indeed, breaking Drupal is not a desired effect..

drumm’s picture

drumm
he/him
Location NY, US
commented
Status: Reviewed & tested by the community » Fixed

Committed to HEAD.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)