fix: use case-insensitive comparison for GitHub orgs to fix RBAC #2499#4641
fix: use case-insensitive comparison for GitHub orgs to fix RBAC #2499#4641venkatamutyala wants to merge 2 commits intodexidp:masterfrom
Conversation
…dp#2499 Signed-off-by: Venkat <venkata@venkatamutyala.com>
venkatamutyala
force-pushed
the
fix/case-insensitive-comparison
branch
from
adc1055 to
0dd0951
Compare
nabokihms
left a comment
nabokihms
left a comment
There was a problem hiding this comment.
I have one comment and a question. The original issue was about the org name, but this PR fixes team names, not the org name. Am I missing something?
|
|
||
| for _, t := range teams { | ||
| if t.Org.Login == orgName { | ||
| if strings.EqualFold(t.Org.Login, orgName) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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]
Signed-off-by: Venkat <venkata@venkatamutyala.com>
venkatamutyala
force-pushed
the
fix/case-insensitive-comparison
branch
from
f55c0d0 to
90afcad
Compare
@nabokihms I think the function is a little misleading. It's fetching the users teams across all orgs and then filters them by org name. It's the comparison on org name where it's silently failing because it's case sensitive. I updated the link to the API call as well in my most recent commit |
|
|
||
| for _, t := range teams { | ||
| if t.Org.Login == orgName { | ||
| if strings.EqualFold(t.Org.Login, orgName) { |
There was a problem hiding this comment.
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]
| for _, t := range teams { | ||
| if t.Org.Login == orgName { | ||
| // case-insensitive check to avoid silent failures when config casing is different than GitHub | ||
| if strings.EqualFold(t.Org.Login, orgName) { |
There was a problem hiding this comment.
Could you also add tests for this edge-case?
| for _, t := range teams { | ||
| if t.Org.Login == orgName { | ||
| // case-insensitive check to avoid silent failures when config casing is different than GitHub | ||
| if strings.EqualFold(t.Org.Login, orgName) { |
There was a problem hiding this comment.
The warning is required if there is a case mismatch.
- 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
- we need to suggest using lowercase orgs in the config
The warning will solve both.
2 participants
Overview
Previously, configuring
orgs: [{name: "MyOrg"}]when the actual org wasmyorgwould silently return empty groups, causing users to be denied access despite being valid org membersWhat this PR does / why we need it
adds case-insensitive comparison for GitHub organization names
Closes #2499
Special notes for your reviewer
For testing, I did a docker build, updated my dex install to use this custom image, i changed my github org to have mixed casing and then tested the login via dex github connector and had no issues.