CVS edit link for abhigupta

This is my second attempt at applying for this module. This implements all the suggestions given by the previous reviewer with regards to improving security by requiring HTTP Authentication when accessing the feed, besides having a random string. Also, I have optimized the code by doing things like using globals.

The module is available here http://watchdog-feed.googlecode.com/files/watchdog_rss.tar.gz

=====================================

I have created a new module called Watchdog RSS.

This modules securely exposes the Watchdog Events as a RSS feed by creating a unique Feed URL appended with long hashed random string. This module will make is easy for site admins to stay up to date on the happenings of their website and respond to any error immediately.

CommentFileSizeAuthor
#9 watchdog_rss.tar_.gz4.12 KBabhigupta
#7 Watchdog RSS for Drupal 64.3 KBabhigupta
#5 Watchdog RSS after Coder Fixes4.76 KBabhigupta
#1 Watchdog RSS Module3.96 KBabhigupta

Comments

abhigupta’s picture

abhigupta commented
StatusFileSize
new Watchdog RSS Module3.96 KB

I am attaching the module file here. This module is for Drupal 5.

abhigupta’s picture

abhigupta commented
Status: Postponed (maintainer needs more info) » Needs review

Oops forgot to update the status.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Issue tags: +Drupal 5.x, +Module review
abhigupta’s picture

abhigupta commented
StatusFileSize
new Watchdog RSS after Coder Fixes4.76 KB

I just ran the module through Coder module and fixed all the issues including the minor ones.

Attached is the latest version of the code.

Additionally, you can look at the code here http://code.google.com/p/watchdog-feed/source/browse/#svn/trunk

izmeez’s picture

izmeez commented

subscribing

abhigupta’s picture

abhigupta commented
Issue tags: +Drupal 6.x
StatusFileSize
new Watchdog RSS for Drupal 64.3 KB

I am also attaching the Drupal 6 version of this module ... hopefully now someone can review it quickly.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Needs work

Let us keep the module version for Drupal 6.

/**
 * Watchdog RSS Admin Form to Reset the URL
 */
function watchdog_rss_admin_form() {
  $form['#base'] = 'watchdog_rss_form';
  // ...
}

As reported by the comment, that is not anymore supported.

abhigupta’s picture

abhigupta commented
Status: Needs work » Needs review
StatusFileSize
new watchdog_rss.tar_.gz4.12 KB

Hey Alberto,

Thanks for taking the time to review my module. I have fixed the issue that you pointed out. Let me know if there is anything else that you would like to me look at.

