Fix GitHub auth token bug using wrong HTTP header capitalization in Python2.7

Review Request #11173 — Created Sept. 17, 2020 and submitted

Information

Review Board
release-4.0.x

Reviewers

When trying to add a newly configured repo, it complained about the
GitHub Auth Token missing required permissions.

The underlying issue was that the wrong capitalization was used when
getting the OAuth scopes header for Python2.7.

Did the following with all relevant versions of Python3 and 2:

  • Ran unit tests and they passed

  • Manually added a repo with with a personal access token

Summary ID Author
Normalize headers from GitHub for consistency in Python 2 and 3
d71bb40fba95823bd8afc846196986da26191112 HailanXyouknow
Description From Last Updated

The summary doesn't need to contain the branch name (that's already shown in the "Branch" field). We can also be …

daviddavid

The testing done is quite comprehensive, and I don't want to discourage that too much, but we can make it …

daviddavid

For the description, you can remove the first line. We don't need to duplicate the information from the summary. The …

daviddavid

If we're going to wrap the description at 70 columns, then I think the newline breaks need to happen after …

mike_conleymike_conley

So I just was running my devserver on Python 3.8, and I seem to be seeing the header with the …

daviddavid

Just to check, did you run the run tests on both Python 2.x and Python 3.x? If so, can you …

daviddavid

W293 blank line contains whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ':'

reviewbotreviewbot

Let's flesh this out a bit. Maybe something like: urllib returns different capitalization for header names depending on whether we're …

daviddavid
hailan
  1. 
      
  2. 
      
hailan
hailan
david
  1. The code itself looks good. I do have a few suggestions for how we can improve the summary/description/testing done:

  2. Show all issues

    The summary doesn't need to contain the branch name (that's already shown in the "Branch" field).

    We can also be a bit more specific here. Something like "Fix GitHub auth token bug using wrong HTTP header capitalization"

  3. Show all issues

    The testing done is quite comprehensive, and I don't want to discourage that too much, but we can make it more concise. I usually do something like:

    • Ran unit tests.
    • Was able to manually add a GitHub repository using a personal auth token.
  4. Show all issues

    For the description, you can remove the first line. We don't need to duplicate the information from the summary.

    The description is pretty good about explaining the user-visible appearance of the bug, but let's also explain the underlying cause (We were correctly fetching the token, but then looking for the wrong capitalization in the HTTP headers).

    Let's also wrap the description to 70 columns, which makes it easier to read, especially when it gets sucked into the commit message when we land it.

  5. 
      
hailan
hailan
mike_conley
  1. 
      
  2. Show all issues

    If we're going to wrap the description at 70 columns, then I think the newline breaks need to happen after "about the".

    Maybe cut and paste the description into your editor to make sure it's formatted correctly. Your editor should probably have some kind of column counter visible to tell you where your cursor is - or might even have a built in visual ruler.

    I also think "Underlying issue:" as a section isn't really needed. We can turn this into a sentence, like,

    "The underlying issue was that we weren't using the right capitalization when getting the OAuth scopes header."

    Something like that, anyhow.

    1. Ah, yes! The editor does have a column counter, I just noticed that!
      Thank you :D

  3. 
      
hailan
mike_conley
  1. Looks great, thank you!

  2. 
      
hailan
  1. Ship It!

  2. 
      
david
  1. 
      
  2. Show all issues

    So I just was running my devserver on Python 3.8, and I seem to be seeing the header with the older capitalization. Can you test that this works when running on that?

    If not, we probably need to normalize header capitalization throughout.

    1. Oops, was just running it like './contrib/internal/devserver.py' without any python version attached.
      Will be checking it against all Python versions next time.
      Yes, I will be normalizing it. Thanks David :)

  3. 
      
hailan
Review request changed
Change Summary:

Changes:

  • Added a line to normalize keys from header

  • Deleted the changes to hostingsvcs/tests/test_github.py to test
    the normalization of the keys in the header

Summary:
Fix GitHub auth token bug using wrong HTTP header capitalization
Fix GitHub auth token bug using wrong HTTP header capitalization in Python2.7
Description:
   

When trying to add a newly configured repo, it complained about the

    GitHub Auth Token missing required permissions.

   
   

The underlying issue was that the wrong capitalization was used when

~   getting the OAuth scopes header.

  ~ getting the OAuth scopes header for Python2.7.

Testing Done:
   
  • Ran unit tests and they passed
~  
  • Manually added a repo with a personal access token and it worked
  ~
  • Manually added a repo with Python3.8, 3.7, 3.6 and 2.7 with a
    personal access token and it worked
Commits:
Summary ID Author
Fix GitHub auth token bug using wrong HTTP header capitalization
f0f3172588ad0dfc9c14f3aa20a7a09cf7bcb67d HailanXyouknow
Fix GitHub auth token bug using wrong HTTP header capitalization in Python2
76ca7e44375f653110b8fa6824db65856cb78aa7 HailanXyouknow

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

hailan
david
  1. 
      
  2. Show all issues

    Just to check, did you run the run tests on both Python 2.x and Python 3.x? If so, can you make that clear in your testing done section?

  3. reviewboard/hostingsvcs/github.py (Diff revision 4)
     
     
     
    Show all issues

    Let's flesh this out a bit. Maybe something like:

    urllib returns different capitalization for header names depending on whether we're on Python 2 or Python 3. Normalize everything is in lower case for consistency.

    Note that there's also a typo here (oath -> oauth), in case you keep any of your prior comment's wording.

  4. 
      
hailan
chipx86
  1. Ship It!
  2. 
      
hailan
Review request changed
Status:
Completed
Change Summary:
Pushed to release-4.0.x (b6923f4)