Unaccessible project titles and uris visible

@Josh Benner: Can you give me a little more information about how the user can see the title of a node they are not supposed to be able to see? An example URL of where this occurs would be nice. Before I test your patch I need to be able to recreate the problem myself and I can't figure out where it is that you're seeing problems. Also, what permissions (project and project issues related) does the user you are testing this with have?

On a related note, something similar is true with regards to display of the project name in project issue metadata tables (and probably in email metadata). If, for example, a user has 'access own projects' but not 'access projects', he will be able to view issues that are currently assigned to one of his own projects. However, if that issue had been assigned to other projects previously, the titles of those projects will show up in the issue metadata tables. This is a fairly minor problem, but something we should fix.

The question is how do we fix it? The access checking part is pretty easy, but how do we display the information to the user? I don't think it would make a lot of sense to not display anything, since it would be difficult to tell if the issue was somehow not assigned to a project all or if it was assigned to a project the user didn't have permission to view. Maybe we create a theme_project_title_redacted() function that just displays '*******' instead of 'Secret project' or something like that. Any suggestions?

I'm setting this to cnw since I think this should be included in the patch as well. However, Josh, I'm still not sure how to replicate your original problem.

Ah.......those evil node access control modules show their ugly faces again :) I didn't realize that you also were using one of those. Which one are you using, just for the record? This probably doesn't matter but it's good to know.

I'll try to take a look at this again with that knowledge in mind. Thanks for providing additional information. Happy moving :)

@Josh Benner: I'm able to replicate what you describe in comment #4 above. Thanks for clarifying.

This is even more of a pain than I thought it would be. Not only are the titles of project displayed in metadata tables, they are also displayed in the breadcrumbs of issues assigned to projects, and the link to the project in the breadcrumb contains the short name (ui) of the project.

Here are patches for project and project_issue that take care of most the the things brought up above.

The caveats are:
1. Since the path module takes care of aliasing node/123 to /project/secret, I wasn't able to think of a good way to force the links to hidden projects in breadcrumbs, etc. to make the links in '/node/123' form instead of '/project/secret'. We could completely disable project path aliasing, but I don't think that's a good solution.

2. There is a problem completely separate from this patch/issue, #234664: Users cannot submit comments if issue is assgined to a project they can't view without changing project. I only mention that here because it relates to this issue in the sense that it is another complication of restricting a users view access to individual projects.

I really feel like a CIA agent now after having worked on this patch (see screenshot).

hrm. i don't think we should have node_access checks in a theming function. seems like the theme function really should only handle the theming of what a 'hidden' project name looks like, and we need a separate internal function for checking the titles permissions.

is there something i'm missing here?

+1 to hunmonk's arguments in #10. Otherwise, thanks to everyone for working on this.

Ok, lets give this one a shot.

Patches for both project_issue and project are attached.

couple of things:

1. if the title is hidden, should the breadcrumb portion of that really still be a link? i know it might be a PITA to fix that, but it seems odd to link to '********', especially when the user won't have perms to view anyways.
2. since $project can be NULL, do we need an isset($project) here? if (!empty($project->title)) {

Attached patch makes change suggested in #13 so that breadcrumbs are not links if the user would not have access to the project node in the first place.

To answer #2 above, no, we don't need to call isset(), because according to http://us.php.net/manual/en/function.empty.php, "empty() is the opposite of (boolean) var, except that no warning is generated when the variable is not set."

Also note that this patch now *only* touches the project issue module. I changed this because I realized that if we patch both the project and project issue module, users who upgrade the project issue module without upgrading the project module could get errors. In any case, what's changed here really belongs in project issue anyway.

This patch worked well in my testing.

One caveat--if you install simple access, and set a project node so that certain users do not have access to that project, it seems that those users will get lots of errors if they try to create a new issue attached to *any* project. This happens without the attached patch, so I don't believe this problem is at all related to this issue. I'm investigating what's going on and will post a new issue when I figure out where the problem is. But I just wanted to warn anyone that if they run into this problem it's not the fault of this patch.

I'm having second thoughts about whether this fix should be implemented or not. If this patch is committed it'll be impossible for a user to follow up on an issue he would otherwise be allowed to follow up on (that is, in the case where the project the issue is assigned to is not viewable by the user). As I understand it, we're currently not controlling access to individual issues based on whether or not a user has access to the project the issue has been assigned to, and without doing this we're going to run into a lot of problems, I think. But changing to do so would be a fairly significant shift in functionality, and probably something that's best thought about more carefully and that is not part of a bug fix release.

i would agree. i don't want this to hold up the 5.x-2.2 release -- setting an appropriate status to encourage discussion.

In case anyone needs to use this patch, use the version attached here. Previous versions did not always sanitize input properly before display.

"needs more info" implies we're waiting for the original poster to tell us what they're talking about, it's an incomplete request, we need to know more about the problem before we can move forward, etc. we're not actually waiting for more data, we just need to figure out if/how we want to tackle this issue. if we're back to the drawing board on how to solve this, i'd call this just "active"...

that said, I totally agree we shouldn't hold up the next release for this, and it does seem to be a can of worms that requires a more clear plan to address the underlying causes, rather than a patch that attempts to address some of the symptoms.