Ensure SSH is given a valid port number.

Review Request #7619 — Created Sept. 8, 2015 and submitted

Information

jwu
Review Board
release-2.0.x

Reviewers

When an invalid port is given in a repository path, a cryptic error message was shown.

The change provides a better error message to the user when an invalid port is in the repository path.

Ran unit tests.

Manually verified that the error message was displayed upon replication.

Description From Last Updated

This doesn't need to be in the try does it?

brenniebrennie

What happens if this doesn't pass a list. Also, there is trailing whitespace and this should end with a period.

brenniebrennie

SSHError isn't really appropriate in this case because its an error in parsing, not with SSH itself. Can we add …

brenniebrennie

Can this include the port number? Also, we use single quotes for strings.

brenniebrennie

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 17 W503 line break before binary operator

reviewbotreviewbot

This isn't going to work quite correctly, because whatever is passed to _() needs to be a constant. How about …

daviddavid

Col: 17 E128 continuation line under-indented for visual indent

reviewbotreviewbot

I'd call this SSHInvalidPortError, for consistency with the other classes.

chipx86chipx86

Blank line between these.

chipx86chipx86

For docstrings, you should use double quotes. """

chipx86chipx86

You can combine these into one statement: You should also use super(...) for calling the parent constructor. The other classes …

chipx86chipx86

Make sure these are imported in alphabetical order.

chipx86chipx86

Blank line between these. We always have a blank line between statements and blocks, or blocks and blocks on the …

chipx86chipx86

"path" -> "port"

mike_conleymike_conley

Elsewhere in this file, instead of using super(class), we seem to directly call into SSHError.__init__. Do we know why that …

mike_conleymike_conley

You don't need this blank line, since the block is ending.

chipx86chipx86
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/scmtools/forms.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/scmtools/forms.py
    
    
  2. 
      
JW
JW
brennie
  1. 
      
    1. Please make sure to fill out the testing done with how you tested. E.g.,

      Ran unit tests.
      
      Manually verified....
      
  2. reviewboard/scmtools/forms.py (Diff revision 1)
     
     
    Show all issues

    This doesn't need to be in the try does it?

    1. Ah, so it doesn’t. I wasn’t aware that URLValidator could take a message argument to change what message would be displayed; so my initial attempt was to use the except to change the error message.

  3. reviewboard/scmtools/forms.py (Diff revision 1)
     
     
    Show all issues

    What happens if this doesn't pass a list.

    Also, there is trailing whitespace and this should end with a period.

    1. I think the list is a way to be able to show multiple errors but I might be wrong. Couldn’t find much information about it. If you don’t pass a list in the code it still works and appears to prints normally.

    2. In that case, we don't really need to use a list here. It's cleaner if we don't use one.

  4. 
      
chipx86
  1. I appreciate the change, but we actually don't validate URLs in the form for a very good reason: Repository paths are not always URLs.

    Perforce uses paths like myservername:1666.

    CVS uses paths like :pserver:username@servername:/cvsroot/foo

    Git uses names like git@domain:/path

    Even when they are URLs, sometimes they're not fully-qualified domains (so https://myserver/, rather than https://myserver.mycompany.com/), which won't pass the validator.

    Repository path validation would need to be done in each SCMTool instead, per their own validation rules.

    Right now, we already have a check_repository() function that SCMTool subclasses can implement, which checks to make sure the URL is accessible. This is where validation logic should go. It should ensure that the path passed in matches expectations, and raise a suitable error (perhaps a new RepositoryPathValidationError that subclasses SCMError). The form and API code would need to know to handle this new error.

    1. I see. I’ll abandon the URL Validation here then.

      I did some more digging and found that the error being given was a ValueError that gets raised in ssh/utils.py - check_host() function. Specifically when this piece of code gets run:

              if ':' in netloc:
                  hostname, port = netloc.split(':')
                  port = int(port)
      

      Guessing that if ‘:’ appeared later on in an ssh address that it would have been assumed to be the port number? If that’s the case, could we write an except for the ValueError in scmtools/forms.py - _verify_repository_path()? I’ve tested it out and it seems to work catch the ValueError, but I’m not sure if a ValueError would pop up anywhere else during the repo path verification process.

    2. Since different repository types have very different ideas of what's in the Path field, the form shouldn't be doing any validation or ValueError catching, since it doesn't know enough to know what's correct and what's not, nor should it mask or take the fall for broken code inside a SCMTool or the SSH backend.

      What does the repository path that's failing look like in the form, and what does it look like when it gets to that part of check_host()?

    3. If that's the case then should the error be raised in the ssh/utils.py file itself? Or if only one colon is expected in the ssh url could we check for that?

      Also, I copied the bug error path so the repository path in the form is: ssh://xxx.oracle.com://hgroot/ and when it reaches check_host() the values of variables of interest are:

      netloc = 'xxx.oracle.com:'
      port = ''
      
    4. That URL is malformed. The correct url is ssh://xxx.oracle.com/hgroot/ (that part was user error). This bug is about providing a better error messages when repository paths fail validation.

    5. I'm aware the URL is malformed. I'm wondering if for this specific bug, the error is an SCMTool error or an SSH error?

  2. 
      
JW
JW
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/ssh/utils.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/ssh/utils.py
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/ssh/utils.py (Diff revision 2)
     
     
    Show all issues

    Can this include the port number?

    Also, we use single quotes for strings.

  3. 
      
brennie
  1. Your description and summary need some work. You may want to give this guide on writing good descriptions a read.

    In general, the summary and description should provide information on why the change was necessary.

  2. reviewboard/ssh/utils.py (Diff revision 2)
     
     
    Show all issues

    SSHError isn't really appropriate in this case because its an error in parsing, not with SSH itself.

    Can we add a new error class somewhere that describes a parsing error?

    1. After looking through the current error.py files I couldn't find an appropriate one to place the error class. I also looked through the Django and Python exceptions and couldn't find one that seemed to fit.

      So I'm a bit unsure on where to place this new error class.

    2. It can go in ssh/errors.py

  3. 
      
JW
JW
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/ssh/utils.py
        reviewboard/ssh/errors.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/ssh/utils.py
        reviewboard/ssh/errors.py
    
    
  2. reviewboard/ssh/errors.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  3. reviewboard/ssh/errors.py (Diff revision 3)
     
     
    Show all issues
    Col: 17
     W503 line break before binary operator
    
  4. 
      
JW
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/ssh/utils.py
        reviewboard/ssh/errors.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/ssh/utils.py
        reviewboard/ssh/errors.py
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/ssh/errors.py (Diff revision 4)
     
     
     
    Show all issues

    This isn't going to work quite correctly, because whatever is passed to _() needs to be a constant. How about something like this:

    msg = (_('"%s" is not a valid port number. Please ensure '
             'the path has a correct port number specified.')
           % port)
    
  3. 
      
JW
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/ssh/utils.py
        reviewboard/ssh/errors.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/ssh/utils.py
        reviewboard/ssh/errors.py
    
    
  2. reviewboard/ssh/errors.py (Diff revision 5)
     
     
    Show all issues
    Col: 17
     E128 continuation line under-indented for visual indent
    
  3. 
      
JW
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/ssh/utils.py
        reviewboard/ssh/errors.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/ssh/utils.py
        reviewboard/ssh/errors.py
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/ssh/errors.py (Diff revision 6)
     
     
    Show all issues

    I'd call this SSHInvalidPortError, for consistency with the other classes.

  3. reviewboard/ssh/errors.py (Diff revision 6)
     
     
     
    Show all issues

    Blank line between these.

  4. reviewboard/ssh/errors.py (Diff revision 6)
     
     
    Show all issues

    For docstrings, you should use double quotes. """

  5. reviewboard/ssh/errors.py (Diff revision 6)
     
     
     
     
    Show all issues

    You can combine these into one statement:

    You should also use super(...) for calling the parent constructor. The other classes don't do this, but only because they used to have to support versions of Python where super() couldn't be used with exceptions.

    End result should be like:

    super(InvalidSSHPortError, self).__init__(
        self,
        _('......'
          '......')
        % port)
    

    Note two things about this:

    1. When dealing with long parameters like this, we're doing one parameter per line.
    2. The % var goes on its own line, aligned with th thing it's formatting, when dealing with wrapped strings.
  6. reviewboard/ssh/utils.py (Diff revision 6)
     
     
     
    Show all issues

    Make sure these are imported in alphabetical order.

  7. reviewboard/ssh/utils.py (Diff revision 6)
     
     
     
    Show all issues

    Blank line between these. We always have a blank line between statements and blocks, or blocks and blocks on the same level.

  8. 
      
JW
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/ssh/utils.py
        reviewboard/ssh/errors.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/ssh/utils.py
        reviewboard/ssh/errors.py
    
    
  2. 
      
mike_conley
  1. 
      
  2. reviewboard/ssh/errors.py (Diff revision 7)
     
     
    Show all issues

    "path" -> "port"

  3. reviewboard/ssh/errors.py (Diff revision 7)
     
     
    Show all issues

    Elsewhere in this file, instead of using super(class), we seem to directly call into SSHError.__init__.

    Do we know why that is?

    1. According to Christian:

      "You should also use super(...) for calling the parent constructor. The other classes don't do this, but only because they used to have to support versions of Python where super() couldn't be used with exceptions."

    2. Ah, okay! Thanks!

  4. 
      
JW
JW
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/ssh/utils.py
        reviewboard/ssh/errors.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/ssh/utils.py
        reviewboard/ssh/errors.py
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/ssh/utils.py (Diff revision 8)
     
     
     
     
    Show all issues

    You don't need this blank line, since the block is ending.

  3. 
      
JW
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/ssh/utils.py
        reviewboard/ssh/errors.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/ssh/utils.py
        reviewboard/ssh/errors.py
    
    
  2. 
      
JW
Review request changed
Status:
Completed
Change Summary:
Pushed to 2.0.x (214465c)