Administrator requirement for outside collaborators does not seem to work with an admin team

[ossf]

Administrator requirement for outside collaborators does not seem to work with an admin team

[coheigea]

FAO @jeffmendoza

[justaugustus]

I was about to open a separate issue, but maybe it makes sense to tack onto this...
I got this issue soon after the PR was merged.

Could we invert the logic and make ownerless allowed by default, mostly because ownerless is somewhat of a misnomer?
Any org with sufficient org admins mean the repos have owners transitively.

For org admins that are fine with that, we should not change behavior and notify them, but instead allow them to opt-in to the new restriction.

ref: #88
cc: @olivekl

[justaugustus]

I could also see a few other scenarios:

• no outside collaborators
• outside collaborators, but not oc admins
• outside collaborators are allowed admin
• outside collaborators are allowed admin IFF there are org member admins
• ownerless disallowed
• ownerless allowed

[jeffmendoza]

However, the repo has a team with admin access, and a member of the org is on this team. Perhaps the change didn't take into account the scenario of an admin team as opposed to a member being an admin directly on the team?

@coheigea I did take this into account, and it should be passing in this case. (code is here), however there could be a bug. Do you mind sharing the org/name of your repo, and I can check the logs? I'll double check the logic in my test org as well.

Could we invert the logic and make ownerless allowed by default, mostly because ownerless is somewhat of a misnomer?
Any org with sufficient org admins mean the repos have owners transitively.
For org admins that are fine with that, we should not change behavior and notify them, but instead allow them to opt-in to the new restriction.

@justaugustus A couple thoughts here. While I acknowledge that it is least disruptive to have new features turned off by default, it is also a goal for me to have Allstar's default settings represent the highest level of security best practices that we (OpenSSF) can recommend. For example, the Branch Protection default settings are quite restrictive. I don't think it makes sense for the policy defaults to be a relic of the order in which they were implemented. An area I should improve here is documentation. We should have a "What's New" or "Changelog" front and center covering these changes and not leave them buried in the commit history.

That said, I also acknowledge that my point of view is from a large-org perspective, and while the org admins do have transient permissions on all repos, they can't be expected to be responsible for all repos, and a direct admin should be required. I see how this would not be the case for a small org. I'd like to bring this up at the next "Best Practices" WG meeting to discuss further.

[coheigea]

@jeffmendoza The issue is raised here: Talend/helm-charts-public#43

[jeffmendoza]

@coheigea Thank you! I've checked the logs and unfortunately Allstar is not picking up any teams for your repo. I'm not quite sure why, as it is picking up teams for some other installations. It would be very helpful if you could do a little debug, would this be possible?

Create your own PAT then run something like this for your repo:

curl \
  -H "Authorization: token $(cat ~/ghpat)" \
  -H "Accept: application/vnd.github.v3+json" \
  https://api.github.com/repos/jeffmendoza-test-org/test-repo-two/teams

(I keep my pat in ~/ghpat)
For my test repo, I get:

[
  {
    "name": "Admins",
    "id": 5490956,
    "node_id": "T_kwDOBb-5Cc4AU8kM",
    "slug": "admins",
    "description": "",
    "privacy": "closed",
    "url": "https://api.github.com/organizations/96450825/team/5490956",
    "html_url": "https://github.com/orgs/jeffmendoza-test-org/teams/admins",
    "members_url": "https://api.github.com/organizations/96450825/team/5490956/members{/member}",
    "repositories_url": "https://api.github.com/organizations/96450825/team/5490956/repos",
    "permission": "admin",
    "permissions": {
      "admin": true,
      "maintain": true,
      "push": true,
      "triage": true,
      "pull": true
    },
    "parent": null
  }
]

Just let me know if you get any teams, and if they have the permissions that you expect (admin). Thanks!

[coheigea]

Hi @jeffmendoza ,

Calling https://api.github.com/repos/talend/helm-charts-public/teams I can see two teams, the second of which is the admin team:

[jeffmendoza]

@coheigea Thank you for testing!

I've figured out the issue. The API is supposed to work on any repo, regardless of org permissions, if the App requested "administration read" permissions, which Allstar did.

However, it seems that this API is not working on orgs that have the App installed on "Only select repositories", if Allstar is installed on "All repositories" for the org, then the API works fine. I believe this to be a bug with the GitHub API and have opened a support ticket with them.

While we wait, and I potentially have to rework the code to use a different API, I will disable this feature. Sorry for the noise, and thanks again for helping.

[olivekl]

@jeffmendoza We could add a doc update in the meanwhile. The current installation directions in the README.md suggest choosing "All repositories" at the App installation step but with no indication of why. While this gets sorted out, we could word it a bit more strongly. Something like: "Choose 'All Repositories' under Repository Access, even if you plan to disable Allstar on some repositories later. If you do not choose 'All Repositories' in this step, certain Allstar features may not work." Would that help?

[coheigea]

Thanks for the great support @jeffmendoza , hopefully GitHub can fix the issue quickly, because I think it's a valuable check.

It could be extended to configure a maximum number of individual admins for a given repo, so that we can enforce policies about how many people can have admin privileges on a repo.

[jeffmendoza]

This is fixed in #115 , but it not turned on in default settings. I won't turn it on by default without further communication to users.