[D6] ExtAdmin

welcome!

Ah, this is sweet! I was searching for this some months ago and was already wondering how long it'll take until this gets implemented :)

Note that you have to work in a version specific branch to create releases later. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.

I've seen you're already working on your coding style issues on ventral.org, great!

I'm looking forward to have a deeper look on this

Please consider to get a review bonus and we will come back to your application sooner.

Hi bukharov!
It seems you rename your master branch to 6.x-1.x-dev. It's correct?

You must have a master branch and also a development branch with the format: 6.x-1.x. Now your branch's name is 6.x-1.x-dev.
See guidelines: http://drupal.org/node/1127732

Otherwise, it seems that contrib directory in your module is unnecessary.

I think you must correct these errors before a deeper revision.

Hi @bukharov,

I think you need a master branch as well as the development branch.

Also when I clone the repository git clone http://git.drupal.org/sandbox/bukharov/1479182.git extadmin

I Get

Cloning into 'extadmin'...
remote: Counting objects: 10083, done.
remote: Compressing objects: 100% (4997/4997), done.
remote: Total 10083 (delta 4831), reused 9843 (delta 4674)
Receiving objects: 100% (10083/10083), 38.01 MiB | 4.98 MiB/s, done.
Resolving deltas: 100% (4831/4831), done.
warning: remote HEAD refers to nonexistent ref, unable to checkout.

Maybe re-instating the master branch would help?

Awesome project, I'm going to leave it as that as I think there might be someone better placed to review the code - good luck!

I am starting a formal code review on this module now. It is large so it may take some time. I just wanted to post a separate comment re http://drupal.org/node/1484270#comment-5956958. @bukharov, I think the confusion about branches is because you have the correct commands in comment #8, but the wrong commands still at the top of the ticket. If you can you might want to update those. I'll try to finish this in the next few hours.

Another note: your Git repositories are huge both for this module and the required theme. This is because you have the extadmin/ build folders in there, and even though you've removed them in recent commits, they're still in the history. It takes a long time to clone this way. Please see http://stubbisms.wordpress.com/2009/07/10/git-script-to-show-largest-pac... for suggestions on how to remove them from the history permanently if you don't need them any more.

My code review is below. Other reviewers should note that I have NOT reviewed the theme, only the support module. This is a large project!

1. Your README.txt instructions are not helpful for installation. You should definitely put in that the ExtAdmin_Theme is required, and that module should have enough instructions to download and enable it. Once this is done all I got was "Loading ExtAdmin..." because I didn't see that you had a SECOND README.txt in ext/ that required me to download this library.

It would also be good to call out the fact that the GPL version of the library is the one that should be downloaded. It's a big download (42MB) and it would be nice to know if the free version is going to work before going to the trouble of installing it.

No error message was given. it just showed "Loading ExtAdmin..." and stopped.

