To recreate, do the following:

  1. User alpha creates a new project and creates some releases for the project. User alpha has administrative privileges and sets the status of one of the release nodes to 0 (unpublished).
  2. User alpha gives CVS commit access to user beta via the CVS access tab on the project.
  3. User beta is an authenticated user but has no additional roles. On the site, authenticated users have the following (relevant) permissions:
  • CVS module: Access CVS messages
  • node module: access content
  • project module: access own projects, access projects, maintain projects
  • If user beta goes to the "View all releases" page for the project created in step 1, he will see all of the releases, including the unpublished release.
  • If user beta clicks on the title of the unpublished release, he will get an "access denied" message.
    • CommentFileSizeAuthor
    #9 cvs_access_view_nodes_4.patch978 bytesdww
    #8 cvs_access_view_nodes_3.patch980 byteshunmonk
    #7 cvs_access_view_nodes_2.patch1.01 KBdww
    #6 cvs_access_view_nodes_1.patch952 byteshunmonk
    #4 cvs_access_view_nodes_0.patch617 byteshunmonk
    #2 cvs_access_view_nodes.patch888 byteshunmonk

    Comments

    dww’s picture

    dww
    we/he/they
    commented

    i could have sworn this was all working as expected not long ago. i wonder if i just managed to overlook this in my testing due to role/permission confusion, or if something introduced a regression. guess we'll just have to debug it and find out.

    hunmonk’s picture

    hunmonk commented
    Status: Active » Needs review
    StatusFileSize
    new cvs_access_view_nodes.patch888 bytes

    it works for the release overview page, where project_check_admin_access($project) is properly called. however, there is no access check like this for viewing individual nodes. attached patch adds that check to project_release_access() 'view' op.

    dww’s picture

    dww
    we/he/they
    commented

    Thanks for the diagnosis, sounds right. This patch looks ok to me. However, here's an alternative:

    project_project_access() could check for project admin access in the $op 'view' case. Given the "CVS access makes you a maintainer" thing, that seems like what you'd expect, e.g. on sites that only allow "access own projects" permission -- if someone gave you CVS access to a project, you should be treated as if it's your "own", too. That'd keep project_release_access() simple and sharing the identical logic with project_project_access(), which is nice. And, that seems more consistent with the other "once you have CVS access it's like you own the project" behavior, though I'll agree it's not so consistent with other permissions regarding your "own" nodes.

    If you object to this approach, then your patch is RTBC.

    hunmonk’s picture

    hunmonk commented
    StatusFileSize
    new cvs_access_view_nodes_0.patch617 bytes

    yeah, that approach seems most sensible. attached should handle it. i considered removing the if (user_access('access own projects') && $node->uid == $user->uid) { check in that function as well, but it looks like project_check_admin_access() requires 'maintain projects' perm to allow viewing in the case of being the node owner, so i guess this patch i have provides the most consitency with the least amount of code change.

    dww’s picture

    dww
    we/he/they
    commented
    Status: Needs review » Reviewed & tested by the community

    Yup, that's great. Thanks.

    hunmonk’s picture

    hunmonk commented
    Status: Reviewed & tested by the community » Needs review
    StatusFileSize
    new cvs_access_view_nodes_1.patch952 bytes

    not quite. project_check_admin_access() needs the project node, not the release node :)

    attached tests for the node type, and loads the project if it's not a project_project type. tested this, and it works to solve this specific bug, and i don't think it will introduce any regressions.

    dww’s picture

    dww
    we/he/they
    commented
    StatusFileSize
    new cvs_access_view_nodes_2.patch1.01 KB

    I think this is a more straight-forward way to do this. I don't care that it slightly changes the semantics of who can view projects in these weird edge-cases. ;)

    hunmonk’s picture

    hunmonk commented
    StatusFileSize
    new cvs_access_view_nodes_3.patch980 bytes

    same effect. a bit simpler.

    dww’s picture

    dww
    we/he/they
    commented
    StatusFileSize
    new cvs_access_view_nodes_4.patch978 bytes

    Maybe this is even better... We can only assume $node->pid is what we think it is if it's a 'project_release' node. Instead of double negative, let's express this explicitly.

    hunmonk’s picture

    hunmonk commented
    Status: Needs review » Fixed

    i think we've split enough hairs here :)

    tested and committed the latest patch.

    Anonymous’s picture

    (not verified) commented
    Status: Fixed » Closed (fixed)