I had a security issue:
When one user toggle Status to limit access to his page as Private, the page is no more listed in the All mysite block... but the URL is still accessible :-(
As one can guess the URL...

Here is my solution:
1) Add 'view public mysite' access perm (line 356)

/**
 * Implementation of hook_perm().
 * @ingroup drupal
 */
function mysite_perm() {
  $array = array('administer mysite', 'edit mysite', 'view public mysites', 'view all mysites');

2) Modify access conditions (line 371)

/**
 * Implementation of hook_user().
 * @ingroup drupal
 */
function mysite_user($type, &$edit, &$user) {
  if ($type == 'view' && user_access('edit mysite', $user)) {
    $mysite = mysite_get($user->uid);
    if ( ($mysite->uid > 0 && $mysite->status > 0 && user_access('view public mysites')) || ($mysite->uid > 0 && user_access('view all mysites')) || user_access('administer mysite') ) {

Finally this determine a real access control based on Status - in the same time, we can still define a role that can access all the mysite pages but modify none (this role is weird to me but...)

Comments

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Version: 5.x-2.0-beta5 » 5.x-2.0

Well, this is privacy not security, but its still a bug, I think.

If a user with 'view all mysites' permission hits a 'private' page, no content should be rendered -- in fact, the visitor should be sent to their own MySite page instead.

You would have to change the following access condition as well (mysite_page, lines 627-630:

  // access checks: can this user view this data?
  else if ($mysite->status == 1 || user_access('administer mysite') || user_access('view all mysites')) {
    $show = TRUE;
  }

You should be able to correct this behavior by changing the IF condition:

  // access checks: can this user view this data?
  else if ($mysite->status == 1 || user_access('administer mysite')) {
    $show = TRUE;
  }

This will send people to an Access Denied message when trying to view a private page.

And the beta is already released....

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

The logic for that last correction is wrong, it should be:

  // access checks: can this user view this data?
  else if (user_access('administer mysite') || ($mysite->status == 1 && user_access('view all mysites'))) {
    $show = TRUE;
  }

The logic in mysite_user() should be:

    if ($mysite->uid > 0 && (user_access('administer mysite') || $user->uid == $mysite->uid || ($mysite->status == 1 && user_access('view all mysites'))) {
...
agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Status: Needs review » Fixed

I managed to fix this without adding a new permission. The 'view all mysites' permission should be sufficient.

Committed to HEAD.

Thanks.

sashainparis’s picture

sashainparis
Primary language French
Location Provins
commented

Merci.
Less is better :-)

One ')' was missing at the end for mysite_user():

    if ($mysite->uid > 0 && (user_access('administer mysite') || $user->uid == $mysite->uid || ($mysite->status == 1 && user_access('view all mysites'))) ) {
...

Nota: "access denied" appears but there is no redirection to user's own page. Not a matter for me but you might like the feedback.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

Yes, I saw the missing ) and committed the correct code.

The 'Access denied' is how the function returns currently, it would take some thought to change that. But since people have to hack the URL to get to that page anyway -- since all links to it should be hidden -- I think Access Denied is probably ok.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

I can correct this behavior my editing the end of the mysite_page() function, lines 700+ become:

  else if ($user->uid > 0 && user_access('edit mysite')) {
    drupal_goto('mysite/'. $user->uid .'/view');
  }
  else {
    drupal_access_denied();
    return;
  }
}

I will commit this to HEAD tonight and it will be part of the bugfix release (5.x.2.1) this weekend.

The HEAD tarball is stable enough to use until 5.x.2.1 comes out.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Status: Fixed » Closed (fixed)

Committed and released as 5.x.2.1

Thanks!