Skip to content

fix: use case-insensitive comparison for GitHub orgs to fix RBAC #2499 #4641

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
venkatamutyala wants to merge 1 commit into
base: master
Choose a base branch
from
+38 −2
Open

fix: use case-insensitive comparison for GitHub orgs to fix RBAC #2499 #4641

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions connector/github/github.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -699,8 +699,8 @@ type org struct {
// which inserts a bearer token as part of the request.
func (c *githubConnector) teamsForOrg(ctx context.Context, client *http.Client, orgName string) ([]string, error) {
apiURL, groups := c.apiURL+"/user/teams", []string{}
warnedCaseMismatch := false
for {
// https://developer.github.com/v3/orgs/teams/#list-user-teams
var (
teams []team
err error
Expand All @@ -710,7 +710,17 @@ func (c *githubConnector) teamsForOrg(ctx context.Context, client *http.Client,
}

for _, t := range teams {
if t.Org.Login == orgName {
// GitHub org names are case-insensitive; the API returns canonical (lowercase) form.
// https://docs.github.com/en/rest/teams/teams?apiVersion=2026-03-10#list-teams-for-the-authenticated-user
if strings.EqualFold(t.Org.Login, orgName) {
Copy link
Copy Markdown
Member

@nabokihms nabokihms Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good to me, but let's add the comment on why the comparison here is case-insensitive.

Also, it's kind of a breaking change for people who, for some reason, used an org with an upper case in their configs. Users from these orgs can log in now, but they could not before this change.

Copy link
Copy Markdown
Author

@venkatamutyala venkatamutyala Mar 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nabokihms Added a comment and updated the link as well to go straight to the relevant API call.

RE: breaking change.

Potentially. I would think if you didn't want an org to have access you would have removed the org entirely rather than relying on the current case mismatch as security.

Copy link
Copy Markdown
Author

@venkatamutyala venkatamutyala Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nabokihms thoughts?

Copy link
Copy Markdown
Member

@nabokihms nabokihms Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: let's make the comment a bit more specific:

// GitHub org names are case-insensitive; the API returns canonical (lowercase) form.
// [link-to-the-API-doc]

Copy link
Copy Markdown
Author

@venkatamutyala venkatamutyala May 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. @nabokihms

Copy link
Copy Markdown
Member

@nabokihms nabokihms Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add tests for this edge-case?

Copy link
Copy Markdown
Author

@venkatamutyala venkatamutyala May 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nabokihms Added. Please let me know if you have any feedback on it.

Copy link
Copy Markdown
Member

@nabokihms nabokihms Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The warning is required if there is a case mismatch.

  1. In the final groups, the ID token org name will be in lowercase, so we need to warn users that it was converted to lowercase
  2. we need to suggest using lowercase orgs in the config

The warning will solve both.

Copy link
Copy Markdown
Author

@venkatamutyala venkatamutyala May 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added. Let me know if you have any feedback on the wording.

if !warnedCaseMismatch && t.Org.Login != orgName {
c.logger.Warn(
"org name in config differs in case from GitHub canonical form; use lowercase in config to keep group claims consistent",
"org", orgName,
"canonical", t.Org.Login,
)
warnedCaseMismatch = true
}
groups = append(groups, c.teamGroupClaims(t)...)
}
}
Expand Down
26 changes: 26 additions & 0 deletions connector/github/github_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,32 @@ func TestUserGroupsWithTeamNameAndSlugFieldConfig(t *testing.T) {
})
}

// tests case-insensitive org matching and the warn log on case mismatch
func TestTeamsForOrgCaseInsensitive(t *testing.T) {
s := newTestServer(map[string]testResponse{
"/user/teams": {
data: []team{
{Name: "team-1", Org: org{Login: "myorg"}},
{Name: "team-2", Org: org{Login: "myorg"}},
{Name: "team-3", Org: org{Login: "otherorg"}},
},
},
})
defer s.Close()

var logBuf strings.Builder
logger := slog.New(slog.NewTextHandler(&logBuf, &slog.HandlerOptions{Level: slog.LevelWarn}))
c := githubConnector{apiURL: s.URL, logger: logger}

for _, configOrg := range []string{"MyOrg", "MYORG", "myOrg"} {
groups, err := c.teamsForOrg(context.Background(), newClient(), configOrg)
expectNil(t, err)
expectEquals(t, groups, []string{"team-1", "team-2"})
}

expectEquals(t, strings.Count(logBuf.String(), "level=WARN"), 3)
}

// tests that the users login is used as their username when they have no username set
func TestUsernameIncludedInFederatedIdentity(t *testing.T) {
s := newTestServer(map[string]testResponse{
Expand Down