Only store password in config for GitHub Enterprise (due to Enterprise limitations)

[nttibbetts]

Generally not a good idea to store passwords in cleartext, even moreso when a token is generated immediately after authenticating that makes storing the password unnecessary.

Please let me know if there's anything I missed or didn't think of when removing this.

[codecov-io]

Current coverage is 95.27% (diff: 100%)

Merging #79 into master will increase coverage by 0.08%

@@             master        #79   diff @@
==========================================
  Files            34         34          
  Lines          2076       2094    +18   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1976       1995    +19   
+ Misses          100         99     -1   
  Partials          0          0          

Powered by Codecov. Last update 699e6a9...2c30940

[donnemartin]

Hi @nttibbetts, gitsome should not be storing the password in the config, see https://github.com/donnemartin/gitsome/commit/98596faae50d87b1b68dd916b47691b14cb107b5.

Are you seeing otherwise?

I think the docstrings could be updated and it seems I missed removing some dead code.

[donnemartin]

Hmm, actually I think you're right, seems I missed a code path. Reviewing...

[nttibbetts]

Sadly, yes, I am still seeing this. It seems it would only happen when using gitsome for the first time and letting it create the access token for you.

It seems you might have caused a regression when adding enterprise support db8e5d71

[donnemartin]

I think this might be a little more complicated. Enterprise doesn't seem to allow exchanging the password for a token:

https://github.com/donnemartin/gitsome/blob/master/gitsome/config.py#L293-L294

I'll have to fire up my sandbox Enterprise instance and do some more testing tomorrow.

[donnemartin]

I think the following diff might be the most straightforward way to address this issue. If you agree, could you please update the PR?

--- a/gitsome/config.py
+++ b/gitsome/config.py
@@ -91,6 +91,9 @@ class Config(object):

     :type user_pass: str
     :param user_pass: The user's pass in ~/.gitsomeconfig.
+        This is only stored for GitHub Enterprise if the user supplies a
+        password instead of a token.  Exchanging a password for a personal
+        access token does not seem to be supported for GitHub Enterprise.

     :type user_token: str
     :param user_token: The user's token in ~/.gitsomeconfig.
@@ -616,10 +619,6 @@ class Config(object):
             parser.set(self.CONFIG_SECTION,
                        self.CONFIG_USER_LOGIN,
                        self.user_login)
-            if self.user_pass is not None:
-                parser.set(self.CONFIG_SECTION,
-                           self.CONFIG_USER_PASS,
-                           self.user_pass)
             if self.user_token is not None:
                 parser.set(self.CONFIG_SECTION,
                            self.CONFIG_USER_TOKEN,
@@ -632,6 +631,10 @@ class Config(object):
                 parser.set(self.CONFIG_SECTION,
                            self.CONFIG_ENTERPRISE_URL,
                            self.enterprise_url)
+                if self.user_pass is not None:
+                    parser.set(self.CONFIG_SECTION,
+                               self.CONFIG_USER_PASS,
+                               self.user_pass)
             parser.set(self.CONFIG_SECTION,
                        self.CONFIG_VERIFY_SSL,
                        self.verify_ssl)

[Edit] Change summary:

• Update the docstring for user_pass
• Move the parser.set call for CONFIG_USER_PASS to within the if self.enterprise_url is not None check in def save_config(self)

[nttibbetts]

Only tweak I would suggest is keeping the remove_option call if it's not an enterprise user so as to clean up any cases that have already occurred.

I'll make changes and throw in a couple of tests for this.

[donnemartin]

Sounds good, thanks!

[donnemartin]

@nttibbetts looks like you're updating the PR, please let me know when it's ready for another review.

[nttibbetts]

@donnemartin it's all set for another review.

[donnemartin]

Nice job @nttibbetts, thanks for making the changes.

Do any GitHub Enterprise users want to do a sanity test on this change?

[donnemartin]

Tested this on GitHub Enterprise, looks good 👍