elekto-io / elekto

Software for the Elekto online elections system
https://elekto.io
Apache License 2.0
30 stars 19 forks source link

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

Open geekygirldawn opened 1 year ago

geekygirldawn commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 2 months ago

Heh, we just ran into this again with this year's Kubernetes steering election, can someone review https://github.com/elekto-io/elekto/pull/99 ?