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.

Files: 
CommentFileSizeAuthor
#40 ctools-n870938-40-d7.patch4.62 KBDamienMcKenna
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch ctools-n870938-40-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#39 ctools-n870938-39-d7.patch3.79 KBDamienMcKenna
PASSED: [[SimpleTest]]: [MySQL] 51 pass(es).
[ View ]
#38 ctools-n870938-38-d7.patch3.84 KBDamienMcKenna
PASSED: [[SimpleTest]]: [MySQL] 51 pass(es).
[ View ]

Comments

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.

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.

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

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).

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.

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.

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.

subscribe

Project:Chaos tool suite (ctools)» Views
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']

Project:Views» 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.

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.

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.

@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.

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.

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.

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.

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.

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

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

<?php
     
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.

Project:Views» 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.

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

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.

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.

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.

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

For example:

<?php
$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
        )
)

<?php
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?)

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.

@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.

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?

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

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. :/

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?

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

@dereine, thanks. I'll try it.

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).

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.

Status:Active» Needs review
StatusFileSize
new3.84 KB
PASSED: [[SimpleTest]]: [MySQL] 51 pass(es).
[ View ]

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.

StatusFileSize
new3.79 KB
PASSED: [[SimpleTest]]: [MySQL] 51 pass(es).
[ View ]

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.

StatusFileSize
new4.62 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch ctools-n870938-40-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Updated to include an API change for the new permission.

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

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.

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.

Issue tags:+Novice

Testbot:
> Unable to apply patch.

So patch needs re-roll. Tagging as Novice.

subscribing(can't find 'Follow' button)

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.