launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> 
> === modified file 'lib/lp/archiveuploader/dscfile.py'
> --- lib/lp/archiveuploader/dscfile.py	2016-06-01 01:59:32 +0000
> +++ lib/lp/archiveuploader/dscfile.py	2016-11-17 17:09:06 +0000
> @@ -144,6 +144,9 @@
>          if verify_signature:
>              self.signingkey, self.parsed_content = self.verifySignature(
>                  self.raw_content, self.filepath)
> +            if self.signingkey.active == False:

Any reason not to spell this as "if not self.signingkey.active" now?

> +                raise UploadError("File %s is signed with a deactivated key %s"
> +                                  % (self.filepath, self.signingkey.keyid))
>          else:
>              self.logger.debug("%s can be unsigned." % self.filename)
>              self.parsed_content = self.raw_content
> @@ -179,10 +182,6 @@
>              raise UploadError("Signing key %s not registered in launchpad."
>                                % sig.fingerprint)
>  
> -        if key.active == False:
> -            raise UploadError("File %s is signed with a deactivated key %s"
> -                              % (filename, key.keyid))
> -

It's kinda dodgy that we're now treating the signature as valid and setting self.signingkey when the signing key is effectively revoked. It at least deserves the privatisation of verifySignature and a comment before the single call to it.

>          return (key, sig.plain_data)
>  
>      def parseAddress(self, addr, fieldname="Maintainer"):


-- 
https://code.launchpad.net/~cjwatson/launchpad/better-upload-error-notifications/+merge/311179
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.