Merge ~ilasc/launchpad:bug-1852482 into launchpad:master

Proposed by Ioana Lasc on 2019-11-15
Status: Merged
Approved by: Colin Watson on 2019-11-22
Approved revision: a290acdfa1019d1cc2e28865c7310a631f245599
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: ~ilasc/launchpad:bug-1852482
Merge into: launchpad:master
Diff against target: 107 lines (+42/-5)
3 files modified
lib/lp/code/errors.py (+21/-0)
lib/lp/code/model/gitnamespace.py (+2/-5)
lib/lp/code/model/tests/test_gitrepository.py (+19/-0)
Reviewer Review Type Date Requested Status
Colin Watson 2019-11-15 Approve on 2019-11-22
Review via email: mp+375614@code.launchpad.net

Commit message

GitRepositoryCreatorNotOwner exception was surfacing with a 500 http code instead of 400 because of non ASCII character in the name of the user 'devnull' used to invoke remote build.

To post a comment you must log in.
Colin Watson (cjwatson) wrote :

This doesn't really show the problem. You need the test to be under TestGitRepositoryWebservice, not TestGitRepositorySet; and you need to use the endpoint via a webservice caller and inspect the HTTP response. I'd expect the call to look something like:

  response = webservice.named_post("/+git", "new", ...)

... with owner and target keyword arguments both set to the api_url of a user that isn't the one the webservice caller is acting as. Compare with other tests in TestGitRepositoryWebservice.

~ilasc/launchpad:bug-1852482 updated on 2019-11-18
f61f001... by Ioana Lasc on 2019-11-18

Backed out test on TestGitRepositorySet and added test on endpoint New on the TestGitRepositoryWebservice.

Ioana Lasc (ilasc) wrote :

Thanks Colin!

I backed out out the RepoSet level test and added one on the Webservice as recommended.

I'm still confused as to why this exception surfaces as a 500 html code instead of 400.

I wonder is there another layer between Snapcraft and LP that converts the LP 400 into a Snapcraft 500 error code while passing the LP exception message ?

With the Unit Test added at Webservice I'm getting this back from the endpoint:

HTTP/1.1 400 Bad Request
Content-Length: 8
Content-Security-Policy: frame-ancestors 'self';
Content-Type: text/plain;charset=utf-8
Strict-Transport-Security: max-age=15552000
Vary: Accept
X-Content-Type-Options: nosniff
X-Frame-Options: SAMEORIGIN
X-Launchpad-Revision: bde8fcb9c6ce57fa73001a9bd2a4e51cdb0313b7
X-Lazr-Notifications: []
X-Xss-Protection: 1; mode=block

Colin Watson (cjwatson) wrote :

This test isn't right - a Snap isn't a legitimate target in the sense required by GitRepositorySet.new. Try just passing target=owner_url instead?

~ilasc/launchpad:bug-1852482 updated on 2019-11-18
eead11c... by Ioana Lasc on 2019-11-18

Pointing target to owner_url.

Ioana Lasc (ilasc) wrote :

Indeed it wasn't. I had the target pointing that way before and then I started trying to reproduce the exact behavior reported in the Bug and forgot the Snap in there.

I've now moved back but still getting 400 back instead of 500. The message below gets generated in validateRegistrant on _BaseGitNamespace by raising a GitRepositoryCreatorNotOwner as expected:

HTTP/1.1 400 Bad Request
Content-Length: 77
Content-Type: text/plain

Person-name-100279 cannot create Git repositories owned by Person-name-100277

Colin Watson (cjwatson) wrote :

You can reproduce it on production like this, if that helps? This at least proves that it can't be some strange substitution in snapcraft.

In [1]: lp.git_repositories.new(owner='/~devnull', target='/~devnull', name='foo')
[...]
ServerError: HTTP Error 500: Internal Server Error
Response headers:
---
-content-encoding: gzip
connection: close
content-length: 147
content-type: text/html;charset=utf-8
date: Mon, 18 Nov 2019 10:55:01 GMT
server: zope.server.http (HTTP)
status: 500
vary: Accept-Encoding
x-powered-by: Zope (www.zope.org), Python (www.python.org)
---
Response body:
---
<html><head><title>GitRepositoryCreatorNotOwner</title></head>
<body><h2>GitRepositoryCreatorNotOwner</h2>
A server error occurred.
</body></html>

