Add a new permission to CTools to control importing rather than depending on the core PHP usage permission and then use that new permission in Views to solve this original problem:

"Only UID1 have access to "Import view from code". My other used has the administrator role and all permissions checked, but only Add new views is visible."

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Fixed

Okay, now with this commit the other admin needs "use PHP for settings", too.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

tsvenson’s picture

Status: Closed (fixed) » Active

Does this mean I have to enable the PHP module to allow other than UID1 to be able to import views from code? If not, the problem with that only UID1 have access to importing code is still there in latest dev.

dawehner’s picture

Assigned: Unassigned » merlinofchaos

In d6 it was

use PHP for block visibility

which is part of block.module.

In d7 "use php for settings" is used which is part of the php.module, which is not always enabled.

Assign to merlinofchaos to decide

* to drop "use php for settings" and just check for "administer views"
* to add a "import view" permission
* keep "use php for settings" and require php.module for imports
* another alternative

greggles’s picture

I agree with the idea that it should be possible for someone other than uid 1 to be able to import views.

I agree with the goal of making it obvious that anyone who can do that can also execute arbitrary PHP and therefore it needs to be somewhat hard to do that.

I think it's fine that Views require the php.module be enabled and the "use php for settings" permission in order to import views. People who want to import views without enabling php.module can use something like exportables and code in a views.inc file (or similar).

merlinofchaos’s picture

Ok, so my thinking is this:

Ability to import views (and other exportable objects via export.inc): Common need, but known to be dangerous.
Ability to embed php code directly into sites: Common need but discouraged.

Therefore, I think we don't really want to encourage people to enable php.module which activates a dangerous and discouraged but sometimes necessary feature to access a dangerous but not discouraged and often necessary feature.

Now, the *real* solution to this is to rewrite exports to use a safe format, such as JSON. That is not, however, trivial, plus it has to be able to use BOTH code paths since we don't want to embed JSON code in our PHP files. That'd be very weird. But using JSON code to pass exports from site to site is safe.

Since core no longer offers this permission, perhaps the answer is for CTools to offer a permission. Views relies on CTools, plus, CTools contains export.inc and that will spread this to pretty much every module out there that currently uses views-style exporting.

greggles’s picture

but known to be dangerous.

I don't think that's the case. I think people have no idea it's dangerous.

Really the two things are equivalent in terms of risk. Calling one "dangerous but discouraged" vs. "dangerous but not discouraged" is just pointing out a logical inconsistency in what we discourage: we should discourage both equally!

Makes sense to me to put a new permission that sounds sufficiently scary into CTools.

Unnamed’s picture

subscribe

