Code review comment for lp:~mvo/aptdaemon/support-for-whitelisted-repositories

On Mon, Aug 27, 2012 at 01:55:22PM -0000, Martin Pitt wrote:
> Review: Needs Fixing

Thanks for the review.

> As a general nitpick, I'd prefer to rename "whitelisted" in any public API/properties etc. to "passwordless" or something similarly meaningful, to point out what this list actually does. This especially applies to the conffile, as renaming it later is painful.

Right. I tend to agree but its not neseccarily passwordless, i.e. the
default implementation of this has auth_admin. Its just easier to
override this via e.g. the policykit-desktop-privileges or a special
webapps packages that does the same. Maybe a better name would be
"potentially_passordless" instead of "whitelisted"? But that gets a
bit long :/ Any hints appreciated.

> The test case does not actually verify that the expected PK privilege is being used, as it only goes until the simulation stage (or did I get that wrong?). You could tell the mock polkitd to permit org.debian.apt.install-packages-from-whitelisted-repo and nothing else, and verify that installation of silly-* works, but other-pkg fails due to a permission error. It would also be nice to then try to remove the package again and verify that for this case (i. e. any but "install" it requires the "full" privilege.

Indeed good points, I added tests for that now too and also
consolidated the whitelist test stuff into a single file.

> I also think read_repository_whitelist() should become more robust against errors in the conffile. ConfigParser defaults to "strict" and thus excepts out if there a duplicate keys, etc. We don't want aptdaemon to crash on a ConfigParser.* exception; I think it should just be logged and it should return an empty set then. (Also easy to testcase).

Thanks, good point. This is more robust now I added tests and logging
(and test for the logging).

This should be ready for re-review, except that we need to find a
better name for whitelisted.

Thanks,
 Michael