Don't store repository credentials when using a hosting service.

Review Request #8469 — Created Oct. 17, 2016 and submitted

Information

Review Board
release-2.0.x
c61de4b...

Reviewers

Browsers absolutely love to auto-fill anything that resembles a
username/password field, despite this frequently being a terrible idea
for services requiring linking up with other services. As such, it's
common for a username/password to end up in a repository, even when a
hosting service is being used.

While not always a problem, some services do end up affected, like
TFS/TFS-Git in Power Pack. They make a call to
Repository.get_credentials(), which prioritizes the repository's
credentials over the hosting service's, for cases where a repository may
use something different than the hosting service (although no hosting
services currently allow for setting both, that's how the existing
behavior has been up until now).

This change turns off username/password storage for a repository when
using a hosting service, to help prevent that sort of problem. It also
removes any stored credentials on upgrade, to help with breakages or
bugs that may exist due to improper stored credentials.

Tested creating/editing TFS and TFS-Git repositories, which were
affected by this bug. The repositories didn't end up containing any
password information, leaving it fully to the hosting service.

Tested saving a repository without a hosting service. Saw that the
passwords did store.

Tested doing a site upgrade on a site with bad passwords stored for
the repositories. Saw that the passwords were removed.

Description From Last Updated

We're not doing anything else with respositories so why not just chain the .update() call in there?

brenniebrennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/cmdline/rbsite.py
        reviewboard/scmtools/forms.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/cmdline/rbsite.py
        reviewboard/scmtools/forms.py
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/cmdline/rbsite.py (Diff revision 1)
     
     
     
     
     
     
     
    Show all issues

    We're not doing anything else with respositories so why not just chain the .update() call in there?

    1. It's kinda weird to do:

      (
          Repository.objects
          .blah
          .blah
          .update(...)
      )
      

      Our other queries like this are used for looks/queryset building. I'd like to separate that out from operations being performed. Best not to bury that inside what looks like a chainable queryset when using this form, imho.

  3. 
      
chipx86
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (cc5f3d2)
Loading...