I do not know PHP.
And I think that if this functionality is declared in an interface, it should work.
I spent two days trying to set up views to understand why the output data only on the first user. : (
While in Drupal 6, it works.
Me as a beginner using this function without programming it would be easy enough to implement a functional portal, but with the error of my opportunities are severely limited.

FrequenceBanane’s picture

subscribe

mlncn’s picture

Project: Chaos Tool Suite (ctools) » Views (for Drupal 7)
Version: 7.x-1.x-dev » 7.x-3.x-dev

Bumping this, as it should not be necessary to enable PHP module only for the setting to exist to import views. Sure, any time you're feeding Drupal raw PHP there is potential danger, and there should be a "know the source of your export and know what you're doing" warning, but copy-pasting views (and other exportables) from one site to another is a bit safer than putting arbitrary PHP in nodes and blocks :-)

Should we move this to the CTools queue or open a CTools issue?

[edit: corrected 'raw HTML' to 'raw PHP']

dawehner’s picture

Project: Views (for Drupal 7) » Chaos Tool Suite (ctools)
Version: 7.x-3.x-dev » 7.x-1.x-dev
Component: User interface » Code

At least for the import there should be a permission in ctools.

greggles’s picture

copy-pasting views (and other exportables) from one site to another is a bit safer than putting arbitrary PHP in nodes and block

No, it really isn't.

merlinofchaos’s picture

What greggles says is truth.

The ultimate solution is to get a non PHP output format (say, JSON) for exports that can be used for copy-pasting imports. That way bulk export still uses PHP and a paste import uses JSON. That's a little bit involved.

But after talking with greggles, I'm afraid the permissions here are unlikely to change.

pwolanin’s picture

@merlinofchaos - in #6 you assert that we would not want to embed JSON in our PHP files, but to me embedding it or having separate JSON-only files would be preferable to having 2 separate code paths.

At Drupalcon David Strauss was suggesting that for D8 we export settings to JSON files, so this would seem similar to me.

merlinofchaos’s picture

My fear is that parsing them would be unnecessarily slow. I suppose we could set up a test where we set it up on a reasonably complex export and bench it.

pwolanin’s picture

Yes, that's certainly worth doing such a benchmark. i think json_decode() is reasonably fast. As a side note, however, doesn't Views serialize and cache the built view after reading it from the file? I thought it only went to the file at all when the Views cache is cleared.

merlinofchaos’s picture

Because it's now using export.inc in D7 it does not, and in fact that capability probably needs to be restored in CTools' export.inc.

effulgentsia’s picture

If performance issues aren't a problem or can be solved, +1 to moving from PHP to JSON as the standard import/export format in CTools and D8 core.

For the time being, if CTools allows executing php code without the php module enabled, can we at least have a setting for turning that off? So that someone can run a site that strips that permission away (e.g., via settings.php), even for user 1.

David_Rothstein’s picture

The JSON stuff is obviously a better long-term solution, but in terms of the specific issue here, I don't think it would be too bad for Ctools to define a new permission for this, would it?

As long as it's abundantly clear (e.g. via setting 'restrict access' => TRUE and a clearly-written permission description) that granting this allows arbitrary PHP code to be executed, then it seems like Ctools import is generic enough functionality that it's OK to bend the rules for it.

Although it would be good if all PHP code execution via the UI in Drupal 7 were restricted to sites that have the PHP module enabled (and I think it was someone's intention that that be the case), the reality is that:

  1. Turning on the PHP module has other side effects that not everyone wants (e.g., creation of a new text format).
  2. Even Drupal core itself does not follow the "rule" here: #932110: On some servers, the Update Manager allows administrators to directly execute arbitrary code even without the PHP module
merlinofchaos’s picture

Hmm. At the moment it looks like we're still using this:

      case 'import':
        return user_access('use PHP for block visibility');

Since that permission doesn't exist in D7, uh, I guess only uid 1 can use it right now. Something that got missed in the updates.

mlncn’s picture

Project: Views (for Drupal 7) » Chaos Tool Suite (ctools)
Version: 7.x-3.x-dev » 7.x-1.x-dev

Exactly. Plus one David Rothstein. (Having a permission to use PHP with CTools is better than making people enable PHP module-- that's what i was trying to say, and Greggles already proposed that, so let's do it.)

How about, modeled on PHP module's permission, including the caps in the permission name...
'use PHP for ctools import' => array(
'title' => t('Use PHP for importing exportables'),
'restrict access' => TRUE,
),

And then +1 on moving this an lots of Drupal configuration to JSON.

pwolanin’s picture

How hard would it be to do a JSON implementation so we can at least benchmark them side by side?

merlinofchaos’s picture

Not too hard, honestly. The vast majority of exportables use the default implementation of exports, so I think just replacing that implementation with a JSON export would not be too diffuclt, as least as an exercise.

I think in D7, we will have little choice but to keep PHP, though. So if we introduce JSON exports, it has to be an option in D7 which we can then eliminate in D8.

merlinofchaos’s picture

Also, one thing I'm really concerned about, and we need to address this:

JSON does not have arrays the way PHP does. Arrays that are not purely sequential (i.e, most of them) are actually objects. This could have an impact on the structure of objects we want to export, as we could seriously lose 'arrayness' and that could be a real issue in some places.

merlinofchaos’s picture

Another issue with JSON: A view, for example, is not just a stdClass object, it's a view object. I'm not sure JSON has any way of actually doing that.

pwolanin’s picture

json_decode() has the option to make either stdClass objects OR associative arrays from its hash structures.

For example:


$json = '{"names":["alice","fred"], "result":"good", "info":{"zip":"20001"}}';
print_r(json_decode($json));
stdClass Object
(
    [names] => Array
        (
            [0] => alice
            [1] => fred
        )

    [result] => good
    [info] => stdClass Object
        (
            [zip] => 20001
        )

)

print_r(json_decode($json, TRUE));    
Array
(
    [names] => Array
        (
            [0] => alice
            [1] => fred
        )

    [result] => good
    [info] => Array
        (
            [zip] => 20001
        )

)

Also, there is no reason you can't have a top wrapper level to tell you what kind of object this is, etc.

In that case you'd basically need a way to instantiate or populate the properties of a views object or any other object by giving it an array.

I don't see that the constructor currents takes any arguments (also uses a deprecated style where the constructor is named after the class?)

merlinofchaos’s picture

Category: bug » task

After spending some time thinking about this:

1) I changed the "use PHP for block visibility" to "use PHP for settings" which I'd failed to catch several times.
2) JSON exports would have to be simpler than the current exports but I think that is doable. Right now the exports create fully configured objects, but really they shouldn't be. They should just be exporting storage bits just like loading from the database. In theory that would make them smaller, as well.
3) It will still take a significant amount of time to retool to make this possible, but it's on the list of things I would like to do.

