cron_sec, when enabled as the lightest module, implements key security similar to Drupal 7's enhanced key security for D6, and ptionally allows cron to run at an elevated permission level thought the use of switching the active user for the session to one specified in its configuration, and then switching the user back as the cron session ends to the anonymous user.

Motivation for writing this module resulted from a combination of using the revisioning module and link-checker. When link-checker's hook cron ran, it did a nice job of identifying moved links, and per the appropriate setting, tried to update the containing content with the new URL. But, the anonymous user didn't have (for obvious reasons) permissions through the revisioning module to publish revisions. So, instead, I'd have all these unpublished corrected pieces of content that I'd have to manually go through and publish. That was causing me unnecessary work.

So, by creating an authenticated user that could be active when cron ran, and giving that user the correct permissions, the problem went away.

The Sandbox Project page is at https://drupal.org/sandbox/sdsheridan/2067471 .

Git Repository Link: http://drupalcode.org/sandbox/sdsheridan/2067471.git/commit/cdf493f72fff...

Comments

sdsheridan’s picture

Status:Active» Needs review
PA robot’s picture

Status:Needs review» Needs work

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxsdsheridan2067471git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

sdsheridan’s picture

Status:Needs work» Needs review
geberele’s picture

Hi sdsheridan!
I ran a ventral review on your project and it requires some changes, mainly in the coding standards. Please fix them up:
http://pareview.sh/pareview/httpgitdrupalorgsandboxsdsheridan2067471git

