Bug 43610 - Add netgroup support to Policy Kit
Summary: Add netgroup support to Policy Kit
Status: RESOLVED FIXED
Alias: None
Product: PolicyKit
Classification: Unclassified
Component: daemon (show other bugs)
Version: unspecified
Hardware: Other All
: medium enhancement
Assignee: David Zeuthen (not reading bugmail)
QA Contact: David Zeuthen (not reading bugmail)
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-07 17:35 UTC by Nikki VonHollen
Modified: 2011-12-23 11:40 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
Basic unittest support and a few tests (34.87 KB, patch)
2011-12-14 20:19 UTC, Nikki VonHollen
Details | Splinter Review
Added netgroup support and expanded unit tests with MockLibc (48.12 KB, patch)
2011-12-14 20:48 UTC, Nikki VonHollen
Details | Splinter Review
Added netgroup support and expanded unit tests with MockLibc (151.81 KB, patch)
2011-12-20 17:43 UTC, Nikki VonHollen
Details | Splinter Review
Updated MockLibc to fix srcdir != builddir bug (12.25 KB, patch)
2011-12-21 18:54 UTC, Nikki VonHollen
Details | Splinter Review
Netgroup support and additional testing with MockLibc (164.07 KB, patch)
2011-12-22 13:53 UTC, Nikki VonHollen
Details | Splinter Review
Netgroup support and additional testing with MockLibc (126.36 KB, patch)
2011-12-22 14:57 UTC, Nikki VonHollen
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nikki VonHollen 2011-12-07 17:35:54 UTC
Same as:
https://bugs.launchpad.net/ubuntu/+source/policykit-1/+bug/724052

Netgroup support in Policy Kit would work similarly to unix groups. Where one might use the identity "unix-group:foo", the netgroup version would allow "netgroup:foo".

Netgroup entries only acknowledge the username portion of the netgroup (hostname, username, domainname) triple. 

An example /etc/netgroup file would look like:
somegroup (-,john,) (-,jane,) othergroup
othergroup (-,mary,) (-,bill,)

Which puts "unix-user:mary" in netgroups "somegroup" and "othergroup".

I'll be submitting a patch in the next week or two to add this functionality. The only problem is I'll have to re-factor local authority and identity code to remove the need for getgrouplist which doesn't exist for netgroups (see get_groups_for_user in polkitbackendlocalauthority.c).

The new tests I'm adding for bug #43608 cover all the code I'm re-factoring.
Comment 1 Nikki VonHollen 2011-12-14 20:19:51 UTC
Created attachment 54448 [details] [review]
Basic unittest support and a few tests

Adds basic unit tests for:
PolkitIdentity, PolkitUnixUser, PolkitUnixGroup, PolkitBackendLocalAuthorizationStore, and PolkitBackendLocalAuthority.
Comment 2 Nikki VonHollen 2011-12-14 20:48:29 UTC
Created attachment 54450 [details] [review]
Added netgroup support and expanded unit tests with MockLibc

Please ignore the previous comment/attachment. I got the two bug numbers mixed up.

This patch relies on 'policykit-unittest.patch' from bug #43608.

There are a few issues I ran into implementing netgroups:
1. Netgroups can't support glob syntax like other identities do in *.pkla
files.
2. A netgroup with a wildcard entry like (,,), which means any
user/host/domain, will be ignored in the admin identities list.
3. I use git submodules to include the MockLibc source code, but I'm not sure
if that's what we should stick with. I can include code

Since I'm using submodules, the first commit with the submodule needs to run
'git submodule add' to put special metadata in the repo which can't happen via
patch AFAIK. For eaxmple:
cd PolicyKit
patch -p1 < ~/policykit-unittest.patch
patch -p1 < ~/policykit-netgroup.patch
git submodule add -b v1 https://code.google.com/p/mocklibc/ noinst/mocklibc
Comment 3 David Zeuthen (not reading bugmail) 2011-12-20 08:59:03 UTC
Comment on attachment 54450 [details] [review]
Added netgroup support and expanded unit tests with MockLibc

Review of attachment 54450 [details] [review]:
-----------------------------------------------------------------

