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"

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pfaocle’s picture

Version: 4.7.0-beta6 » x.y.z

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

rstamm’s picture

Priority: Normal » Critical

I think its a critical bug

wulff’s picture

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

rstamm’s picture

You are right.

It's only a simple and quick solution.

profix898’s picture

Status: Active » Needs review
FileSize
950 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

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

FileSize
959 bytes

rerolled patch

Kjartan’s picture

Status: Needs review » Needs work

Patch fails to apply.

rstamm’s picture

Status: Needs work » Needs review
FileSize
702 bytes

re-rolled

AmrMostafa’s picture

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

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

Status: Reviewed & tested by the community » Fixed

Committed to HEAD.

Anonymous’s picture

Status: Fixed » Closed (fixed)