Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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."
Comment | File | Size | Author |
---|---|---|---|
#69 | views-use_ctools_import_permission-2396925-1.patch | 435 bytes | jomarocas |
Comments
Comment #1
dawehnerOkay, now with this commit the other admin needs "use PHP for settings", too.
Comment #3
tsvenson CreditAttribution: tsvenson commentedDoes 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.
Comment #4
dawehnerIn d6 it was
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
Comment #5
gregglesI 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).
Comment #6
merlinofchaos CreditAttribution: merlinofchaos commentedOk, 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.
Comment #7
gregglesI 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.
Comment #8
Unnamed CreditAttribution: Unnamed commentedsubscribe
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.
Comment #9
FrequenceBanane CreditAttribution: FrequenceBanane commentedsubscribe
Comment #10
mlncn CreditAttribution: mlncn commentedBumping 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']
Comment #11
dawehnerAt least for the import there should be a permission in ctools.
Comment #12
gregglesNo, it really isn't.
Comment #13
merlinofchaos CreditAttribution: merlinofchaos commentedWhat 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.
Comment #14
pwolanin CreditAttribution: pwolanin commented@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.
Comment #15
merlinofchaos CreditAttribution: merlinofchaos commentedMy 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.
Comment #16
pwolanin CreditAttribution: pwolanin commentedYes, 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.
Comment #17
merlinofchaos CreditAttribution: merlinofchaos commentedBecause 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.
Comment #18
effulgentsia CreditAttribution: effulgentsia commentedIf 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.
Comment #19
David_Rothstein CreditAttribution: David_Rothstein commentedThe 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:
Comment #20
merlinofchaos CreditAttribution: merlinofchaos commentedHmm. At the moment it looks like we're still using this:
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.
Comment #21
mlncn CreditAttribution: mlncn commentedExactly. 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.
Comment #22
pwolanin CreditAttribution: pwolanin commentedHow hard would it be to do a JSON implementation so we can at least benchmark them side by side?
Comment #23
merlinofchaos CreditAttribution: merlinofchaos commentedNot 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.
Comment #24
merlinofchaos CreditAttribution: merlinofchaos commentedAlso, 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.
Comment #25
merlinofchaos CreditAttribution: merlinofchaos commentedAnother 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.
Comment #26
pwolanin CreditAttribution: pwolanin commentedjson_decode() has the option to make either stdClass objects OR associative arrays from its hash structures.
For example:
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?)
Comment #27
merlinofchaos CreditAttribution: merlinofchaos commentedAfter 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.
Comment #28
tsvenson CreditAttribution: tsvenson commented@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.
Comment #29
danillonunes CreditAttribution: danillonunes commentedMaybe 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?
Comment #30
dawehnerOne 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
Comment #31
merlinofchaos CreditAttribution: merlinofchaos commentedAlso 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. :/
Comment #32
khiminrm CreditAttribution: khiminrm commentedHi, all! After update to Drupal 7.7 and latest Ctools version I can't get working this code for anonymous user?
It worked before updates.
Please, help. Is this not possible already?
Comment #33
dawehnerThis is total out of scope of this issue. Better use $view->preview and forget $view->execute and render.
Comment #34
khiminrm CreditAttribution: khiminrm commented@dereine, thanks. I'll try it.
Comment #35
PasqualleAs 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).
Comment #36
Pasquallerelated:
#1372850: Harden access check for flag import
#1372854: Harden access check for relation type import
Comment #37
pwolanin CreditAttribution: pwolanin commentedWould 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.
Comment #38
DamienMcKennaI 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.
Comment #39
DamienMcKennaThinking on it further, this really needs to be a separate permission. My rationale:
I suspect this might be an API change, so probably needs further refinement.
Comment #40
DamienMcKennaUpdated to include an API change for the new permission.
Comment #41
PasqualleI agree with the change in #40, this is badly needed.
Comment #42
OsamaBinLogin CreditAttribution: OsamaBinLogin commentedFor 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.
Comment #45
sergiu.savva CreditAttribution: sergiu.savva commented#40: ctools-n870938-40-d7.patch queued for re-testing.
Comment #47
geek-merlinTestbot:
> Unable to apply patch.
So patch needs re-roll. Tagging as Novice.
Comment #48
RoSk0subscribing(can't find 'Follow' button)
Comment #49
AnybodyI can confirm that this bad issue still exists in the latest versions. There has been no progress for a longer period of time.
Comment #50
lokapujyaRerolled.
This is my first look at ctools code. But,
might prevent an API change.
Comment #52
lokapujyaForgot to rebase.
Comment #55
MichelleThe 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
Comment #56
DamienMcKennaCross-referencing the other issue.
Comment #58
mrjmd CreditAttribution: mrjmd commentedComment #59
japerryI 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.
Comment #62
megadutch1956 CreditAttribution: megadutch1956 commentedExcuse my stupid question, but where does this patch go? In what file?
Comment #63
MichelleIt was already committed on January 17th so you just need to use a more recent release. No need to patch.
Comment #64
gregglesI 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:
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
Comment #65
PasqualleThe 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..
Comment #66
gregglesWhat 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.
Comment #67
DamienMcKennaSo 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.
Comment #68
gregglesThe 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.
Comment #69
jomarocas CreditAttribution: jomarocas as a volunteer commentedok 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
Comment #70
Marc DeLay CreditAttribution: Marc DeLay as a volunteer commented@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.