Dummy Permission

Hi,

As per the ventral review, the readme.txt is missing. read this link to see how to add one: http://drupal.org/node/447604

Found 2 errors:

FILE: ...sites/all/modules/pareview_temp/test_candidate/dummy_permissions.module
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
149 | ERROR | Files must end in a single new line character

Line 138: The $message argument to drupal_set_message() should be enclosed within t() so that it is translatable.

drupal_set_message("Saved");

All the best with your project!
Jaya

Hi again,

I also tested the project on a drupal 7 test environment and found that I am unable to add users to the "trusted users". How do we add that?

Thanks
jaya

Hi there,

Just a couple of things.

Lines 107,116,122,126 Should have the title wrapped in a t() function.

Line 117, the description should use the t() function.

Missing return type from docblocks in dummy_permissions_list_form, dummy_permissions_add_form

Add a link to the permissions list form from the modules .info file
e.g. configure = admin/people/permissions/dummy_permissions

You should be able to remove the variable 'dummy_permissions' during a hook_uninstall.

Hi Gaelan,

Whn we try to add a new dummy permission, there is a checkbox for trusted users only. I assume by that you mean authenticated users. I am wondering whether that functionality can be changed to selection of some users only...so that I can have my view visible not to all users of a particular role but only to some particular users. Makes sense?

Thanks!
Jaya

Sorry if i am seeming dumb! :) When we say restrict access, how do i restrict it to say user 3 alone? Suppose I create 2 views, one for user 3 and one for user 4. And i want to show view 3 only to user 3 and view 4 only to user 4. If i could select which trusted users I have in mind in the "add new dummy permission" it would be a very useful functionality. In fact for sites with hundreds of users of different roles, it would even be nice if we could have a role filtering first and then the user filtering.

I am just testing this out for review purposes and dont actually have a need for this module currently, but do see great potential, hence my comments. Please do let me know if you had some other reason for the conception of the module.

Thanks

Hi,

I created a new dummy permission. I was able to use it in the views permissions and configure it in the permissions section of the site also. But was wondering if adding an extra field in the configurations section of which users this permission is applicable to, may make this module really very powerful.

Thank you
Jaya

This sounds like a feature that should live in the existing config_perms module. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in the config_perm issue queue to discuss what you need. You should also get in contact with the maintainer(s) to determine what is needed that this works for you.

If that fails for whatever reason please get back to us and set this back to "needs review".

klausi is referring to Custom Permissions, which was originally called Site Configuration Permissions, hence the module file and function names (conf_perms).

#1664386: Pathless custom permissions. seems to disagree.
In fact, it seems like Custom Permissions can do everything Dummy Permission can do, no?

We should have cought this earlier.

I found a few issues:

1. There's still a master branch in git. You have to delete it; see http://drupal.org/node/1659588
2. The text in your readme file needs to wrap at 80 characters.
3. Your readme and module files need to end with a blank line

For more information: http://ventral.org/pareview/httpgitdrupalorgsandboxgaelan1798428git . I recommend you run your code through PAReview before you request another review.

This is a really handy looking module and I think you're pretty close to getting it approved.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

If you reopen this please keep in mind that we are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll 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" (found some problems with the project) or "reviewed & tested by the community" (found no major flaws).

Hi,

Manual review:
1) You should delete the variables dummy_permissions at the uninstall of the module. http://api.drupal.org/api/drupal/modules!system!system.api.php/function/...

That's all, this modules works great, thanks.

Manual review:
1) in your add dummy permissions form, you need to add a validate on the field machine name.
2) in drupal core you have a functionnality: when a new drupal permission is created by a module, these permissions is assigned to the user_admin_role, that can be set via the variable :
variable_set('user_admin_role', $admin_role->rid);
i think you need to emulate this functionnality, when a new permission is created via your user interface. For the moment that doesn't works. (only for your module permission 'Administer Dummy Permissions', because the permission is added to the admin_role on the module installation).

That's all

l.176 .module

$admin_role = variable_get("user_admin_role");
user_role_grant_permissions($admin_role, array($machine_name));

You need to add a default value to the variable_get(),
and test this value before calling user_role_grant_permissions().
Some drupal installations may not have an user_admin_role in that case your call to user_role_grant_permission will failed.

Otherwise that's looks ok.

You should remove the .gitignore file from the repo.

l.176 .module
your test is wrong $admin_role is always set.

  $admin_role = variable_get("user_admin_role");
  // here admin_role == null or a role id

double post

manual review:

1. no edit form for existing dummy permissions?
2. dummy_permissions_list_form(): this is vulnerable to XSS exploits if I enter a permission description like <script>alert('XSS');</script>. You need to sanitize all user provided text before printing, or (as in this case) mark the permission that is able to inject malicious stuff as 'restrict access' => TRUE. I guess Drupal does not expect to have malicious stuff in permission descriptions, so I think the only proper way to solve this is to restrict this permission. This is a security blocker.

Poor Gaelan, you were so close! As soon as the security issue is fixed I promise to approve you.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Forgot to add security tag. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

That fix now protects your own page from exposing an XSS risk, but as I said: the permission description is also displayed on admin/people/permissions for example and it is not filtered there. So we still have an XSS threat here as Drupal does not expect user provided stuff in permissions and their descriptions. The only sane way out here is to mark 'administer dummy permissions' as 'restrict access' => TRUE. Defining new permission is a pretty heavy admin task anyway (like administering Views for example), so that should not be of any harm?

As I can still exploit the security issue this has to go back to needs work.

Yes: "When handling data, the golden rule is to store exactly what the user typed. When a user edits a post they created earlier, the form should contain the same things as it did when they first submitted it. This means that conversions are performed when content is output, not when saved to the database." from https://drupal.org/node/28984

What's the problem with 'administer dummy permissions' as 'restrict access' => TRUE?

check_plain() in hook_permission() is bad, since nothing is printed there to the user. And the escaped text would be saved to the database which violates the golden rule of handling data.

So 'restrict access' on its own is enough.

Thanks for your contribution, Gaelan!

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.