Skip to content

Follow-up from "Add group web-hooks failed notifications"

The following discussion from !110824 (merged) should be addressed:

  • @.luke started a discussion: (+7 comments)

    Suggestion

    I have a concern here that we're introducing a permission read_web_hooks that sounds nearly identical to an existing permission called read_web_hook.

    But read_web_hook and read_web_hooks seem to allow and deny differently is that right?

    And then another layer of confusion (for me!) is Groups::HooksController uses admin_group to decide if a hook can be viewed. I can't see where we use read_web_hook (non-plural) in our codebase, but it's not in any controllers as far as I can see.

    I'm worried that we're setting ourselves up for confusion in future, over which permission is the permission to use when we want to check if a user can view web hooks for a group.

    🤔 I see that we already have this set up for project hooks, and we're following the same pattern. Project hooks have:

    • read_web_hooks defined in ProjectPolicy
    • read_web_hook defined in ProjectHookPolicy

    And I wouldn't be sure which to use when I want to check if a user can read a project hook! Like Group::HooksController, Projects::HooksController actually just uses admin_project to decide if someone can see the hook.

    Just to step back a bit - is the intention of this new permission really just to check if we should display the callout, which is to say, maintainers should see it and no one else - and in that case, could we just check again maintainer_access for both the group and project call out?

    Ability.allowed?(current_user, :maintainer_access, group)