Merge ~ilasc/launchpad:bug-1852482 into launchpad: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
diff --git a/lib/lp/code/errors.py b/lib/lp/code/errors.py
index daba338..22f6e16 100644
--- a/lib/lp/code/errors.py
+++ b/lib/lp/code/errors.py
@@ -68,6 +68,7 @@ import httplib
6868
69from bzrlib.plugins.builder.recipe import RecipeParseError69from bzrlib.plugins.builder.recipe import RecipeParseError
70from lazr.restful.declarations import error_status70from lazr.restful.declarations import error_status
71import six
7172
72from lp.app.errors import (73from lp.app.errors import (
73 NameLookupFailed,74 NameLookupFailed,
@@ -411,6 +412,7 @@ class GitRepositoryCreationForbidden(GitRepositoryCreationException):
411 """412 """
412413
413414
415@six.python_2_unicode_compatible
414@error_status(httplib.BAD_REQUEST)416@error_status(httplib.BAD_REQUEST)
415class GitRepositoryCreatorNotMemberOfOwnerTeam(GitRepositoryCreationException):417class GitRepositoryCreatorNotMemberOfOwnerTeam(GitRepositoryCreationException):
416 """Git repository creator is not a member of the owner team.418 """Git repository creator is not a member of the owner team.
@@ -419,7 +421,17 @@ class GitRepositoryCreatorNotMemberOfOwnerTeam(GitRepositoryCreationException):
419 owner of the repository to a team that they are not a member of.421 owner of the repository to a team that they are not a member of.
420 """422 """
421423
424 def __init__(self, registrant, owner):
425 self.registrant = registrant
426 self.owner = owner
427
428 def __str__(self):
429 message = ('%s is not a member of %s'
430 % (self.registrant.displayname, self.owner.displayname))
431 return message
432
422433
434@six.python_2_unicode_compatible
423@error_status(httplib.BAD_REQUEST)435@error_status(httplib.BAD_REQUEST)
424class GitRepositoryCreatorNotOwner(GitRepositoryCreationException):436class GitRepositoryCreatorNotOwner(GitRepositoryCreationException):
425 """A user cannot create a Git repository belonging to another user.437 """A user cannot create a Git repository belonging to another user.
@@ -428,6 +440,15 @@ class GitRepositoryCreatorNotOwner(GitRepositoryCreationException):
428 owner of the repository to another user.440 owner of the repository to another user.
429 """441 """
430442
443 def __init__(self, registrant, owner):
444 self.registrant = registrant
445 self.owner = owner
446
447 def __str__(self):
448 message = ('%s cannot create Git repositories owned by %s'
449 % (self.registrant.displayname, self.owner.displayname))
450 return message
451
431452
432class GitRepositoryCreationFault(Exception):453class GitRepositoryCreationFault(Exception):
433 """Raised when there is a hosting fault creating a Git repository."""454 """Raised when there is a hosting fault creating a Git repository."""
diff --git a/lib/lp/code/model/gitnamespace.py b/lib/lp/code/model/gitnamespace.py
index 75ea427..4fb56e3 100644
--- a/lib/lp/code/model/gitnamespace.py
+++ b/lib/lp/code/model/gitnamespace.py
@@ -165,12 +165,9 @@ class _BaseGitNamespace:
165 if not registrant.inTeam(owner):165 if not registrant.inTeam(owner):
166 if owner.is_team:166 if owner.is_team:
167 raise GitRepositoryCreatorNotMemberOfOwnerTeam(167 raise GitRepositoryCreatorNotMemberOfOwnerTeam(
168 "%s is not a member of %s"168 registrant, owner)
169 % (registrant.displayname, owner.displayname))
170 else:169 else:
171 raise GitRepositoryCreatorNotOwner(170 raise GitRepositoryCreatorNotOwner(registrant, owner)
172 "%s cannot create Git repositories owned by %s"
173 % (registrant.displayname, owner.displayname))
174171
175 if not self.getAllowedInformationTypes(registrant):172 if not self.getAllowedInformationTypes(registrant):
176 raise GitRepositoryCreationForbidden(173 raise GitRepositoryCreationForbidden(
diff --git a/lib/lp/code/model/tests/test_gitrepository.py b/lib/lp/code/model/tests/test_gitrepository.py
index 145bff4..f1a80ed 100644
--- a/lib/lp/code/model/tests/test_gitrepository.py
+++ b/lib/lp/code/model/tests/test_gitrepository.py
@@ -1,3 +1,7 @@
1# -*- coding: utf-8 -*-
2# NOTE: The first line above must stay first; do not move the copyright
3# notice to the top. See http://www.python.org/dev/peps/pep-0263/.
4#
1# Copyright 2015-2019 Canonical Ltd. This software is licensed under the5# Copyright 2015-2019 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).6# GNU Affero General Public License version 3 (see the file LICENSE).
37
@@ -3468,6 +3472,21 @@ class TestGitRepositoryWebservice(TestCaseWithFactory):
3468 def test_new_person(self):3472 def test_new_person(self):
3469 self.assertNewWorks(self.factory.makePerson())3473 self.assertNewWorks(self.factory.makePerson())
34703474
3475 def test_new_repo_not_owner(self):
3476 non_ascii_name = u'André Luís Lopes'
3477 other_user = self.factory.makePerson(displayname=non_ascii_name)
3478 owner_url = api_url(other_user)
3479 webservice_user = self.factory.makePerson()
3480 name = "repository"
3481 webservice = webservice_for_person(
3482 webservice_user, permission=OAuthPermission.WRITE_PUBLIC)
3483 webservice.default_api_version = "devel"
3484 response = webservice.named_post(
3485 "/+git", "new", owner=owner_url, target=owner_url, name=name)
3486 self.assertEqual(400, response.status)
3487 self.assertIn(u'cannot create Git repositories owned by'
3488 u' André Luís Lopes', response.body.decode('utf-8'))
3489
3471 def assertGetRepositoriesWorks(self, target_db):3490 def assertGetRepositoriesWorks(self, target_db):
3472 if IPerson.providedBy(target_db):3491 if IPerson.providedBy(target_db):
3473 owner_db = target_db3492 owner_db = target_db

Subscribers

People subscribed via source and target branches

to status/vote changes: