[Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/codeimport-git-auth into lp:launchpad

2016-10-12 Thread noreply 
The proposal to merge lp:~cjwatson/launchpad/codeimport-git-auth into 
lp:launchpad has been updated.

Status: Needs review => Merged

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/codeimport-git-auth/+merge/307968
-- 
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help   : https://help.launchpad.net/ListHelp

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/codeimport-git-auth into lp:launchpad

2016-10-12 Thread Colin Watson 


Diff comments:

> 
> === modified file 'lib/lp/code/model/codeimportjob.py'
> --- lib/lp/code/model/codeimportjob.py2016-10-03 17:00:56 +
> +++ lib/lp/code/model/codeimportjob.py2016-10-07 15:56:06 +
> @@ -340,3 +348,57 @@
>  # 4)
>  getUtility(ICodeImportEventSet).newReclaim(
>  code_import, machine, job_id)
> +
> +
> +@implementer(IMacaroonIssuer)
> +class CodeImportJobMacaroonIssuer:
> +
> +@property
> +def _root_secret(self):
> +secret = config.codeimport.macaroon_secret_key
> +if not secret:
> +raise RuntimeError(
> +"codeimport.macaroon_secret_key not configured.")
> +return secret
> +
> +def issueMacaroon(self, context):
> +"""See `IMacaroonIssuer`."""
> +assert context.code_import.git_repository is not None
> +macaroon = Macaroon(
> +location=config.vhost.mainsite.hostname,
> +identifier="code-import-job", key=self._root_secret)
> +macaroon.add_first_party_caveat("code-import-job %s" % context.id)
> +return macaroon
> +
> +def checkMacaroonIssuer(self, macaroon):
> +"""See `IMacaroonIssuer`."""
> +try:
> +verifier = Verifier()
> +verifier.satisfy_general(
> +lambda caveat: caveat.startswith("code-import-job "))
> +return verifier.verify(macaroon, self._root_secret)
> +except Exception:
> +return False
> +
> +def verifyMacaroon(self, macaroon, context):
> +"""See `IMacaroonIssuer`."""
> +if IGitRepository.providedBy(context):
> +if context.repository_type != GitRepositoryType.IMPORTED:
> +return False
> +code_import = getUtility(ICodeImportSet).getByGitRepository(
> +context)
> +if code_import is None:
> +return False
> +job = code_import.import_job
> +if job is None:
> +return False
> +else:
> +job = context

I agree it's odd, but on balance I prefer the opposite resolution from the one 
I think you're suggesting: I've moved the GitRepository -> CodeImportJob lookup 
out to the caller in GitAPI instead.  That way, CodeImportJobMacaroonIssuer 
operates on CodeImportJob objects and GitAPI handles the specifics of 
GitRepository objects, which makes a lot more sense to me.

> +try:
> +verifier = Verifier()
> +verifier.satisfy_exact("code-import-job %s" % job.id)
> +return (
> +verifier.verify(macaroon, self._root_secret) and
> +job.state == CodeImportJobState.RUNNING)
> +except Exception:
> +return False


-- 
https://code.launchpad.net/~cjwatson/launchpad/codeimport-git-auth/+merge/307968
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help   : https://help.launchpad.net/ListHelp

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/codeimport-git-auth into lp:launchpad

2016-10-12 Thread William Grant 
Review: Approve code



Diff comments:

> 
> === modified file 'lib/lp/code/model/codeimportjob.py'
> --- lib/lp/code/model/codeimportjob.py2016-10-03 17:00:56 +
> +++ lib/lp/code/model/codeimportjob.py2016-10-07 15:56:06 +
> @@ -340,3 +348,57 @@
>  # 4)
>  getUtility(ICodeImportEventSet).newReclaim(
>  code_import, machine, job_id)
> +
> +
> +@implementer(IMacaroonIssuer)
> +class CodeImportJobMacaroonIssuer:
> +
> +@property
> +def _root_secret(self):
> +secret = config.codeimport.macaroon_secret_key
> +if not secret:
> +raise RuntimeError(
> +"codeimport.macaroon_secret_key not configured.")
> +return secret
> +
> +def issueMacaroon(self, context):
> +"""See `IMacaroonIssuer`."""
> +assert context.code_import.git_repository is not None
> +macaroon = Macaroon(
> +location=config.vhost.mainsite.hostname,
> +identifier="code-import-job", key=self._root_secret)
> +macaroon.add_first_party_caveat("code-import-job %s" % context.id)
> +return macaroon
> +
> +def checkMacaroonIssuer(self, macaroon):
> +"""See `IMacaroonIssuer`."""
> +try:
> +verifier = Verifier()
> +verifier.satisfy_general(
> +lambda caveat: caveat.startswith("code-import-job "))
> +return verifier.verify(macaroon, self._root_secret)

We should probably also check that the location matches.

It's also slightly confusing that verifyMacaroon doesn't call 
checkMacaroonIssuer -- seems like a recipe for mistakes later.

> +except Exception:
> +return False
> +
> +def verifyMacaroon(self, macaroon, context):
> +"""See `IMacaroonIssuer`."""
> +if IGitRepository.providedBy(context):
> +if context.repository_type != GitRepositoryType.IMPORTED:
> +return False
> +code_import = getUtility(ICodeImportSet).getByGitRepository(
> +context)
> +if code_import is None:
> +return False
> +job = code_import.import_job
> +if job is None:
> +return False
> +else:
> +job = context

This case is only used in tests. Would it be better to refactor this slightly 
so we didn't have the weird unused alternate case and tests that don't test the 
normal path?

> +try:
> +verifier = Verifier()
> +verifier.satisfy_exact("code-import-job %s" % job.id)
> +return (
> +verifier.verify(macaroon, self._root_secret) and
> +job.state == CodeImportJobState.RUNNING)
> +except Exception:
> +return False

Cunning. Undebuggable and fragile, but it'll work for now and hopefully nobody 
will touch it in inappropriate ways before we have a more generic mechanism.

> 
> === modified file 'lib/lp/code/xmlrpc/git.py'
> --- lib/lp/code/xmlrpc/git.py 2016-10-06 20:37:39 +
> +++ lib/lp/code/xmlrpc/git.py 2016-10-07 15:56:06 +
> @@ -68,22 +73,47 @@
>  super(GitAPI, self).__init__(*args, **kwargs)
>  self.repository_set = getUtility(IGitRepositorySet)
>  
> -def _performLookup(self, path):
> +def _verifyMacaroon(self, macaroon_raw, repository=None):
> +try:
> +macaroon = Macaroon.deserialize(macaroon_raw)
> +except Exception:
> +return False
> +try:
> +issuer = getUtility(IMacaroonIssuer, macaroon.identifier)
> +except ComponentLookupError:
> +return False
> +if repository is not None:
> +return issuer.verifyMacaroon(macaroon, repository)
> +else:
> +return issuer.checkMacaroonIssuer(macaroon)
> +
> +def _performLookup(self, requester, path, auth_params):

The new "requester" parameter seems unused.

>  repository, extra_path = getUtility(IGitLookup).getByPath(path)
>  if repository is None:
>  return None
> -try:
> -hosting_path = repository.getInternalPath()
> -except Unauthorized:
> -return None
> -writable = (
> -repository.repository_type == GitRepositoryType.HOSTED and
> -check_permission("launchpad.Edit", repository))
> +macaroon_raw = auth_params.get("macaroon")
> +naked_repository = removeSecurityProxy(repository)
> +if (macaroon_raw is not None and
> +self._verifyMacaroon(macaroon_raw, naked_repository)):
> +# The authentication parameters specifically grant access to
> +# this repository, so we can bypass other checks.
> +hosting_path = naked_repository.getInternalPath()
> +writable = True
> +private = naked_repository.private

I wouldn't half mind a quick assertion here that repository_type == IMPORTED. 
Makes the i

[Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/codeimport-git-auth into lp:launchpad

2016-10-07 Thread Colin Watson 
Colin Watson has proposed merging lp:~cjwatson/launchpad/codeimport-git-auth 
into lp:launchpad.

Commit message:
Allow pushing to Git repositories associated with running code import jobs 
using macaroon credentials.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1469459 in Launchpad itself: "import external code into a LP git repo 
(natively)"
  https://bugs.launchpad.net/launchpad/+bug/1469459

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/codeimport-git-auth/+merge/307968

Allow pushing to Git repositories associated with running code import jobs 
using macaroon credentials.

We don't actually issue suitable macaroons as yet outside the test suite; that 
will come in a later branch.

I've tried to design the IMacaroonIssuer interface so that it can be useful for 
some other similar applications (e.g. granting access tokens to builders), but 
it's provisional and I entirely expect it to need to change when it's actually 
battle-tested for that kind of thing.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of 
lp:~cjwatson/launchpad/codeimport-git-auth into lp:launchpad.
=== modified file 'lib/lp/code/configure.zcml'
--- lib/lp/code/configure.zcml	2016-10-03 17:00:56 +
+++ lib/lp/code/configure.zcml	2016-10-07 15:56:06 +
@@ -729,6 +729,15 @@
 reclaimJob"/>
   
 
+  
+
+  
+
+  
+
   
 
   

=== modified file 'lib/lp/code/model/codeimportjob.py'
--- lib/lp/code/model/codeimportjob.py	2016-10-03 17:00:56 +
+++ lib/lp/code/model/codeimportjob.py	2016-10-07 15:56:06 +
@@ -12,6 +12,10 @@
 
 import datetime
 
+from pymacaroons import (
+Macaroon,
+Verifier,
+)
 from sqlobject import (
 ForeignKey,
 IntCol,
@@ -27,7 +31,9 @@
 CodeImportMachineState,
 CodeImportResultStatus,
 CodeImportReviewStatus,
+GitRepositoryType,
 )
+from lp.code.interfaces.codeimport import ICodeImportSet
 from lp.code.interfaces.codeimportevent import ICodeImportEventSet
 from lp.code.interfaces.codeimportjob import (
 ICodeImportJob,
@@ -37,6 +43,7 @@
 )
 from lp.code.interfaces.codeimportmachine import ICodeImportMachineSet
 from lp.code.interfaces.codeimportresult import ICodeImportResultSet
+from lp.code.interfaces.gitrepository import IGitRepository
 from lp.code.model.codeimportresult import CodeImportResult
 from lp.registry.interfaces.person import validate_public_person
 from lp.services.config import config
@@ -48,6 +55,7 @@
 SQLBase,
 sqlvalues,
 )
+from lp.services.macaroons.interfaces import IMacaroonIssuer
 
 
 @implementer(ICodeImportJob)
@@ -340,3 +348,57 @@
 # 4)
 getUtility(ICodeImportEventSet).newReclaim(
 code_import, machine, job_id)
+
+
+@implementer(IMacaroonIssuer)
+class CodeImportJobMacaroonIssuer:
+
+@property
+def _root_secret(self):
+secret = config.codeimport.macaroon_secret_key
+if not secret:
+raise RuntimeError(
+"codeimport.macaroon_secret_key not configured.")
+return secret
+
+def issueMacaroon(self, context):
+"""See `IMacaroonIssuer`."""
+assert context.code_import.git_repository is not None
+macaroon = Macaroon(
+location=config.vhost.mainsite.hostname,
+identifier="code-import-job", key=self._root_secret)
+macaroon.add_first_party_caveat("code-import-job %s" % context.id)
+return macaroon
+
+def checkMacaroonIssuer(self, macaroon):
+"""See `IMacaroonIssuer`."""
+try:
+verifier = Verifier()
+verifier.satisfy_general(
+lambda caveat: caveat.startswith("code-import-job "))
+return verifier.verify(macaroon, self._root_secret)
+except Exception:
+return False
+
+def verifyMacaroon(self, macaroon, context):
+"""See `IMacaroonIssuer`."""
+if IGitRepository.providedBy(context):
+if context.repository_type != GitRepositoryType.IMPORTED:
+return False
+code_import = getUtility(ICodeImportSet).getByGitRepository(
+context)
+if code_import is None:
+return False
+job = code_import.import_job
+if job is None:
+return False
+else:
+job = context
+try:
+verifier = Verifier()
+verifier.satisfy_exact("code-import-job %s" % job.id)
+return (
+verifier.verify(macaroon, self._root_secret) and
+job.state == CodeImportJobState.RUNNING)
+except Exception:
+return False

=== modified file 'lib/lp/code/model/tests/test_codeimportjob.py'
--- lib/lp/code/model/tests/test_codeimportjob.py	2015-10-19 10:56:16 +
+++ lib/lp/code/model/tests/test_codeimportjob.py	2016-10-07 15:56:06 +
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Cano