ptserver: Fix AccessOK -restricted for addToGroup

The function AccessOK is used by all of ptserver RPC handlers that
need to do an authorization check, and the last two arguments are set
as such:

- When adding a member to a group, 'mem' is PRP_ADD_MEM and 'any' is
  PRP_ADD_ANY

- When removing a member from a group, 'mem' is PRP_REMOVE_MEM and
  'any' is 0

- When modifying an entry (setFieldsEntry) or modifying some global
  database fields, 'mem' and 'any' are both set to 0

- When reading an entry and not modifying it, 'mem' and/or 'any' are
  set to other values (depending on if we're checking membership,
  examining the entry itself, etc)

Commit 93ece98c (ptserver-restricted-mode-20050415) added a check to
AccessOK to make it return false for -restricted mode when we are
adding a member to a group, or when 'mem' and 'any' are both 0. This
didn't catch the case when we are removing a member from a group,
though, when 'mem' is PRP_REMOVE_MEM.

It looks like commit a614a8d9 (ptutils-restricted-accessok-20081025)
tried to fix this by adding a check for PRP_REMOVE_MEM, but it also
required 'any' to be set to 0 for the conditional to succeed. This is
true when removing a member from a group, but when adding a member to
a group, 'any' is PRP_ADD_ANY, and so this check fails.

This means that currently, when restricted mode is turned on,
non-admins can still run addToGroup and setFieldsEntry successfully.

Fix this by checking for PRP_ADD_MEM/PRP_REMOVE_MEM separately from
checking if 'mem'/'any' are set to 0. Break up this conditional into
separate if() statements with comments to try to make the checks
more clear.

Reviewed-on: https://gerrit.openafs.org/13370
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
(cherry picked from commit dfc78d533ef64c8d6daf134e2a0f67c5c16f7369)

Change-Id: I7f53570b42e2700a33dd5e72a31f6f7f8b876e79
Reviewed-on: https://gerrit.openafs.org/13686
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Tested-by: Andrew Deason <adeason@sinenomine.net>
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Reviewed-by: Stephan Wiesand <stephan.wiesand@desy.de>

diff --git a/src/ptserver/ptutils.c b/src/ptserver/ptutils.c
index 511f57af51a352274e1545244d6402aa4470a7db..7ea7202dcc831a3cde9a77a15959810b75de07b5 100644 (file)
--- a/src/ptserver/ptutils.c
+++ b/src/ptserver/ptutils.c
@@ -286,8 +286,17 @@ AccessOK(struct ubik_trans *ut, afs_int32 cid,             /* caller id */
        return 1;
     if (cid == SYSADMINID)
        return 1;               /* special case fileserver */
-    if (restricted && ((mem == PRP_ADD_MEM) || (mem == PRP_REMOVE_MEM)) && (any == 0))
-       return 0;
+    if (restricted) {
+        if (mem == PRP_ADD_MEM || mem == PRP_REMOVE_MEM) {
+            /* operation is for adding/removing members from a group */
+            return 0;
+        }
+        if (mem == 0 && any == 0) {
+            /* operation is for modifying an entry (or some administrative
+             * global operations) */
+            return 0;
+        }
+    }
     if (tentry) {
        flags = tentry->flags;
        oid = tentry->owner;