Hi Sam Gleske , we are picking up this story now after some delay. I'm updating you below on where we are at, and what our thinking is for the next step.
I made an attempt at implementing the hybrid solution ("if matrix role is defined for user, use that, else, use github derived role"). From an implementation perspective, I have not succeeded. It requires nesting the UI widgets of the 2 plugins:
From a maintainability perspective, I would like to treat the above two widgets as black boxes. Our feature should just compose the underlying features.
The correct approach appears to be to use jelly include in order to compose the page:
- Matrix widget: <st:include page="config.jelly" class="hudson.security.GlobalMatrixAuthorizationStrategy" />
- GH widget: <st:include page="config.jelly" class="org.jenkinsci.plugins.GithubAuthorizationStrategy" />
However I receive a runtime exception when attempting to render the page using the above approaches (I don't have it on hand sorry, I have since moved to a new approach). I suspect that the jelly framework is not very great at encapsulating UI components to allow this kind of composition, and I'll be required to jump through hoops to plumb it all together.
Rethinking the approach
Let's come back to the original problem: this plugin has a security vulnerability. A malicious user could compromise the credential on an agent, and use it to become an admin on the master. This is because agent->master accounts currently require full admin. This is especially problematic in enterprise environments, where agents can be operated by other teams, so it is trivial for them to obtain the agent credential. Jenkins has made steps to address this with Slave to Master Access Control, but that approach is undermined by the flaw in this plugin.
In order to make it easy for people to do the right thing, I feel that the original suggestion of adding a new configuration, agentUserNames, is the better approach. This is because it specifically addresses the security problem, in an opinionated way. The alternate suggestion of giving users a super-generic matrix-hybrid permission system also allows the problem to be solved, but it does not present the security flaw. Most users will probably keep doing what they are doing if we offer the hybrid solution.
Therefore, I think the original suggestion is more pragmatic, solving the security flaw, and making it obvious to users that the flaw exists.
We are going with the original approach suggested in this ticket, since it is much easier to implement, and leads to better security outcomes for the community.