Accessing multilanguage variables depends on 'administer site configuration'

I've made a patch for this, changes the required permission to “access administration pages”, which most users with access to admin pages should have.

I'm not sure what the reasoning for the original check is, but this seems like it would be an adequate remedy, at least for our use case.

Upgrading to “bug” status. Unless I'm very much mistaken, translating variables should not require the “Chernobyl bit” that “administer site configuration” is.

I agree that “administer site configuration” is way too powerful a permission to use for content translation. On my site, translating variables is more about content administration than site configuration, so I'd rather see the module use "administer content" than “administer site configuration”. But, "access administration pages" is ok too, for me. RTBC as far as I'm concerned...

"Access administration pages" seems like too low of a permission for people to be able to override global variables as there are security implications. I do realize the actual (usually inoccuous?) variables that can be edited are set elsewhere, (ie. admin/config/regional/i18n/variable), but I am just wondering if this patch isn't introducing additional security holes?

Maybe we could define a new permission called "override global variables" and set "restrict access" in hook_permission to TRUE, and test for (user_access('administer site configuration') || user_access('override global variables')) in that same bit as the patch? But even with this, isn't there still a worry that giving the "override global variables" permission to untrusted users could pose a security issue?

Just had a junior spend hours trying to figure out why a customer's changes weren't being saved. This could definitely be classified as a bug. Having a form available that does nothing because you do not have a rather unrelated permission is at the very least confusing.

I support the suggestion in #4 to assign a specific permission for editing variables. "administer site configuration" is just way too permissive to hand out lightly.

+ for #4

I guess there is no reason for this patch, since that you can define your own permission using hook_variable_group_info().

Then there is still need for a patch, given that the variables are exposed by the Variable module. So they need to be fixed here. Good catch, though!

I don't think it is a good idea.

This is a generic permission for editing variables and really, the 'Variable Admin' module is just intended for site administrators with the highest permissions.

Modules providing a specific realm should handle themselves who can edit / access it. So take the 'Variable Admin' module as a last resource tool for *site administrators* only.

If you need a page for any other user to have this low level access to variables you can either:
- Build it yourself with another module
- Just alter the the variable_admin permissions in hook_menu_alter.

Then make it a sandbox module, you don't want to have security issues filed for it because your module overrides other modules' access permissions.

So in short: Cool. But not in a module I'm maintaining.

Hey guys, I think we can create a new permission and then assign it to the roles we want, right? We don't need to hardcode a permission. Do you think it is a good practice? By default UID 1 is going to have access and then you can customize it to match anything you want. So neither parts lose anything.
Another idea would be, why not creating a specific permission for each realm? So it's more generic and customizable.
I can bring the patch for review if you want, I didn't upload it now because I feel like it would put effort for nothing.

Anyway, this is the workaround I'm using to quickly make my translatable settings form to work.
Just call it before you load all your defaults values.

/**
 * Helper to switch the language of the variables to the proper one.
 *
 * @see https://www.drupal.org/node/2268415
 */
function _mymodule_prepare_variables_realm() {
  if (empty($_GET['variable_realm_key_language'])) {
    return;
  }

  $language = $_GET['variable_realm_key_language'];

  variable_realm_switch('language', $language, FALSE);
  variable_realm_rebuild();
}

#9: This is a bug and should be fixed. Let me explain the issue

I created a custom hook_menu callback which renders my variables for a certain variable group.
This page is accessible using my custom permission defined in the hook_menu, AND shows the variable language switch.

For users who do not have the "administer site configuration" permission, these variable-values show up using the original value regardless of the language selected.
This results in breaking translated variables if they save the form.

If you dont suppose to have access to variable translation, we should at least hide the language switch for those users, although I prefer it would just work using an existing "translate" permission though.

I agree this is a bug. The module is exposing realm links which don't work as expected (the variables are not loaded as they should with the query params).
As for the solution: Hiding the links for users that don't have administer site configuration is the only real fix for this bug.

Besides this point I see a bunch of people (including myself) struggling with the concept that the administer site configuration permission is really the best solution. But I agree this part is a feature request.
A special permission for this, off course with a warning like 'Give to trusted roles only; this permission has security implications.', seems to provide much better access controll for all kinds of different situations. Without direct security implications. This permission could also be used to show/hide the links to switch realms above a form.

Off course other modules can also implement a init hook and add a custom permission. It would make the most sense to do it in the place it belongs though. I would kindly ask you to reconsider.

Here's a patch that adds a new permission for the variable realms. A permission per realm would probably be better, at least my patch migrates the old to the new permission.

The building of $role_names is not needed, user_roles() already returns role names keyed by role ID? And you can pass $permission to user_roles(), then you get only those roles that have the "administer site configuration" permission.

We should use the old array() syntax so that this keeps working on ancient PHP 5.3 sites.

Thanks for the review. This patch addresses all the issues.

Fix the last array and improved the update function.

Cool! I just looked into field_update_7004() how drupal core updates permissions and it seems they are using _update_7000_user_role_grant_permissions() instead of user_role_grant_permissions(). The problem with user_role_grant_permissions() is that it gets all available permission modules from module_implements() which might be cached.

So although _update_7000_user_role_grant_permissions() is a private function I think we should use that instead. It is also used by field module in core, so this should be ok.

Ok, I replaced user_role_grant_permissions() with _update_7000_user_role_grant_permissions for this new patch as requested.

Nice, thanks!

I still don't think this is a good idea, please see again https://www.drupal.org/project/variable/issues/2268415#comment-9864953

This patch would certainly be a security issue.

Ah right, we should mark the permission as "restrict access" => TRUE to indicate that it can have security compromising consequences. Would that work for you?

To be honest, I don't quite get the argument that this is a security issue.

The fact is that you can *save* variables for the given language if you can access the respective settings page but you can't see them. How is being able to change the value OK but seeing it isn't is something I don't understand ;)

Many modules have settings permission just for their specific page, and you can grant just those, e.g. the eu cookie compliance module, you can access that directly without any other permission.