mugle-dev team mailing list archive

[Matt Giuca <786876@xxxxxxxxxxxxxxxxxx>]

I am trying to get my head around the purpose of checkPermissions. The
canonical version, in UserServiceImpl, seems to do the same thing twice,
and the second check is rather redundant. The cloned versions have been
modified to do the first check properly, but not the others.

Here is pseudocode for UserServiceImpl.checkPermissions:

INPUT givenUser:
    curUser := get the current user
    // Permissions checks (raise exception)
    if curUser is not active:
        throw permission exception
    if (curUser is not an admin) and (curUser is not the givenUser):
        throw permission exception
    // Calculate the clientview (public/private)
    if curUser is not the givenUser:
        make the view PUBLIC
    else (curUser is the givenUser):
        make the view PRIVATE

Now if you look at this code carefully (ignoring active/inactive users),
there are basically three possibilities:

1. curUser is the givenUser:
    make the view PRIVATE
2. curUser is not the givenUser, but is an admin
    make the view PUBLIC
3. curUser is not the givenUser, and not an admin either
    throw permission exception

Hence the second part is really redundant, because the ONLY way to make
the view PUBLIC is if curUser is an admin, in which case he can do
anything he wants to anyway. I thought the whole point of the
public/private system was that if it was private and the user did not
have sufficient roles, they would be prevented from reading or writing.
If that's true, then just the public/private system on its own is a good
general way to set permissions. Having checkPermissions do this extra
check makes it inflexible, as it is effectively imposing the constraint
"you may only do this if you are an admin OR the owner of this object",
and ignoring all of the finer rules we have defined elsewhere.

That is why checkPermissions cannot be used by all of the "getters",
because when you want to READ an object you don't want to throw an
exception just because you don't own the object. So the getters are
actually relying on the proper permissions system, whereas anybody who
calls checkPermissions is relying on two separate permissions systems
which do the same thing.

Is this a fair assessment? I am wondering what happens if I simply
remove that early check that throws the UserPrivilegeException, whether
the privileges are still taken care of later on. ?

-- 
You received this bug notification because you are a member of MUGLE
Developers, which is a direct subscriber.
https://bugs.launchpad.net/bugs/786876

Title:
  Almost all data access is given private privileges

Status in Melbourne University Game-based Learning Environment:
  Triaged

Bug description:
  The data view system has been horribly abused. I haven't made a
  complete analysis of all the classes, but by the look of it, in most
  cases, objects are being presented with the private view whether the
  user owns it or not. Particularly the Game, GameVersion and GameFile
  exhibit this. The offending code is in the ServiceImpl classes, which
  assign the view type.

  At this point, most of our security is happening by accident, and can
  easily be subverted.

  It looks like most of the WRITES have buggy checks which end up
  resulting in "private" no matter what (eg, make it public if two keys
  of incompatible types are equal, but otherwise make it private). Most
  of the READS simply assign "private" without any checks at all. This
  code needs a complete audit.

  Note that you often don't notice these problems, because the role
  isn't high enough even for private access. For example, on GameData,
  most fields have public access for admin only, but private access for
  developers. That means guests can't see Games, but developers have
  full access to games - even ones they didn't create. This demonstrates
  the silliness of the developer role (bug #786842).

  This bug is responsible for bug #786685.