tsvenson’s picture

@merlinofchaos: Nice to see there is some progress on this. Personally it doesn't really affect me anymore since I have moved over to using the Features module to manage views, and everything else.

Still, this would improve the experience for many users of smaller, less complicated sites than those I'm working on.

danillonunes’s picture

Maybe I'm wrong, but it's not possible to export using PHP's serialize() so the import can be safer than when it needs to interpret a pure PHP code?

dawehner’s picture

One major problem would be that the export would be unreadable and un-versionable.
Nowadays many people use exposed to module to be able to keep track of their "code"-development

merlinofchaos’s picture

Also a serialized export can't contain code and as of now, VIews exports are actually full code, not just a single object export. This is a design flaw. :/

khiminrm’s picture

Hi, all! After update to Drupal 7.7 and latest Ctools version I can't get working this code for anonymous user?

$view = views_get_view('myview');
$view->execute();
if (isset($view->result)) {
    print $view->render(); 
   }

It worked before updates.
Please, help. Is this not possible already?

dawehner’s picture

This is total out of scope of this issue. Better use $view->preview and forget $view->execute and render.

khiminrm’s picture

@dereine, thanks. I'll try it.

Pasqualle’s picture

As an intermediate solution can we add a new permission (into ctools or core php module)? As a short goal is to allow import of exportables, but do not enable PHP input anywhere else in the Drupal UI (to achieve no PHP in db).

Pasqualle’s picture

pwolanin’s picture

Would it be possible to make Views exports a single object instead of code? That would make it much easier to use JSON or XML as an alternate serialization.

DamienMcKenna’s picture

Status: Active » Needs review
FileSize
3.84 KB