Colin Watson (cjwatson) wrote :

A thought: the actual user https://launchpad.net/~devnull has a non-ASCII character in their name. Perhaps that makes a difference? When I try with ~ilasc rather than ~devnull I get a proper error message.

Ioana Lasc (ilasc) wrote :

Excellent, this helps further investigation, thanks Colin! I'll work with that to figure out where the 500 is stemming from.

Ioana Lasc (ilasc) wrote :

Indeed the non-ASCII character in the displayname seems to be the problem Colin.

On a quick test with the local DB Person with id=43, display name=André Luís Lopes and:
   other_user = IStore(Person).find(Person, name='andrelop').one()

unit test on the webservice revealed the same behavior we're seeing in Staging and Prod:

HTTP/1.1 500 Internal Server Error
Content-Length: 147
Content-Type: text/html;charset=utf-8

<html><head><title>GitRepositoryCreatorNotOwner</title></head>
<body><h2>GitRepositoryCreatorNotOwner</h2>
A server error occurred.
</body></html>

Will create a Person now with non-ASCII in displayname for the Unit Test and attempt to figure out where the status gets flipped to 500.

~ilasc/launchpad:bug-1852482 updated on 2019-11-20
b958192... by Ioana Lasc on 2019-11-20

Added non-ASCII character in the displayname of non owner user for Webservice Unit Test and UTF-8 encoding in creation of GitRepositoryCreatorNotOwner and GitRepositoryCreatorNotMemberOfOwnerTeam exceptions.

fb18557... by Ioana Lasc on 2019-11-20

Removed test method test_Anew_not_owner from TestGitRepositorySet - used only for debug / experimentation purposes.

ab2e14b... by Ioana Lasc on 2019-11-20

Added better name for new test method on Webservice.

f305f14... by Ioana Lasc on 2019-11-20

Cleanup after running make lint.

Ioana Lasc (ilasc) wrote :

Adding:
   1. non-ASCII character in the displayname of non owner user for Webservice Unit Test
   2. and UTF-8 encoding in creation of GitRepositoryCreatorNotOwner and GitRepositoryCreatorNotMemberOfOwnerTeam exceptions

seems to do the trick.

Now getting below output:

HTTP/1.1 400 Bad Request
Content-Length: 77
Content-Type: text/plain

Person-name-100002 cannot create Git repositories owned by André Luís Lopes

MP now ready for review / new direction suggestions.

Colin Watson (cjwatson) wrote :

Looks pretty close now, thanks! I'd just like to see a few tidy-ups, particularly for future-proofing. While we're still some way off having Launchpad running on Python 3, we should have it in mind for all new code that we write.

review: Approve
Colin Watson (cjwatson) wrote :

Also, if you could fix the commit message field of this MP, that would be appreciated (as the merge robot will use it when we get to the point of landing this). I normally put things that aren't intended to be copied into the merge commit message in the description field rather than the commit message field.

~ilasc/launchpad:bug-1852482 updated on 2019-11-22
e45c05c... by Ioana Lasc on 2019-11-22

Moved to python 3 compatible strings for GitRepositoryCreatorNotMemberOfOwnerTeam and GitRepositoryCreatorNotOwner.

a290acd... by Ioana Lasc on 2019-11-22

Fixed order of imports in errors.py.

