Problem
In uuid_default_entities_example.module we create a bunch of default entities to demonstrate the UUID module's capability of exporting/importing entities for things like default demo content etc. But at the moment there are some sub-optimal things with this:
- It's not very obvious what happens when you enable that module, i.e. people don't realize what content that really has been created.
- The example user that's being created happens to have the administrator role which can make people a bit worried (I was lazy/stupid doing it that in the first place to circumvent "permission problems" I think :P).
Suggestion 1
Change the default entities in code:
- To make things more obvious we should use more explicit labels (node titles, term names and user names) for all entities that are created. My suggestion here is to append "(UUID example)" to all labels of the entities created.
- Remove the
administratorrole from the example user (and as per previous point, also change its name to something more obvious).
And an update hook:
And for sites that, for some weird reason, are running with this module enabled, or didn't remove the example content after trying out the module, we can provide an update hook renaming the entities (1), removing the role from the user (2).
Suggestion 2
Remove uuid_default_entities_example.module all together. I don't think it fills a big enough purpose. The functionality is probably used by very few people(?).
After removing the module we can implement the update hook from Suggestion 1 and be done with it.
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | uuid-labels-in-examples-2047393-13.patch | 1.2 KB | btopro |
| #4 | 2047393-remove-default-entities-example-1.patch | 13.14 KB | dixon_ |
Comments
Comment #1
dixon_My favorite is Suggestion 2 from above, because I don't think the
uuid_default_entities_example.modulefills a big purpose.I used the same kind of functionality for a project of mine once, and wanted to show how it worked. But the whole thing is very weird (it depends on deploy for exporting the entities etc.). So I don't think it's very useful and many people can't be using it.
Any other suggestions?
Comment #2
dixon_Comment #3
dixon_Better title
Comment #4
dixon_Here's a quick patch (totally not tested yet) to show how I was thinking with the update hook.
And obviously, since we're just killing a module like this we should write in the next release note (immediately after committing this):
"Please uninstall uuid_default_entities_example.module before running with this update"
Edit: I think that's an ok way to do it considering the UUID module is still marked as alpha stability.
Comment #5
bvirtual commentedI just spent 20 minutes tracking down a "security" issue, a new userid mohamed was added to my test D7 web site, along with a page. Only by viewing the web log, seeing it happened when I enabled the example module, did I realize it was not a cracked Drupal web site issue, but a severe lack of documentation for the module.
I am not likely the only one who urgently tried to find out who hacked the Drupal web site. Drupal does NOT deserve to get a reputation for allowing modules to cause havoc for the system administrator.
This is a critical issue, and should be fixed ASAP. IMHO
The easiest fix I can think of is to document this creation on the modules page inside the Description for the module with something like this:
Enabling this example module will create a new user "mohamed" with FULL ADMIN rights and a new example web page.
The above documentation should include all other created or touched database tables. Surprises are not nice.
It would be better if after the test is completed, the userid was blocked, downgraded from Administrator. Yes?
Most certainly the web page that is created should be SELF DOCUMENTING. That means it should state in one or more languages how it got created, why, and that it can be deleted at any time, with no impact.
It should also state the userid can be deleted.
And the web page should state what else can be cleaned up.
Finally, the Drupal code conventions should include the concepts mentioned above. And any module not adhering to them should not make it out the sand box. Why?
Security is important.
Creating the appearance of a break in ... is not desirable. Ask Sony who published discs that installed rootkits in order to protect their copyright material. Yes, to me, and others, these two are equivalent, in appearance of a hacked computer.
It's not nice to cause false alarms. When using desirable software like Drupal. IMO
Hope this helps places some perspective on the lack of documentation this example module can cause, has caused.
Comment #6
bvirtual commentedI'll add the page status should be changed to unpublish. Why? Just search for the body content of the example page, with exact match on, and see dozens of web pages, public pages, with this example page on it. Making a 'test' example page be 'published' so search engines can spider it... is that wise?
Comment #7
bvirtual commentedI will test your patch when the updates I have suggested have been made to it. I see at least one mod is in place already, to reduce the admin rights of the user. Good thoughts Dick.
Comment #8
dixon_I should have committed this patch a long time ago, but please let me know how testing goes and I'll commit it soon!
Thanks
Comment #9
dixon_Committed.
Comment #11
darol100 commentedThis issue still happening in this version. 7.x-1.0-alpha5. I feel like this is a backdoor for my sites.
Can anyone fix this issue ? And release a new update of the module with this fix?
Comment #12
btopro commentedHas anyone been able to run upgrade.php for this update? I'm getting the following error for it:
Fatal error: Class name must be a valid object or a string in drupal-7/includes/common.inc on line 7837Comment #13
btopro commentedfigured it out. The patch that made it through assumes that node, user, and taxonomy are enabled on the site. Node and user are required to make any site function but taxonomy is not. If you have a site that you do not have the taxonomy module enabled for and you run this update it will fail and will continue to fail till you enable taxonomy module.
I've submitted a patch against what's already been committed to test for the existence if the taxonomy_term entity prior to attempting to do an entity info load against it. Loading against an entity type that doesn't exist causes the following fatal error so this is a critical in my mind:
Fatal error: Class name must be a valid object or a string in drupal-7/includes/common.inc on line 7837Comment #15
dixon_Thanks btopro! That makes a lot of sense.
Committed to 7.x-1.x.