I think the permission issue needs work. If the long-term solution for the security problem is to replace the PHP export format then a better short-term solution is still needed. Placing a soft requirement on the PHP module, i.e. using the permission, but not requiring the module itself be enabled gives a false sense of how it works and leads confusion & duplicate issues (e.g. #1216408: Only user-1 can import context). If a new permission is not going to be required then the module needs to a) require the PHP module be enabled before showing the import functionality and b) provide some indication of how the permission really works.

This patch extends all user_access checks with an additional check to see if the PHP module is enabled, and also gives an indicator on the Site Status page (via hook_requirements) if the PHP module is not enabled.

DamienMcKenna’s picture

FileSize
3.79 KB

Thinking on it further, this really needs to be a separate permission. My rationale:

  • Very few site will want to have the PHP filter enabled, due to security concerns.
  • You cannot control the 'user PHP for settings' permission if the PHP module is not enabled.
  • If you attempt to work around #1 and #2 (by enabling the PHP module, assign the permission and then disabling the module again) future permissions changes will erase the setting.
  • Once CTools is updated to use a new export format (be it something described here or something to be compatible with CMI for D8) a new permission will be needed anyway, at that point relying upon the PHP module will make no sense.
  • Wording can be used to indicate the functionality is a security issue and should only be given to trusted users.

I suspect this might be an API change, so probably needs further refinement.

DamienMcKenna’s picture

FileSize
4.62 KB

Updated to include an API change for the new permission.

Pasqualle’s picture

I agree with the change in #40, this is badly needed.

OsamaBinLogin’s picture

Issue tags: +Error messages

For this reason, there should at least be an error message saying why you can't do this-or-that. I, too, sometimes spend two days spinning my wheels, trying to figure out some unintuitive Gotcha, that I could have fixed quickly given a good error message in the right place.

I get it that giving away too much information is a security risk; eg bad signin should not say which is wrong, the username or pw - that's state information that could help a hacker. But info that is in the Drupal docs, and/or is known by some drupal programmers, is already exposed and there's no reason to withhold it from users who have not yet combed through the first 100 pages of Drupal docs.

Status: Needs review » Needs work

The last submitted patch, ctools-n870938-40-d7.patch, failed testing.

sergiu.savva’s picture

Status: Needs work » Needs review
Issue tags: -Error messages

#40: ctools-n870938-40-d7.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Error messages

The last submitted patch, ctools-n870938-40-d7.patch, failed testing.

geek-merlin’s picture

Issue tags: +Novice

Testbot:
> Unable to apply patch.

So patch needs re-roll. Tagging as Novice.

RoSk0’s picture

subscribing(can't find 'Follow' button)

Anybody’s picture

Issue summary: View changes

I can confirm that this bad issue still exists in the latest versions. There has been no progress for a longer period of time.

lokapujya’s picture

Assigned: merlinofchaos » Unassigned
Status: Needs work » Needs review
Issue tags: -Novice
FileSize
4.68 KB

Rerolled.

-  // Restrict variant import to users who can already execute arbitrary PHP
-  if (user_access('use PHP for settings')) {
+  // Restrict variant import due to security implications.
+  if (user_access('use ctools import')) {

This is my first look at ctools code. But,

<?php
if (user_access('use PHP for settings') || user_access('use ctools import')) {
?>

might prevent an API change.

Status: Needs review » Needs work

The last submitted patch, 50: 870938-50.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
4.62 KB

Forgot to rebase.

  • merlinofchaos committed 0347c0b on 8.x-2.x
    Issue #870938 Update "php" setting to PHP module for ability to import...

DamienMcKenna queued 52: 870938-52.patch for re-testing.

Michelle’s picture

Title: "Import view from code" only possible for UID1 » Add new permission for controlling imports
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Error messages

The patch in #52 applies cleanly and the permission shows up and it works for controlling "admin/structure/pages/import". In that respect, it is working fine.

However, it doesn't actually solve the original problem since it doesn't affect the menu access for "admin/structure/views/import". Because the code that needs to be changed for that is in Views, I filed a new issue to use this permission once it is committed and am updating this issue to focus on the CTools part. #2396925: Change permission for views import to use new CTools permission

DamienMcKenna’s picture

Cross-referencing the other issue.

mrjmd queued 52: 870938-52.patch for re-testing.

mrjmd’s picture

japerry’s picture

Status: Reviewed & tested by the community » Fixed

I agree that we need to move to JSON based imports, but I doubt that will happen with D7. Given the age of this issue and movement on that issue.

The issue in #39 and re-rolled in #52 looks good to me and committed. Views and other modules that implement their own permissions will have to be updated to use the API if they want to make use of this feature.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

megadutch1956’s picture

Excuse my stupid question, but where does this patch go? In what file?

Michelle’s picture

It was already committed on January 17th so you just need to use a more recent release. No need to patch.

greggles’s picture

FileSize
49.34 KB

I sort of lost track of this issue at some point and was brought back to it by a similar issue in the views module - #2329259: Refine import permissions / update php argument access.

IMO this is not a particularly effective solution because it still lets uid 1 use this feature and execute arbitrary php code.

The justification for this patch starts with:

Very few site will want to have the PHP filter enabled, due to security concerns.

Yes. Exactly :) "Importing" a view via the php code *also* executes arbitrary php code in the exact same way. If having php module enabled scares you then so should the ability to import views/ctools/etc. For demonstration purposes:

Try it out for yourself, quite a fun tool.

Anyway, I'd be very happy to work on a patch that makes this feature depend on php.module (whether in this issue or not). I encourage people who care about blocking the execution of arbitrary php to use the paranoia module

Pasqualle’s picture

make this feature depend on php.module

The problem is:
- I want to use import
- I never want to use php filter

If I enable the php module, I have the php filter..

greggles’s picture

What I'm trying to point out is that if you never want to use php filter then you probably don't want to have the ability to use import. Or really: you shouldn't.

DamienMcKenna’s picture

So how about splitting out the import functionality that includes lots of security warnings, and add some warnings to the page itself? The Paranoia module could also include it in its lists of things to block.

greggles’s picture

The paranoia module already does include blocking the import functionality for views module and more can be easily added by form_id or by url :)

I don't understand the proposal about splitting it out. I think to be effective that the warning has to be before the module is downloaded/installed.

jomarocas’s picture

FileSize
435 bytes

ok the patch of this issue working good https://www.drupal.org/node/2396925 i upload again the patch to apply in the next release please, and credit to @Michelle for the patch

Marc DeLay’s picture

@greggles, This whole conversation is kind of absurd because this whole export/import through the interface model is antiquated in the Drupal world.
But the idea that requiring a user to take two dangerous and unrelated actions in order to accomplish the one dangerous action they want to is also a very bad idea. This creates a definite possibility that the admin does not remember to disable the PHP input filter after finishing their views importing because it is not in any reasonable way related to the task they were trying to accomplish.