Team model uses wrong Permission enum

[jMarkP]

It looks like commit 598c7c3 broke deserialization of permissions on the Team model when getting teams with access to a repository (we're on GitHub Enterprise, but AFAICS the Team DTO is the same on GitHub.com).

Curling the repository list teams endpoint on our GitHub Enterprise installation gives responses like

[
  {
    ...
    "permission": "push",
    ...
  },
  {
    ...
    "permission": "pull",
    ...
  },
  {
    ...
    "permission": "admin",
    ...
  },
]

Which matches Permission (which the field on Team used to be before that commit), but not PermissionLevel (what it is now).

When I use a collaborator endpoint and try to examine the permissions on the response I get a stack trace like

System.ArgumentException: Value 'push' is not a valid 'PermissionLevel' enum value.
   at Octokit.StringEnum`1.ParseValue() in /home/runner/work/octokit.net/octokit.net/Octokit/Models/Response/StringEnum.cs:line 149
   at Octokit.StringEnum`1.get_Value() in /home/runner/work/octokit.net/octokit.net/Octokit/Models/Response/StringEnum.cs:line 45
   at Foo.ConvertPermission(StringEnum`1 permission) 
   at Foo.<GetRepositoryTeamsAsync>b__7_0(Team x) 

(This is running against GitHub Enterprise versions 2.22.7 and 3.0.5)

(Edited to correct a link to the GitHub docs which linked to the wrong API endpoint)

[jMarkP]

This is when calling RepositoriesClient.GetAllTeams()

[jMarkP]

For the time being we're working around this with

var permission = new StringEnum<Permission>(team.Permission.StringValue)

[mockjv]

This appears to also affect the Permission enum as well, which is limited to pull, push, and admin.

[husseinsakr]

Issue found in version: 2.0.1

We are also seeing this issue when getting all pull requests on the git repository

var allPullRequests = await github.Repository.PullRequest.GetAllForRepository(org, repo);

The error we are seeing is

Value 'push' is not a valid 'PermissionLevel' enum value.

To re-produce

1. Add a github team to to file .github/CODEOWNERS - Example: * @org/my-team
2. Call the api

We think this PR introduced this bug #2156

We downgraded to version 0.44. The issue is resolved

[StingyJack]

@nickfloyd - what is allowed for a fix on this? Revert the property type from the StringEnum PermissionLevel to the enum Permission? Without changing all of the underlying data to comply with the values allowed by PermissionLevel, I dont know what else is going to be reasonable.

[nickfloyd]

This one is a bit of a mess. Great sleuthing y'all. Given the use cases:

PermissionLevel should be for App Installations

"permissions": {
  "read",
  "write",
  "admin"
  },

Permissions for teams

On create:

  "permissions": {
    "push",
    "pull"
  },

On update:

  "permissions": {
    "admin",
    "push",
    "pull"
  },

There is also the concept of RepositoryPermissions (also called ReviewPermissions) for collaborators

"permissions": {
      "pull",
      "triage",
      "push",
      "maintain",
      "admin"
    },

I did a quick run-through on what's in main, and there seems to be a bit of a mismatch on a handful of the uses detailed above. Given that all of these should be fixed and aligned with the REST APIs themselves, here's my recommendation:

We find all instances of permissions enum mismatches (there are a few, but not too many), correct them, label that as a breaking change, and bump the major version detailing why. While that won't "fix" the underlying data, it will prevent future issues and unblock folks.

We might also consider doing the following for code clarity:

• Rename Permissions enums and classes to more appropriately fit the resource it's tied to
• Add esoteric permission enums (this is not DRY code but it will be clear code) to reduce confusion
• Consider having all of the permissions-related enums in one permissions file / class

Thoughts? This is up for grabs, but if no one can get to it - I'd be glad to knock it out, given that this could cause data issues.

[kfcampbell]

@nickfloyd that seems like a reasonable proposal to me!

Priority-wise, I'm not sure I have a great idea of where this should fit in.

[notauserx]

@nickfloyd I would like to take a crack at it.

[nickfloyd]

@notauserx Let us know if you are still interested in giving this one a shot, and thank you for offering! ❤️ If so, just assign it to yourself and move it to in progress

[notauserx]

@nickfloyd, I would be happy to get this done. Can you please assign it to me, I don't think(or can't figure out) how to assign it to myself.

My PR fixes this permission for the teams and updates the endpoints for several endpoints on the ITeamsClient that are related to permissions. I have written a summary of my changes here.

I stopped here because the PR was already quite huge and I was thinking to get some reviews/feedback.

With the changes so far, the permissions for teams are updated, however, I did not touch all the other permissions enum mismatches. Do we want to put all that in a single PR, or get this merged and then tackle the other instances?

[nickfloyd]

@notauserx I think your line of thinking is the best route - let's merge your PR when you feel it's g2g and it's been reviewed one last time and then move on to the other instances as you suggest. That way we can avoid the tax of a huge PR and the risk something like that could introduce.

[notauserx]

@nickfloyd cooking a PR to update the remaining permission enum mismatches(App installations and Collaborators) based on your suggestions.