Generally the patch looks very good, good job! High-level comments

 - I'm not sure I like git submodules (but TBH I don't have any good reason
   except "it's different!" ... but I think that's reason enough).

   So at this point I would prefer just adding the C and H files from mocklibc
   into tests/mocklibc/*.[ch] and instead of a full buildsystem (with configure.ac
   and all that entails) just having tests/mocklibc/Makefile.am.

   We can always sync with upstream mocklibc if there are bug-fixes or new
   features needed. I just think it's a lot easier to do it this way.

 - Please use 'git format-patch' to format the patch. That way I don't have
   to a) use 'git add' on all the new files; and b) write the commit message.
   (As other developers, I too am lazy!)

 - I think we should probably call it PolkitUnixNetgroup (and
   polkit_unix_netgroup and unix-netgroup:name) instead of
   PolkitNetGroup (and polkit_net_group and netgroup:name).
   Yeah, I agree it's a bit superfluous with the Unix prefix but we
   already do this for users and groups  

 - The coding standards generally use "p != NULL" and "p == NULL" to make
   it explicit that p is a pointer and not a boolean. It's not a must to
   follow these conventions in new code but I would appreciate if we did
   in existing code (consistency matters etc.)

 - Please avoid using C++-style comments .. the main reason for this is
   that I use "//" only when temporarily commenting out code. This makes
   it easy to search for it. Yeah, it's a weird reason but bear with me :-)

::: test/polkit/polkitidentitytest.c
@@ +28,5 @@
> +struct comparison_test_data_type {
> +  const gchar *subject_a;
> +  const gchar *subject_b;
> +  gboolean equal;
> +};

Not a biggie but the code generally uses CamelCase and typedef ... and _type is a bit unneeded... so this would simply be "type struct { } ComparisonTestData;"
Comment 4 David Zeuthen (not reading bugmail) 2011-12-20 09:32:16 UTC
Btw, the patch is also missing modifications to

 docs/polkit/polkit-1-sections.txt
 docs/polkit/polkit-1.types
 docs/polkit/polkit-1-docs.xml

to make the newly added type and functions appear in the docs.
Comment 5 Nikki VonHollen 2011-12-20 17:43:53 UTC
Created attachment 54617 [details] [review]
Added netgroup support and expanded unit tests with MockLibc

I fixed all the style/naming problems you mentioned, and I updated the docs for the API and pklocalauthority.

I also included the entire source of MockLibc, but polkit's ./configure still calls the ./configure for MockLibc. I'll submit another patch without the recursive configure if you want, but that may make updating MockLibc a pain. I'll try to work more on that tomorrow.

This set of patches relies on unitttest support but not the previous netgroup patch I posted, so it should be applied against the current public trunk. I used 'git format-patch --stdout <RANGE>' so the patch is in mbox format.

Thanks!
Comment 6 David Zeuthen (not reading bugmail) 2011-12-21 08:12:04 UTC
(In reply to comment #5)
> I'll try to work more on that tomorrow.

Sounds great, thanks! Btw, release-wise I'm shooting for a release on Friday (even though it's a bad idea to release software just before an weekend or, worse, long holiday).
Comment 7 Nikki VonHollen 2011-12-21 18:54:02 UTC
Created attachment 54662 [details] [review]
Updated MockLibc to fix srcdir != builddir bug

I fixed the bug in MockLibc where builds fail if srcdir != builddir. It should work fine for PolicyKit, but the current PolicyKit head fails to build in that situation with the error "ERROR: polkit.h: no such a file or directory" in src/polkit. I'm looking into that issue now.
Comment 8 David Zeuthen (not reading bugmail) 2011-12-22 06:49:04 UTC
(In reply to comment #7)
> Created attachment 54662 [details] [review] [review]
> Updated MockLibc to fix srcdir != builddir bug
> 
> I fixed the bug in MockLibc where builds fail if srcdir != builddir. It should
> work fine for PolicyKit, but the current PolicyKit head fails to build in that
> situation with the error "ERROR: polkit.h: no such a file or directory" in
> src/polkit. I'm looking into that issue now.

OK, but please don't spend too much time on buildsystems! Also, as per comment 3 the mocklib code should go into test/mocklibc (or test/tools/mocklibc if you want), not noinst/mocklibc. Thanks.

[1] : as a general sentiment, I think that people who are capable of writing code should spend time on doing just that - not fighting arcane build systems from the 1980 era!
Comment 9 Nikki VonHollen 2011-12-22 13:53:04 UTC
Created attachment 54715 [details] [review]
Netgroup support and additional testing with MockLibc

David: This patch set is everything you need to apply to mainline with the previous changes and the noinst/mocklibc -> test/mocklibc move you requested.
Comment 10 Nikki VonHollen 2011-12-22 14:57:06 UTC
Created attachment 54727 [details] [review]
Netgroup support and additional testing with MockLibc

Please ignore the last patch which didn't have all the changes.

This is a single patch (git rebase) with netgroup/testing support and also fixes another srcdir != builddir problem with my tests.
Comment 11 David Zeuthen (not reading bugmail) 2011-12-23 11:40:37 UTC
(In reply to comment #10)
> Created attachment 54727 [details] [review] [review]
> Netgroup support and additional testing with MockLibc
> 
> Please ignore the last patch which didn't have all the changes.
> 
> This is a single patch (git rebase) with netgroup/testing support and also
> fixes another srcdir != builddir problem with my tests.

Looks good, I just pushed the patch with a s-o-b

 http://cgit.freedesktop.org/PolicyKit/commit/?id=674357c20c1b6cb421fea6eb6924b274ec477c0e

Btw, I promised to make a release today but as I'm not working today (just found out this morning that it's a company holiday!) it will have to wait to next week or the week after (Red Hat is shut down over the holidays but I might find a little bit of time to do the release). Thanks for your patch!


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.