Skip to content
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.

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

GitHub handles should not be case sensitive in voters.yaml for eligibility #84

Open
geekygirldawn opened this issue Nov 15, 2022 · 6 comments · May be fixed by #99
Open

GitHub handles should not be case sensitive in voters.yaml for eligibility #84

geekygirldawn opened this issue Nov 15, 2022 · 6 comments · May be fixed by #99
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@geekygirldawn
Copy link
Contributor

In the Knative SC 2022 election, we just ran into a case where in voters.yaml, the user "pierdipi" in voters.yaml showed up as ineligible to vote. After updating voters.yaml to use "pierDipi", the user became eligible to vote. I don't think that GH handles should be treated as case sensitive in Elekto unless this is a limitation into how the GH Auth handles logins?

@upodroid
Copy link

upodroid commented Nov 15, 2022

The GitHub API is case insensitive.

 REDACTED  MCW0CDP3YY  ~  $  gh api users/geekygirldawn | jq .login
"geekygirldawn"
 REDACTED  MCW0CDP3YY  ~  $  gh api users/GeekyGirlDawn | jq .login
"geekygirldawn"

@jberkus
Copy link
Member

jberkus commented May 1, 2023

I've been doing some work on this, and it is WIP. Unfortunately, comparing the IDs with the list happens in every individual model and template where it's needed; there's no utility comparison function. So I'm having to go through every function where we make use of the ID (most of them) and check them individually. This is gonna take a while to fix.

@jberkus jberkus added bug Something isn't working help wanted Extra attention is needed labels May 10, 2023
@jberkus
Copy link
Member

jberkus commented May 10, 2023

If someone wants to take this on, here's what's required:

  1. Add two functions to models/meta.Election that checks GH IDs against either the voter or candidate list (probably a 3rd one for admin list, but that's less critical)
  2. Replace every place the GH IDs are compared with calls to these functions (there's a lot of locations).
  3. Write a test to prove that the above is working correctly.

@cblecker
Copy link

cblecker commented Aug 8, 2023

The way we've handled this in the Kubernetes GitHub tooling is using a function that whenever we pull a github username from a data source (when we make an API call, or when we read in data from a YAML source file) we normalize it by stripping the leading "@" if it exists, and forcing it to lower case internally:
https://github.com/kubernetes/test-infra/blob/2c2cf0d32eed64b8f30e2c31835075ce501cf07a/prow/github/types.go#L165-L168

That way you're always comparing a lowercase username string no matter where the data comes from.

@mjlshen
Copy link

mjlshen commented Aug 13, 2023

If someone wants to take this on, here's what's required:

  1. Add two functions to models/meta.Election that checks GH IDs against either the voter or candidate list (probably a 3rd one for admin list, but that's less critical)
  2. Replace every place the GH IDs are compared with calls to these functions (there's a lot of locations).
  3. Write a test to prove that the above is working correctly.

I can start working on this - I may stumble a bit on part 3, but I'll raise any knowledge gaps I think I have on the PR.

@BenTheElder
Copy link

Heh, we just ran into this again with this year's Kubernetes steering election, can someone review #99 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants