Summary

Ensure SSH is given a valid port number.

Review Request #7619 — Created Sept. 8, 2015 and submitted Sept. 19, 2015, 1:49 p.m.

Information

Owner

jwu

Repository

Review Board

Branch

release-2.0.x

Bugs

3891

Depends On

Reviewers

Groups

reviewboard, students

People

Description

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.

Testing Done

Ran unit tests.

Manually verified that the error message was displayed upon replication.

Issues

Description	From	Last Updated
This doesn't need to be in the try does it?	brennie	Sept. 9, 2015, 8:38 a.m.
What happens if this doesn't pass a list. Also, there is trailing whitespace and this should end with a period.	brennie	Sept. 10, 2015, 5:26 a.m.
SSHError isn't really appropriate in this case because its an error in parsing, not with SSH itself. Can we add …	brennie	Sept. 16, 2015, 6:14 a.m.
Can this include the port number? Also, we use single quotes for strings.	brennie	Sept. 16, 2015, 6:14 a.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Sept. 16, 2015, 6:19 a.m.
Col: 17 W503 line break before binary operator	reviewbot	Sept. 16, 2015, 6:19 a.m.
This isn't going to work quite correctly, because whatever is passed to _() needs to be a constant. How about …	david	Sept. 18, 2015, 1:53 p.m.
Col: 17 E128 continuation line under-indented for visual indent	reviewbot	Sept. 18, 2015, 1:56 p.m.
I'd call this SSHInvalidPortError, for consistency with the other classes.	chipx86	Sept. 18, 2015, 4:51 p.m.
Blank line between these.	chipx86	Sept. 18, 2015, 4:51 p.m.
For docstrings, you should use double quotes. """	chipx86	Sept. 18, 2015, 4:51 p.m.
You can combine these into one statement: You should also use super(...) for calling the parent constructor. The other classes …	chipx86	Sept. 18, 2015, 4:51 p.m.
Make sure these are imported in alphabetical order.	chipx86	Sept. 18, 2015, 4:51 p.m.
Blank line between these. We always have a blank line between statements and blocks, or blocks and blocks on the …	chipx86	Sept. 18, 2015, 4:51 p.m.
"path" -> "port"	mike_conley	Sept. 19, 2015, 11:58 a.m.
Elsewhere in this file, instead of using super(class), we seem to directly call into SSHError.__init__. Do we know why that …	mike_conley	Sept. 19, 2015, 11:59 a.m.
You don't need this blank line, since the block is ending.	chipx86	Sept. 19, 2015, 1:43 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/scmtools/forms.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/scmtools/forms.py

Bugs:

+	bug3891

Bugs:

-	bug3891
+	3891

brennie Sept. 8, 2015, 10:36 a.m.

Please make sure to fill out the testing done with how you tested. E.g.,

Ran unit tests.

Manually verified....

reviewboard/scmtools/forms.py (Diff revision 1)

The issue has been resolved. Show all issues

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

jwu Sept. 8, 2015, 8:36 p.m.

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.

reviewboard/scmtools/forms.py (Diff revision 1)

The issue has been resolved. Show all issues

What happens if this doesn't pass a list.

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

jwu Sept. 8, 2015, 8:36 p.m.

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.

brennie Sept. 9, 2015, 4:12 p.m.

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

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.

jwu Sept. 8, 2015, 8:35 p.m.

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.

chipx86 Sept. 9, 2015, 1:44 a.m.

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()?

jwu Sept. 9, 2015, 5:42 a.m.

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 = ''

brennie Sept. 10, 2015, 12:53 p.m.

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.

jwu Sept. 10, 2015, 5:50 p.m.

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?

Testing Done:

	+	Ran unit tests.
	+
	+	Manually verified that the error message was displayed upon replication.

Summary:

-	Add URLValidation to form.py to validate repo's path
+	Add try/except block in ssh/utils.py to catch empty port

Description:

~		Add URLValidation to form.py to validate repo's path
	~	Added a try/except block to catch a Value error that occurs when ssh/utils.py check_host() gets an empty port string.
	+
	+	The original bug, was caused by the user entering in an incorrect path, but I feel the error message should better reflect what was causing the actual error, which in this case was using int() on an empty string. I worded the error in a way that would hopefully cause the reader to double check their path even if they weren't intending to put in a port number (which was the case for bug3891).
	+
	+	I was unsure if a new error in ssh/errors.py would be justified, so I didn't create one.

Branch:

-	master
+	release-2.0.x

Diff:

Revision 2 (+4 -1)

Show changes

reviewboard/ssh/utils.py

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/ssh/utils.py



Tool: Pyflakes
Processed Files:
    reviewboard/ssh/utils.py

reviewboard/ssh/utils.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Can this include the port number?
Also, we use single quotes for strings.
```

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.

reviewboard/ssh/utils.py (Diff revision 2)

The issue has been resolved. 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?

jwu Sept. 15, 2015, 1:29 p.m.

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.

brennie Sept. 15, 2015, 9:41 p.m.
```
It can go in ssh/errors.py
```

Summary:

-	Add try/except block in ssh/utils.py to catch empty port
+	Ensure SSH is given a valid port number.

Description:

~		Added a try/except block to catch a Value error that occurs when ssh/utils.py check_host() gets an empty port string.
	~	When an invalid port is given in a repository path, a cryptic error message was shown.

~		The original bug, was caused by the user entering in an incorrect path, but I feel the error message should better reflect what was causing the actual error, which in this case was using int() on an empty string. I worded the error in a way that would hopefully cause the reader to double check their path even if they weren't intending to put in a port number (which was the case for bug3891).
	~	The change provides a better error message to the user when an invalid port is in the repository path.
-
-		I was unsure if a new error in ssh/errors.py would be justified, so I didn't create one.

Diff:

Revision 3 (+12 -2)

Show changes

	reviewboard/ssh/errors.py
	reviewboard/ssh/utils.py

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

reviewboard/ssh/errors.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
reviewboard/ssh/errors.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 17
 W503 line break before binary operator
```

Diff:

Revision 4 (+13 -2)

Show changes

	reviewboard/ssh/errors.py
	reviewboard/ssh/utils.py

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

reviewboard/ssh/errors.py (Diff revision 4)

The issue has been resolved. 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)

Diff:

Revision 5 (+13 -2)

Show changes

	reviewboard/ssh/errors.py
	reviewboard/ssh/utils.py

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

reviewboard/ssh/errors.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 17
 E128 continuation line under-indented for visual indent
```

Diff:

Revision 6 (+13 -2)

Show changes

	reviewboard/ssh/errors.py
	reviewboard/ssh/utils.py

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

reviewboard/ssh/errors.py (Diff revision 6)
The issue has been resolved. Show all issues
```
I'd call this SSHInvalidPortError, for consistency with the other classes.
```
reviewboard/ssh/errors.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Blank line between these.
```
reviewboard/ssh/errors.py (Diff revision 6)
The issue has been resolved. Show all issues
```
For docstrings, you should use double quotes. """
```

reviewboard/ssh/errors.py (Diff revision 6)

The issue has been resolved. 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:

When dealing with long parameters like this, we're doing one parameter per line.
The % var goes on its own line, aligned with th thing it's formatting, when dealing with wrapped strings.

reviewboard/ssh/utils.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Make sure these are imported in alphabetical order.
```

reviewboard/ssh/utils.py (Diff revision 6)

The issue has been resolved. 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.

Diff:

Revision 7 (+17 -2)

Show changes

	reviewboard/ssh/errors.py
	reviewboard/ssh/utils.py

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

Ship it!

reviewboard/ssh/errors.py (Diff revision 7)
The issue has been resolved. Show all issues
```
"path" -> "port"
```

reviewboard/ssh/errors.py (Diff revision 7)

The issue has been resolved. 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?

jwu Sept. 19, 2015, 11:59 a.m.

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

mike_conley Sept. 19, 2015, 12:15 p.m.
```
Ah, okay! Thanks!
```

Groups:

+	students

Diff:

Revision 8 (+17 -2)

Show changes

	reviewboard/ssh/errors.py
	reviewboard/ssh/utils.py

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

Ship it!

reviewboard/ssh/utils.py (Diff revision 8)
The issue has been resolved. Show all issues
```
You don't need this blank line, since the block is ending.
```

Diff:

Revision 9 (+16 -2)

Show changes

	reviewboard/ssh/errors.py
	reviewboard/ssh/utils.py

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

Status: Closed (submitted)

Change Summary:

Pushed to 2.0.x (214465c)