From private email from killes to me:

Hi Derek,

today, we've had to spiders wreak havoc on d.o. Among other pages they visited the statistics pages too often to be funny.

I am thinking that it doesn't make sense to offer this information to logged out users. We can request people to log in got this, especially since it is expensive to compute.

Can you point me to where the code for this is so I can craft a patch?

Cheers,
Gerhard

I replied:

contributions/modules/project_issue/includes/statistics.inc

And he replied:

Thanks!

While I can change the access permission for this through hook_menu_alter, the links to the pages can only be changed in the module or the theme as a last resort.

Would you be ok to change this in the project issue module itself?

If yes: which permission should I require for statistics access? I was thinking of simpy using user_is_logged_in.

Cheers,
Gerhard

Seems silly to continue this conversation in private, so I'm creating this issue to further discuss.

If it's going to be handled natively inside project_issue and not a d.o hack, I'd rather not assume that anonymous users can't see these pages. It should be trivial to just add a new "view issue statistics" in project_issue_perm() and use that for the menu access on the pages and in the spots in the UI where the links are being generated. Then sites that don't care can give this perm to anon + auth, whereas d.o can just give it to auth.

Should be a very easy + small patch, although if you really wanted to do this right, your patch would include simpletests to ensure the permission is doing what it's supposed to. :) Then a) I could commit + deploy more easily and b) we'd make sure not to break this in the future.

Thanks,
-Derek

CommentFileSizeAuthor
#10 permission-issue-stats-988188-10.patch9.54 KBmr.baileys
#9 988188-missing-hunk.patch1.07 KBdamien tournoud
#2 project-issue-statistics-permission.patch7.95 KBmr.baileys

Comments

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
Assigned: Unassigned » mr.baileys

Talked to killes on IRC and will take a stab at this later, so assigning to myself...

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
Status: Active » Needs review
StatusFileSize
new project-issue-statistics-permission.patch7.95 KB

Here's a patch that introduces a new "access issue statistics" permission + accompanying tests.

  • I've named the new permission "access issue statistics" instead of "view issue statistics" as that seems to be more in line with the permissions already present ('access project issues', 'access projects', ...)
  • Unlike most of the permissions in project and project_issue, the "access issue statistics" is all-or-nothing (no distinction between between "own" and "all" -- you can either view all the statistics pages, or none at all)
  • The patch slightly alters the project-issue-issue-cockpit template since $issue_statistics will only be set if users have "access issue statistics" permission. This might break existing sites/themes though (trying to display an undefined variable), so we might want to always set $issue_statistics (and leave it blank if no permission) and then leave the template as is.
  • The issue cockpit block was cached on a project-basis. I had to add the "access project issues" permission flag to the cid and split it out into one cached version for authorized users, and one for non-authorized users.
gerhard killesreiter’s picture

gerhard killesreiter commented

That looks good to me, but I hope dww can review it too.

miaoulafrite’s picture

miaoulafrite commented

thanks for this information.
in my case issue tracking is related to projects that are themselves related to clients

it would be nice to restrict access to project pages and the issues related to these projects so that customers cannot see what the other projects you run AND the issues you have on these projects.

what do you think?

dww’s picture

dww
we/he/they
commented

@miaoulafrite: I think you're confusing this issue with a node access module that knows about projects. Luckily, such a project exists, and almost works:

http://drupal.org/project/project_access

@mr.baileys: Thanks for the patch! Sadly, I've been totally slammed deploying #11416: Please provide *.zip downloads. and working on the Git migration. We've got a major Git-related deadline this week, and I've got to get a ton of stuff done by then. So, hopefully next week I'll have more time to deal with issues like this. I've added a personal TODO item to resurface in my universe next week to look at this.

Thanks,
-Derek

miaoulafrite’s picture

miaoulafrite commented

@dww: i was thinking of nodeaccess yes. i don't know this module very well but i believe it does not fit to project (which is probably the reason why project_access came up.

since project_access is not yet ready for distribution, what would you suggest to control access among projects?

dww’s picture

dww
we/he/they
commented

I'd suggest you open a separate support request instead of continuing to discuss your questions in an unrelated issue. ;)

damien tournoud’s picture

damien tournoud commented

We were seeing a lot of those post Git launch. I deployed this manually to bzr.

damien tournoud’s picture

damien tournoud commented
StatusFileSize
new 988188-missing-hunk.patch1.07 KB

This hunk (project-specific issue lists) was missing from the patch.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
StatusFileSize
new permission-issue-stats-988188-10.patch9.54 KB

So this is already running on drupal.org? Cool :)

Re-rolled because patch no longer applied cleanly, also combining the patch from #2 with Damien's patch from #9.

dww’s picture

dww
we/he/they
commented
Status: Needs review » Fixed

Great, thanks! Reviewed, ran tests (all passing), tested myself, and committed/pushed:

http://drupal.org/commitlog/commit/1894/d9655bc547754e9de81e9bae99639b95...

Also pushed a follow-up to fix some code-style issues and re-worded the assertion text to make it much less verbose:

http://drupal.org/commitlog/commit/1894/198d2f31a7ee276969374f2c286128f0...

Thanks, everyone!

dww’s picture

dww
we/he/they
commented

Note: this is now deployed (again) for real from a clean vendor import.

dww’s picture

dww
we/he/they
commented

Note: when this was deployed, no one gave the "view issue statistics" permission to any roles. I was getting complaints, so I just checked with nnewton and he was okay with me granting this back to the "git user" role for all project maintainers to at least be able to access this. The main concern was anon + bots crawling these pages, IIRC.

In the future, this page needs to be totally re-done based on metrics from Sampler API, but that's another story entirely. ;)

Just documenting the change in permissions for posterity...

dww’s picture

dww
we/he/they
commented

Whoops, this broke the cache invalidation for the issue summary block on project pages:

#1077880-4: Block cache of issue counts on project pages is not invalidated when it should be

Status: Fixed » Closed (fixed)

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