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

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

hailan
Review Board
release-4.0.x
reviewboard, students

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 Author
Normalize headers from GitHub for consistency in Python 2 and 3
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

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

daviddavid

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

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. 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. 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.

  4. 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.
  5. 
      
hailan
hailan
mike_conley
  1. 
      
  2. 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. 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 Author
-
Fix GitHub auth token bug using wrong HTTP header capitalization
HailanXyouknow
+
Fix GitHub auth token bug using wrong HTTP header capitalization in Python2
HailanXyouknow

Diff:

Revision 3 (+8 -2)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

hailan
david
  1. 
      
  2. 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)
     
     
     

    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: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (b6923f4)
Loading...