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.
Created attachment 54448 [details] [review] Basic unittest support and a few tests Adds basic unit tests for: PolkitIdentity, PolkitUnixUser, PolkitUnixGroup, PolkitBackendLocalAuthorizationStore, and PolkitBackendLocalAuthority.
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 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;"
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.
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!
(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).
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.
(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!
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.
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.
(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.