--
Abhi

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Needs work
Issue tags: -Drupal 5.x 
  1.     'description' => 'Access / Change your site\'s Watchdog RSS Feed URL.',

    Modules should avoid to escape the quotes inside strings that must be translated; the script that extract those strings would probably extract the slash too. 

  2. /**
 * Implementing hook_init to enable Watchdog feed auto detection
 */
 function watchdog_rss_init() {
}

    hook_init() is not called for cached pages; depending on the case, this could create some problems.
    The module defines a custom permission, but the implementation of hook_init() doesn't verify if the user has the permission to access the RSS feed defined from the module. 

  3.   srand((double)microtime()*1000000);
  $char_list = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
  $char_list .= "abcdefghijklmnopqrstuvwxyz";
  $char_list .= "1234567890";

  for ($i = 0; $i < $length; $i++) {
    $random .= drupal_substr($char_list, (rand()%(drupal_strlen($char_list))), 1);
  }

    See the Drupal coding standards to understand how a module code should be written. I generally report about the coding standards respect once for all the module code. 

  4. require_once ('watchdog_rss.module');

    The file to include will not be found, as the path passed to require_once is relative to Drupal root directory (which is the current directory set when Drupal makes the bootstrap). 

  5.   drupal_set_message('<strong>Watchdog RSS URL</strong>: '.
    l($link, $link) .' <br /><strong>Do Not Share this URL with others,
    as it may compromise the security of your system.</strong>');

    The string used is not translatable, should use placeholders, and within t() (which should be used) l() is not used. 

  6.   drupal_load('module', 'user');

    Why is the code loading the module user.module? user.module is always loaded, when Drupal makes a full bootstrap. 

  7.     // We need common.inc for t(), and theme.inc for theme() (called indirectly by t()).
      module_load_include('inc', 'watchdog_rss', 'common');
      module_load_include('inc', 'watchdog_rss', 'theme');

    The same is true for these code lines. the code is trying to load the include files from the module directory, which is not the directory containing common.inc, or theme.inc.

  8. The file watchdog_rss.admin.inc contains only two functions; in that case it could be incorporated into the module file.
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs work » Closed (won't fix)

The module contains code that has been copied from another module that is already in CVS; that would be fine if you would not be applying for a CVS account, and the code would be copied (and changed) correctly.

abhigupta’s picture

abhigupta commented
Status: Closed (won't fix) » Needs review

Hey Alberto,

Thanks for taking the time again. Yes, this module includes code from another module. I applied for this module before, this new issue queue system was put in place and I was told that I should include the HTACCESS authentication in my code and I think that module was suggested to me as a potential source for idea.

After that I didn't have time to go back and review it. Therefore, once I did get time, I fixed the code and applied as appropriately. Initially, I didn't even have the HTTP Authentication in my code and I was relying on the random Feed URL to add security to the system.

I would very much like to contribute this module to the Drupal.

So, if you are okay I would love to fix the module after your suggestions and give it another shot.

--
Abhi

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Closed (won't fix)

The application has been declined; changing status of this report does't change the status of the application.

I cannot find any other application made from you; therefore, I cannot find the code you previously proposed for review, nor what it was suggested you to do.
Copying code from another module (especially in this case) doesn't show originality, nor ingenuity.

abhigupta’s picture

abhigupta commented

Hi Alberto,

The module idea is my own and original. Additionally, majority of the code is my own and originial as well.

Yes, I have copied the part about htaccess control from the htaccess module (http://drupal.org/project/httpauth) and I admitted the first time you said anything about it. I am not trying to hide it or anything.

I have no intention of gaming or cheating here, it somehow seems I am crossing as such to you. The only reason I changed the status was to make sure you noticed my reply and didn't miss it.

I just looked at the httpauth module again and it seems now httpauth can be used by my module without including any of the code from that module. I can easily take out the http authentication code and make httpauth as one of the dependency for my module. This will also make my module smaller.

I would like to try to submit my module after making the modifications. So which further course of action would you suggest for me? Should I forget about the idea of making this module available on drupal.org? Should I continue with this application after making the modification? Should I start a new application after modifications?

-----
If it helps you find it ... my earlier attempt was in December 2008. I received the reply from one of the contribution named AjK on December 15th, 2008.

Here are the contents of email I recieved from AjK:

Message from the CVS maintainer:
There's a number issues that you need to look at.

1. hook_menu() doesn't use $may_cache (the first path is static and therefore should be inside the $may_cache, the rest should be in the else {...})

2. $base_url is used in several functions but not all those functions declare it as global.

3. The watchdog_rss_random_gen() function allows for the key to include characters that should not be used in a URL.

4. And then there's the whole "security by obscurity" [1] thing you have going here. Using a secret key as a defense against anyone looking at the feed. This would be better served by implementing something more like hash digest [2] which is supported by most RSS readers.

[1] http://en.wikipedia.org/wiki/Security_through_obscurity
[2] http://en.wikipedia.org/wiki/Basic_access_authentication

Please ensure you read http://drupal.org/cvs-application/requirements and if necessary please re-apply.

If on reading the above information you wish to add further details please ensure you re-apply for a CVS account again. Please do not reply to this email with additional supporting information as we can not accepting supporting material by email.

Kind regards,
AjK.

P.S. Thanks for taking the time to reply and code review my module! :)

abhigupta’s picture

abhigupta commented

I looked at my module again and I think it could definitely use trimming and fixes especially related to http authentication. I would like to fix it and again submit it for consideration, if you think I still have any chance :)