Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Add a member_list permission to allow or deny viewing of the full member list #83

Closed
wants to merge 2 commits into from

3 participants

@rohieb

This can be useful in situations where users should be able to see their own payment status, but for data security reasons they should not be able to see who else is a member. With the existing permissions, this case is not possible to model. With the new member_list permission, users who have this permission have acccess to all member information and the full member list. If member_list is not set, the users can only see their own information (and only their own entry in the member list).

Both members and directors are given this permission in the default configuration, so no behaviour is changed from the existing configuration.

rohieb added some commits
@rohieb rohieb Add new permission member_list to allow/disallow viewing of all members
If member_list is set, users in the corresponding role can see all members in
the member list. If member_list is not set, the user can only see its own entry
in the list.

This can be useful in situations where users should be able to see their own
payment status, but for data security reasons they should not be able to see who
else is a member. With the existing permissions, this case was not possible.
737888c
@rohieb rohieb Disallow user querying by URL if member_list is not given
Otherwise, if a user with cid=2 does not have member_list permission, it would
still be possible to to something like .../index.php?q=member&cid=4
abca208
@elplatt
Owner

Hi there! This seems like a useful improvement, but a few things need to be changed before I can merge.

  • Needs to be issued for dev branch (I'd recommend just forking dev rather than master)
  • There's no need to add a new permission, you can check member_view and add code to let members view their own data.

Otherwise it looks good!

@elplatt
Owner

Oh yes! Please add yourself to the CONTRIBUTORS file too!

@rohieb

Right, expanding the use case of member_view seems reasonable. BTW, is there any textual description of what all the permissions do?

I mistakenly pull-requested against the master branch because I did not remember to change the default setting GitHub makes when doing a pull request. But you should also be able to merge it manually into the dev branch.

@rohieb

On a deeper view, it seems the code needs heavy modification if we re-interpret the member_view permission this way. Currently, the Member tab is not even shown when member_view is not set, and I guess there are some more places in the code which rely on the "old" interpration of that permission (I have not copmletely checked this though). For me, it seems simpler to just add the new member_list permission.

@elplatt
Owner

Showing the member tab just so a user can see their own information would be strange. Instead, how about making the user's name in the header link directly to their member page?

I'll make a note to add documentation for the permissions in the wiki. Also, I should mention that we're overhauling the member module right now, and it things should make a little more sense after that. The changes should be finished by Monday.

@rohieb
@chris18890

the end effect of this patch can also be achieved by modifying line 88 of seltzer\crm\modules\member\table.inc.php, will look at this later today

@elplatt
Owner

If anyone is still working on this, keep these points in mind:

  • We can use existing permissions (member_view, contact_view, etc)
  • We can now get to a user page by clicking the header link.
  • Users should always be allowed to see their own data. The "view" permissions are to restrict access to seeing the data of other users.
@elplatt
Owner

Closing this because:

  • Pull requested against master not dev.
  • Was written for the old API.
  • member_list duplicates the intended functionality of member_view.
@elplatt elplatt closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 31, 2013
  1. @rohieb

    Add new permission member_list to allow/disallow viewing of all members

    rohieb authored
    If member_list is set, users in the corresponding role can see all members in
    the member list. If member_list is not set, the user can only see its own entry
    in the list.
    
    This can be useful in situations where users should be able to see their own
    payment status, but for data security reasons they should not be able to see who
    else is a member. With the existing permissions, this case was not possible.
  2. @rohieb

    Disallow user querying by URL if member_list is not given

    rohieb authored
    Otherwise, if a user with cid=2 does not have member_list permission, it would
    still be possible to to something like .../index.php?q=member&cid=4
This page is out of date. Refresh to see the latest.
View
4 crm/include/member/install.inc.php
@@ -78,8 +78,8 @@ function member_install($old_revision = 0) {
, '8' => 'webAdmin'
);
$default_perms = array(
- 'member' => array('member_view', 'member_membership_view')
- , 'director' => array('member_plan_edit', 'member_view', 'member_add', 'member_edit', 'member_delete', 'member_membership_view', 'member_membership_edit')
+ 'member' => array('member_list', 'member_view', 'member_membership_view')
+ , 'director' => array('member_list', 'member_plan_edit', 'member_view', 'member_add', 'member_edit', 'member_delete', 'member_membership_view', 'member_membership_edit')
);
foreach ($roles as $rid => $role) {
if (array_key_exists($role, $default_perms)) {
View
1  crm/include/member/member.inc.php
@@ -45,6 +45,7 @@ function member_permissions () {
, 'contact_edit'
, 'contact_delete'
, 'member_plan_edit'
+ , 'member_list'
, 'member_view'
, 'member_add'
, 'member_edit'
View
7 crm/include/member/page.inc.php
@@ -119,6 +119,13 @@ function member_page (&$page_data, $page_name, $options) {
return;
}
+ // Disallow access if neccessary
+ // TODO: maybe add a nice error message
+ if (!user_access('member_list') && $_GET['cid'] != user_id()) {
+ page_set_title($page_data, "Error");
+ return;
+ }
+
// Set page title
page_set_title($page_data, theme('member_contact_name', $cid));
View
6 crm/include/member/table.inc.php
@@ -82,6 +82,12 @@ function member_table ($opts = NULL) {
// Loop through member data
foreach ($members as $member) {
+
+ // If the user does not have member_list access, disallow listing
+ // for all members other than its own user
+ if (!user_access('member_list') && $member['cid'] != user_id()) {
+ continue;
+ }
// Add user data
$row = array();
Something went wrong with that request. Please try again.