Launchpad itself

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

  1. Git
  2. lp:~ilasc/launchpad
  3. bug-1852482
  4. Merge into master
Proposed by Ioana Lasc
Status: Merged
Approved by: Colin Watson
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 (community) Approve
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.
Revision history for this message
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
f61f001... by Ioana Lasc

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

Revision history for this message
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

Revision history for this message
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
eead11c... by Ioana Lasc

Pointing target to owner_url.

Revision history for this message
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

Revision history for this message
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>

Revision history for this message
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.

Revision history for this 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.

Revision history for this message
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
b958192... by Ioana Lasc

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

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

ab2e14b... by Ioana Lasc

Added better name for new test method on Webservice.

f305f14... by Ioana Lasc

Cleanup after running make lint.

Revision history for this message
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.

Revision history for this message
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
Revision history for this message
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
e45c05c... by Ioana Lasc

Moved to python 3 compatible strings for GitRepositoryCreatorNotMemberOfOwnerTeam and GitRepositoryCreatorNotOwner.

a290acd... by Ioana Lasc

Fixed order of imports in errors.py.

Revision history for this message
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

to status/vote changes: