I'd like to thank the effort of making this module available for D7 first.

The following code in the module makes role names with non-ASCII character to be stripped off, entirely in some cases.

<?php
$perm
.= preg_replace('/[^a-zA-Z0-9]/', '', $role_name);
?>

The regular expression totally replaces Chinese/Japanese characters with nothing, for example. The role name ended up with an empty string, and making the module unfunctional.

A quick patch is not to replace any character, but use the role name as is. I don't know why the role name can be used as is...

I noticed some other minor I18N related issues (I don't know they warrant a bug report by themselves).

  • The module is somehow reported to have a non UTF-8 character. I found N+~ character (used in Spanish, I guess), which is not saved as UTF-8.
  • The permission screen cannot be translated due to lack of t() function. E.g. 'Edit users with no custom roles'
Files: 
CommentFileSizeAuthor
#5 administerusersbyrole-transliterate-5.patch835 bytesNiremizov
PASSED: [[SimpleTest]]: [MySQL] 1,141 pass(es).
[ View ]
#3 administerusersbyrole-transliterate-0.patch825 bytessmokris
PASSED: [[SimpleTest]]: [MySQL] 1,141 pass(es).
[ View ]

Comments

v1adimir’s picture

it may be dangerous just removing
$perm .= preg_replace('/[^a-zA-Z0-9]/', '', $role_name);

We can use transliteration module API to generate latin name for hook_permission()

Third party developers seeking an easy way to transliterate text or file names
may use transliteration functions as follows:

if (function_exists('transliteration_get')) {
  $transliterated = transliteration_get($text, $unknown, $source_langcode);
}
NobuT’s picture

it may be dangerous just removing

Just curious, why is it dangerous? Is there any possibility of injecting JavaScript or something to the role name? The regular expression also prohibit the use of some letters with acute or umlaut. I just wonder what was the intention of the regular expression...

smokris’s picture

Version:7.x-1.0-beta1» 7.x-1.x-dev
Status:Active» Needs review
StatusFileSize
new825 bytes
PASSED: [[SimpleTest]]: [MySQL] 1,141 pass(es).
[ View ]

It's dangerous because some characters are not allowed in permissions strings, and invalid characters can result in database corruption.

I added the preg_replace call to address #709068: Square brackets [] in role name. There are a bunch of characters that aren't permitted to be used in permissions strings, but *which* characters aren't permitted unfortunately doesn't seem to be documented anywhere. In addition to square brackets mentioned in that issue, this comment says that commas are invalid.

We can use transliteration module API to generate latin name for hook_permission()

That sounds like a reasonable solution. If transliteration.module is installed, we should transliterate to Latin, then apply the existing preg_replace. Patch attached.

NobuT’s picture

Thank you, Steve. I didn't know there are characters that aren't permitted in permission strings, while they are accepted as a role name. Very interesting.

Niremizov’s picture

StatusFileSize
new835 bytes
PASSED: [[SimpleTest]]: [MySQL] 1,141 pass(es).
[ View ]

Thx for the patch @smokris. I have applied it, but unfortunatelly it does not work as desired. For some reason you have transliterated $perm not $role_name variable. So I rerolled your patch, see below.

AdamPS’s picture

Assigned:Unassigned» AdamPS
Issue summary:View changes

I intend to fix this as part of #2378869: Meta-issue for Beta 2 release. Please sign up as a follower of that issue and there will shortly be a patch that I would like feedback on.

The fix is to use role ID rather than role name to build the permission. We still use the name for the title that is visible to users.

Thanks for the patch, but I think role ID approach is better and also fixes several related issues, e.g. renaming roles.

AdamPS’s picture

Issue tags:+beta2
AdamPS’s picture

Version:7.x-1.x-dev» 7.x-2.0-beta1
Status:Needs review» Closed (fixed)