2. I don't think it's such a good idea in extadmin.module:21 to interfere with all admin/* links just because this module doesn't need them. Is there a good reason for this? Many Drupal admins learn the direct URLs to the things they need as an emergency back-door when things like SimpleMenu, AdminMenu, and other admin tools (like ExtAdmin) fail. It seems like it would be better to allow them to still access these URLs unless there is a reason not to do it.

3. extadmin_theme is required for this module to function... but it is not listed as a requirement in extadmin.info.

4. I suppose this is your choice but I'm not sure I understand why both a module and a theme are required. You could move your theme functionality into your module and use a simple menu hook to allow you to do the page template / script / etc overrides you are doing.

The theme is very simple. It would be a lot easier to install this tool if only the module was required. You don't even use your theme settings in your theme - you call theme_get_settings from the module and use them there. This is really just a suggestion but it would be MUCH easier to review if there was no dependency like this.

5. After installing ExtJS per the secnod README's instructions, the Loading message is now a bit more styled, but the module does not work. I get these errors:

Failed to load resource: the server responded with a status of 404 (Not Found) [mydevsiteURL]/sites/all/themes/extadmin_theme/css/Loader.css
Failed to load resource: the server responded with a status of 404 (Not Found) [mydevsiteURL]/extadmin/api
Failed to load resource: the server responded with a status of 404 (Not Found) [mydevsiteURL]/extadmin/api
Uncaught TypeError: Cannot read property 'name' of undefined

My dev site is not on /, it is in a subdirectory /d6/. This module is not properly handling these paths. I am thus unable to use the module. I will complete the code review that I can cover just from reading the code itself, but cannot RTBC it until the above bug is fixed.

I debugged part of this to ext/api.js which hard-codes this API URL:

Ext.direct.Manager.addProvider({"url":"\/extadmin\/api", ...

Obviously this URL needs to account for Drupal being in a base_path that is not /. There may be other issues like this - this is just the first one I found. [Update: here's another in Login.js line 37:
window.location.href = '/admin?' + Math.random();
]

6. Note to other reviewers: I did a PAReview on this but it's almost useless:
http://ventral.org/pareview/httpgitdrupalorgsandboxbukharov1479182git

This module itself is good. But the Ext interface modules are formatted to their standards, not Drupal's. I don't see how they could possibly pass automated code review.

7. The one thing the report picked up on and I confirmed in several places is the lack of sanitization for the data. Many $_POST variables are used directly without any check_* calls on them. The way the router works also bypasses Drupal's built-in form handling, specifically the way it builds hashed form IDs to prevent XSS and MITM attacks. I will need to study this more to determine whether there is actually a security risk here, but certainly all inputs should be sanitized - just because a user can access the admin theme does not mean they have site-wide global permissions. I am also adding the Security tag to this ticket.

8. How is this module going to support custom content types. Once the bug in #5 is resolved I will want to test this on a site that has many non-standard ctypes. Please document any special steps that are required to enable these aspects. I was able to try this out on your demo site, but you do not have CCK installed so there's no way to try out those aspects.

9. All files need @file comments even if they're JS support files. See app.js.

10. Minor typo in Login.js line 43:

action.result.error || Drupal.t('Unable to process request. Pleas try again later.')

==> "Please"

Your code quality is very good and you have done a lot of work here. I would love to see this released and quickly. But I cannot complete the review any further without being able to run the module. If you can address the points above I will try to get back to you within 24 hrs with a response. Please be patient. Your module is large and complex, and not all Drupal developers are familiar with ExtJS. It may take a few review/needs work cycles to get through it all.

Thanks for the reply, @bukharov. Regarding #2, could you please document this in your README.txt and provide admins with an alternate mechanism to undo the module installation? Many admins can't use Drush on their sites. If you override all admin/* links there would be no way to turn off this module if it was misbehaving (somebody accidentally renames a file...) Is it possible to either stitch in a simple menu hook that can be used to tell the module to bypass itself temporarily, or something along those lines?

Regarding #7, Drupal does validate forms, but you don't check to see if form_id was actually valid. I think the fear is of having a malicious user sniff or guess the form ID of an action performed by a previous admin. You aren't sanitizing everywhere. For instance, in your access-check extadmin_extadmin_access() function, you use the POST form_id if method is Process, which you also accept directly from a POST parameter. It might be a good idea to check how the parameter is formatted before doing a %param% query, which would allow me to inject things like 'a:0:{}', maybe getting you to find a row you weren't expecting to receive, particularly if the admin has a module in there that itself is doing something odd.

Regarding #8, I understand and that's totally your choice. You've already done a huge amount of work here and this is code review not suggestion review. :) But please do document that in README.txt, particularly if any of these five modules will be a problem, since they are in use on a huge number of the sites out there:

cck
views
panels
context
wysiwyg

You may also want to note if things like admin_menu will cause problems/conflicts, although it looks like what you do would simply hide them.

Sorry for the delay, but you have not listed any reviews of other project applications in your issue summary as strongly recommended in the application documentation.

I'll do a review of the extadmin module here.

Review of the 6.x-1.x branch:

• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

1. You should prefix all your functions with "extadmin", not just "ea" to avoid name collisions with other modules. "extadmin" is not that long anyway.
2. Placing the ext JS library in your module folder is not a godd idea, this will make updating the module harder. The Libraries API module is a recommended method for adding 3rd party dependencies.
3. ExtAdmin Theme project page is too short, see http://drupal.org/node/997024
4. "throw new Exception(t('Access denied.'));": Who will catch that exception? Why can't you simply use drupal_access_denied()?
5. "watchdog('extadmin', $call->action . '.' . $call->method . ': ' . $e->getMessage(), (array) $call, WATCHDOG_WARNING);": this is vulnerable to XSS exploits because the attributes come from unsanitized $_POST values. You need to use @ or % placeholders with watchdog() messages, or otherwise that could result in an XSS exploit if an admin views a malicious log message.

The security issue is a blocker. Get a review bonus and I will review/approve this again as soon as possible.

The unsanitized watchdog() call is a security blocker.

Wow that's a huge amount of work you did here ! Very interesting module indeed !

I've been playing around with this for some time and I found the following issues:

- If the theme is installed and activated before the module is enabled the installation is locked in a situation where is no possible to access to the administration and disable the theme again. That's really a problem...

-On install of ea_local I got a fatal error.
Fatal error: Call-time pass-by-reference has been removed in /var/www/d6/sites/all/modules/extadmin/modules/ea_locale/ea_locale.module on line 117
Remove the reference on &$form to solve this.

- I tried the demo and it worked at first but the second time I access it the ajax loader keeps spinning and stayed like that.
With my local installation it was even not possible to make it work a first time.
There was no JS error on console for any of those. I use firefox 16 under ubuntu.

I would like to be of some more help but this module is more related with ExtJs than Drupal.

theme depends on module.
it is not possible to set dependencies[] = ext_admin on the theme dot info then ?

Just a sidenote. In the demo all XHR response is text/html, but the content is actually JSON. Whose responsibility is that? You or Drupal? Drupal has JSON helpers: drupal_json() (I didn't any see actual code).

Hello bukharov

I checked the demo of your module and it looks promising. I put a git clone 10 mins back and still its downloading. only 22% done and already 14 mb downloaded. May be you have to check that, if you can decrease anywhere anything. Hope you did not keep the ext js libraries in the module that it became so heavy.

Also check above 2 comments from reviewers.

Once you done that, change the status back to needs review.

Is this request still active?

But a short comment about your last comment:

Don't use header, there is a drupal function for that: http://api.drupal.org/api/drupal/includes!bootstrap.inc/function/drupal_...
Also there is a function to output JSON data with the correct header: http://api.drupal.org/api/drupal/includes!common.inc/function/drupal_jso...

Ah sorry, didn't realized it.
But there are D6 equivalents - drupal_set_header and drupal_json.

Now a full manual review:

extadmin.module:
- Libraries API would be good to use
- Line 21: Use arg(0) instead of $_GET['q']
- Line 694, 793, 836: Use module_invoke instead of call_user_func
- Line 894, 910: If you are using json_encode/decode than require php 5.2 (Add in the .info php = 5.2)
- Line 863: watchdog still not uses placeholder for user input (This is a important security issue!)

extadmin/modules/ea_comment/services/CommentService.class.php:
- Line 75: Unused variable $node

extadmin/modules/ea_path/services/UrlAliasService.class.php:
- Line 37: $a = $a - Equal assignment

Otherwise I like what you did there. I would set it to RTCB if you fix the one security issue, the other things are in my mind only nice to have and I wanted to note them.

Remove the .gitignore file from your repo.

I'm a little concerned about the security in Drupal forms given the alterations introduced here. Can you provide more detail about why the changes were needed, and your view of the security implications?

The demo looks great - thanks for providing that.

----
Top Shelf Modules - Enterprise modules from the community for the community.

Thanks for the additional info regarding forms. I thought we had a rule about not including .gitignore files, but I can't find any evidence of one.

----
Top Shelf Modules - Enterprise modules from the community for the community.

Thanks for the additional info, the module still looks good to me and it's been over a month. This issue has been waiting a long time, thanks for sticking with it!

Thanks for your contribution, bukharov!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

----
Top Shelf Modules - Enterprise modules from the community for the community.