FILE: /var/www/drupal-7-pareview/pareview_temp/cron_sec.install
--------------------------------------------------------------------------------
FOUND 4 ERROR(S) AND 1 WARNING(S) AFFECTING 3 LINE(S)
--------------------------------------------------------------------------------
18 | ERROR | There should be no white space after an opening "("
18 | ERROR | There should be no white space before a closing ")"
51 | WARNING | Last item of an inline array must not be followed by a comma
62 | ERROR | There should be no white space after an opening "("
62 | ERROR | There should be no white space before a closing ")"
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/cron_sec.module
--------------------------------------------------------------------------------
FOUND 30 ERROR(S) AND 8 WARNING(S) AFFECTING 23 LINE(S)
--------------------------------------------------------------------------------
17 | ERROR | There should be no white space after an opening "("
17 | ERROR | There should be no white space before a closing ")"
18 | ERROR | There must be no space before the colon in a CASE statement
64 | WARNING | Last item of an inline array must not be followed by a comma
78 | ERROR | There should be no white space after an opening "("
79 | ERROR | There should be no white space after an opening "("
79 | ERROR | There should be no white space before a closing ")"
79 | ERROR | There should be no white space before a closing ")"
82 | ERROR | Array indentation error, expected 14 spaces but found 16
82 | ERROR | There should be no white space after an opening "("
83 | ERROR | Expected 1 space before "?"; 28 found
84 | ERROR | Expected 1 space before ":"; 28 found
84 | ERROR | There should be no white space before a closing ")"
85 | ERROR | Array indentation error, expected 14 spaces but found 16
87 | ERROR | There should be no white space after an opening "("
87 | ERROR | There should be no white space before a closing ")"
96 | ERROR | There should be no white space after an opening "("
96 | ERROR | There should be no white space after an opening "("
96 | ERROR | There should be no white space before a closing ")"
96 | ERROR | There should be no white space before a closing ")"
99 | WARNING | Last item of an inline array must not be followed by a comma
99 | ERROR | Expected 1 space between comma and ")"; 0 found
100 | WARNING | Last item of an inline array must not be followed by a comma
100 | ERROR | Expected 1 space between comma and ")"; 0 found
106 | WARNING | Line exceeds 80 characters; contains 102 characters
123 | ERROR | There should be no white space after an opening "("
123 | ERROR | There should be no white space before a closing ")"
126 | WARNING | Avoid backslash escaping in translatable strings when
| | possible, use "" quotes instead
129 | WARNING | Last item of an inline array must not be followed by a comma
131 | ERROR | There should be no white space after an opening "("
131 | ERROR | There should be no white space before a closing ")"
135 | WARNING | Last item of an inline array must not be followed by a comma
139 | WARNING | Line exceeds 80 characters; contains 100 characters
146 | ERROR | The first index in a multi-value array must be on a new line
146 | ERROR | There should be no white space after an opening "("
147 | ERROR | Expected 1 space before "?"; 14 found
148 | ERROR | Expected 1 space before ":"; 14 found
148 | ERROR | There should be no white space before a closing ")"
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/cron_sec.settings.inc
--------------------------------------------------------------------------------
FOUND 50 ERROR(S) AND 8 WARNING(S) AFFECTING 35 LINE(S)
--------------------------------------------------------------------------------
8 | ERROR | Function comment short description must start with a capital
| | letter
18 | ERROR | Array indentation error, expected 4 spaces but found 6
19 | ERROR | Array indentation error, expected 4 spaces but found 6
20 | ERROR | Array indentation error, expected 4 spaces but found 6
21 | ERROR | Array indentation error, expected 4 spaces but found 6
22 | ERROR | Array indentation error, expected 4 spaces but found 6
32 | ERROR | Array indentation error, expected 4 spaces but found 6
33 | ERROR | Array indentation error, expected 4 spaces but found 6
34 | ERROR | Array indentation error, expected 4 spaces but found 6
35 | ERROR | Array indentation error, expected 4 spaces but found 6
38 | ERROR | Array indentation error, expected 4 spaces but found 6
38 | WARNING | Last item of an inline array must not be followed by a comma
42 | ERROR | Array indentation error, expected 4 spaces but found 6
43 | ERROR | Array indentation error, expected 4 spaces but found 6
43 | ERROR | There should be no white space after an opening "("
43 | ERROR | There should be no white space before a closing ")"
44 | ERROR | Array indentation error, expected 4 spaces but found 6
44 | ERROR | There should be no white space after an opening "("
45 | ERROR | Expected 1 space before "?"; 14 found
46 | ERROR | Expected 1 space before ":"; 14 found
46 | ERROR | There should be no white space before a closing ")"
47 | ERROR | Array indentation error, expected 4 spaces but found 10
52 | ERROR | There should be no white space after an opening "("
52 | ERROR | There should be no white space before a closing ")"
54 | ERROR | Array indentation error, expected 6 spaces but found 8
55 | ERROR | Array indentation error, expected 6 spaces but found 8
55 | WARNING | Avoid backslash escaping in translatable strings when
| | possible, use "" quotes instead
56 | ERROR | Array indentation error, expected 6 spaces but found 8
57 | ERROR | Array indentation error, expected 6 spaces but found 8
57 | WARNING | Avoid backslash escaping in translatable strings when
| | possible, use "" quotes instead
63 | WARNING | Last item of an inline array must not be followed by a comma
67 | WARNING | Last item of an inline array must not be followed by a comma
74 | WARNING | Line exceeds 80 characters; contains 99 characters
82 | ERROR | There should be no white space after an opening "("
82 | ERROR | There should be no white space before a closing ")"
83 | ERROR | There should be no white space after an opening "("
83 | ERROR | There should be no white space before a closing ")"
86 | ERROR | There should be no white space after an opening "("
86 | ERROR | There should be no white space before a closing ")"
93 | WARNING | Line exceeds 80 characters; contains 81 characters
93 | ERROR | Function comment short description must be on a single line
103 | ERROR | There should be no white space after an opening "("
103 | ERROR | There should be no white space before a closing ")"
107 | ERROR | There should be no white space after an opening "("
108 | ERROR | There should be no white space before a closing ")"
119 | ERROR | There should be no white space after an opening "("
119 | ERROR | There should be no white space before a closing ")"
119 | ERROR | There should be no white space after an opening "{"
119 | WARNING | Line exceeds 80 characters; contains 118 characters
119 | ERROR | Comments may not appear after statements.
120 | ERROR | There should be no white space after an opening "("
120 | ERROR | There should be no white space before a closing ")"
141 | ERROR | Space found before first semicolon of FOR loop
141 | ERROR | Space found before second semicolon of FOR loop
141 | ERROR | There should be no white space after an opening "("
141 | ERROR | Space found before semicolon; expected "0;" but found "0 ;"
141 | ERROR | Space found before semicolon; expected "$length;" but found
| | "$length ;"
141 | ERROR | There should be no white space before a closing ")"
--------------------------------------------------------------------------------

