og_get_default_roles() does not return values as documented instead returns roles keyed by *arbitrary* integer [#1616950]

og_get_default_roles() function comment indicates that it returns an array of roles keyed by their declaring module. However, it returns an array of roles with *arbitrary* keys.

The default og_get_default_permissions() (and og_roles_overrides()) appear to use arbitrary values as returned by og_get_default_roles(). This will break permissions inheritance when another module is installed after og has been in use. If the new module implements hook_og_default_roles and sets a weight less than og, or is alphabetically before og the "rids" returned by og_get_default_roles() will shift. The keys currently returned by og_get_default_roles() should not be used as rids as the values are arbitrary. If this new module is enabled and disabled the rid => role mapping will shift again resulting in og_roles_override behaving buggy and problematic default role permission inheritance.

This appears to have broken in #1547014: Remove Global permissions.

The previous function declaration of returning keyed by module name was very valuable for the work in #1349754: features integration for organic groups permissions and roles. However if that is not useful elsewhere the features work can implement the necessary code.

Comment	File	Size	Author
#2	og-1616950-2.patch	3.55 KB	jgraham
#2

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

amitaibu

Hebrew

Israel

CreditAttribution: amitaibu commented 5 June 2012 at 18:18

@jgraham, are you gonna work on this? :)

Comment #2

jgraham CreditAttribution: jgraham commented 6 June 2012 at 07:34

Status:

Active

» Needs review

File	Size
og-1616950-2.patch	3.55 KB

reworked og_default_roles() to be keyed by implementing module and then the roles
removed $include parameter for og_default_roles() didn't appear to be used anywhere
Fixed several typos in comments as well as reworking comments as necessary.
Reworked og_default_permissions() to build around new structure in og_default_roles() returned array and to use the role names as the key rather than the arbitrary role index
Reworked og_roles_override() to reflect new output of og_default_permissions(). We use og_default_permissions as the generator for the roles so that we have a flattened array rather than the multidimensional array from og_default_roles().
Adjusted the db_update() call in og_roles_override to explicilty only be called if we are overriding roles for a group instance. I think it was safe before, but running this when not overriding on a specific group instance doesn't make sense.

I think that summarizes the changes.

Comment #3

6 June 2012 at 07:40

Status:

Needs review

» Needs work

The last submitted patch, og-1616950-2.patch, failed testing.

Comment #4

jgraham CreditAttribution: jgraham commented 14 June 2012 at 20:32

Priority:

Major

» Normal

Adjusting severity. It looks like the potentially dangerous code was entirely non-functional. The db_update query was querying rid on string values which would have matched zero rows on the integer valued rids stored in the database. So no permission assignments would have "role-hopped" as I previously surmised.

Just need time to review the tests and see why they failed and if they need some updates.

Comment #5

jgraham CreditAttribution: jgraham commented 18 July 2012 at 22:40

Category:	bug	» feature
Status:	Needs work	» Closed (won't fix)

Closing this out as "won't fix" since I think #1349754: features integration for organic groups permissions and roles is the only code that would benefit from this rewrite, and adjusting status to "feature request" as it may not be a bug as indicated in comment 4.