Colin Watson (cjwatson) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/lib/lp/code/errors.py b/lib/lp/code/errors.py
2index daba338..22f6e16 100644
3--- a/lib/lp/code/errors.py
4+++ b/lib/lp/code/errors.py
5@@ -68,6 +68,7 @@ import httplib
6
7 from bzrlib.plugins.builder.recipe import RecipeParseError
8 from lazr.restful.declarations import error_status
9+import six
10
11 from lp.app.errors import (
12 NameLookupFailed,
13@@ -411,6 +412,7 @@ class GitRepositoryCreationForbidden(GitRepositoryCreationException):
14 """
15
16
17+@six.python_2_unicode_compatible
18 @error_status(httplib.BAD_REQUEST)
19 class GitRepositoryCreatorNotMemberOfOwnerTeam(GitRepositoryCreationException):
20 """Git repository creator is not a member of the owner team.
21@@ -419,7 +421,17 @@ class GitRepositoryCreatorNotMemberOfOwnerTeam(GitRepositoryCreationException):
22 owner of the repository to a team that they are not a member of.
23 """
24
25+ def __init__(self, registrant, owner):
26+ self.registrant = registrant
27+ self.owner = owner
28+
29+ def __str__(self):
30+ message = ('%s is not a member of %s'
31+ % (self.registrant.displayname, self.owner.displayname))
32+ return message
33+
34
35+@six.python_2_unicode_compatible
36 @error_status(httplib.BAD_REQUEST)
37 class GitRepositoryCreatorNotOwner(GitRepositoryCreationException):
38 """A user cannot create a Git repository belonging to another user.
39@@ -428,6 +440,15 @@ class GitRepositoryCreatorNotOwner(GitRepositoryCreationException):
40 owner of the repository to another user.
41 """
42
43+ def __init__(self, registrant, owner):
44+ self.registrant = registrant
45+ self.owner = owner
46+
47+ def __str__(self):
48+ message = ('%s cannot create Git repositories owned by %s'
49+ % (self.registrant.displayname, self.owner.displayname))
50+ return message
51+
52
53 class GitRepositoryCreationFault(Exception):
54 """Raised when there is a hosting fault creating a Git repository."""
55diff --git a/lib/lp/code/model/gitnamespace.py b/lib/lp/code/model/gitnamespace.py
56index 75ea427..4fb56e3 100644
57--- a/lib/lp/code/model/gitnamespace.py
58+++ b/lib/lp/code/model/gitnamespace.py
59@@ -165,12 +165,9 @@ class _BaseGitNamespace:
60 if not registrant.inTeam(owner):
61 if owner.is_team:
62 raise GitRepositoryCreatorNotMemberOfOwnerTeam(
63- "%s is not a member of %s"
64- % (registrant.displayname, owner.displayname))
65+ registrant, owner)
66 else:
67- raise GitRepositoryCreatorNotOwner(
68- "%s cannot create Git repositories owned by %s"
69- % (registrant.displayname, owner.displayname))
70+ raise GitRepositoryCreatorNotOwner(registrant, owner)
71
72 if not self.getAllowedInformationTypes(registrant):
73 raise GitRepositoryCreationForbidden(
74diff --git a/lib/lp/code/model/tests/test_gitrepository.py b/lib/lp/code/model/tests/test_gitrepository.py
75index 145bff4..f1a80ed 100644
76--- a/lib/lp/code/model/tests/test_gitrepository.py
77+++ b/lib/lp/code/model/tests/test_gitrepository.py
78@@ -1,3 +1,7 @@
79+# -*- coding: utf-8 -*-
80+# NOTE: The first line above must stay first; do not move the copyright
81+# notice to the top. See http://www.python.org/dev/peps/pep-0263/.
82+#
83 # Copyright 2015-2019 Canonical Ltd. This software is licensed under the
84 # GNU Affero General Public License version 3 (see the file LICENSE).
85
86@@ -3468,6 +3472,21 @@ class TestGitRepositoryWebservice(TestCaseWithFactory):
87 def test_new_person(self):
88 self.assertNewWorks(self.factory.makePerson())
89
90+ def test_new_repo_not_owner(self):
91+ non_ascii_name = u'André Luís Lopes'
92+ other_user = self.factory.makePerson(displayname=non_ascii_name)
93+ owner_url = api_url(other_user)
94+ webservice_user = self.factory.makePerson()
95+ name = "repository"
96+ webservice = webservice_for_person(
97+ webservice_user, permission=OAuthPermission.WRITE_PUBLIC)
98+ webservice.default_api_version = "devel"
99+ response = webservice.named_post(
100+ "/+git", "new", owner=owner_url, target=owner_url, name=name)
101+ self.assertEqual(400, response.status)
102+ self.assertIn(u'cannot create Git repositories owned by'
103+ u' André Luís Lopes', response.body.decode('utf-8'))
104+
105 def assertGetRepositoriesWorks(self, target_db):
106 if IPerson.providedBy(target_db):
107 owner_db = target_db

Subscribers

People subscribed via source and target branches