sdsheridan’s picture

OK, addressed most of them. I left some things as is as they're easier (in my mind) to read that way, and it looks like pareview doesn't always realise what's going on (such as splitting a "x ? y : z" construct into multiple lines for readability).

Have to say I prefer 'defensive' coding practices like leaving a trailing comma in arrays, but maybe that's just me.

Shawn

geberele’s picture

Hi sdsheridan,
if you want your module to be accepted from the community you need to follow the Drupal Coding standards, for this reason is really important don't get any Error/Warning message using Code Sniffer.
Please fix all the others:
http://pareview.sh/pareview/httpgitdrupalorgsandboxsdsheridan2067471git

sdsheridan’s picture

OK, they're all gone.

geberele’s picture

Ok, you have to get a review bonus now to speed up your own review process.

abghosh82’s picture

Status:Needs work» Needs review

I have checked the module code and did manual review. Following are some of code review suggestions:

cron_sec.settings.inc - Line no. 86 instead of using
preg_match('/[^0-9A-Za-z]/', $element['#value'])
you can consider using the ignore case modifier. Something like below:
preg_match('/[^0-9a-z]/i', $element['#value'])

cron_sec.settings.inc - The function '_cron_sec_random_key' is too heavy to generate a random key, you can consider using some light weight utility like md5(time()) and extract a sub string of required length in your case minimum length is 15 and md5() gives up to 32. You may not need to create a seperate function.

cron_sec.install - In hook_uninstall deleting the variables by picking name form a SQL query:

$result = db_query("SELECT name FROM {variable} WHERE name LIKE 'cron_sec_%%'");

The above looks bit unsafe specially with like in it, it can delete some unwanted variable that starts with 'cron_sec' this can be very rare but there is a possibility. Rather you can consider doing it separately for each variable like below.

variable_del('cron_sec_user');
variable_del('cron_sec_key');

There are not many variable you are using in the module so doing it the way mentioned above may not be a long list. :)

abghosh82’s picture

Status:Needs review» Needs work

With the above comments setting it to needs work.

sdsheridan’s picture

Thanks for the review, abghosh82! I made the suggested changes and pushed to git, except for _cron_sec_random_key. Reason there is that md5 creates only a hex hash, which means there will only be the letters A through F, and only in one case. I wanted the key generation to use the entire alphabet and generate both upper and lower case letters. If you have another suggestion that will achieve that result, let me know.

Shawn

kscheirer’s picture

Title:[D6] cron_sec» [D6] CRON Sec
Status:Needs review» Reviewed & tested by the community
  • I think you could change the name to Cron Security instead of just Sec.
  • Don't escape quotes inside of t(), just use double-quotes on the outside. That makes it easier to translators.
  • In cron_sec_cron(), can you simplify the condition to (!user_access('cron_sec: run CRON without key') || (variable_get('cron_sec_key', '')) != $_GET['key']?
  • "ruturn" should be "return".

Those seem like minor issues. I guess the $key is not well-guarded since it's in the GET string, but I don't think that's a problem.

----
Top Shelf Modules - Crafted, Curated, Contributed.

sdsheridan’s picture

Thanks, kscheirer! I implemented points 2, 3, and 4 and pushed back up to git. I left #1 simply because I didn't want really long function names.

Shawn

kscheirer’s picture

Status:Reviewed & tested by the community» Fixed

It's been a month without any problems reported, so I'm promoting this myself as per https://drupal.org/node/1125818.

Thanks for your contribution, sdsheridan!

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:

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 - Crafted, Curated, Contributed.

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.