checkPermissions only lets admins write

[Matt Giuca]

It's hard to demonstrate at the moment, but I believe that checkPermissions in all of the *ServiceImpl classes, except for UserService, will prevent non-admins from writing to objects, even if they own them.

This is because all of the checks check the current user's key against the object in question's key (which obviously never match), and set it to private if they match, public otherwise. This means it will ALWAYS result in the public interface. Admins can still write to such an interface, but nobody else can.

Therefore, each ServiceImpl class needs that logic to change to a real check to see whether the user owns that object (which is usually going to be the same condition as the exception check above).

[Scott Ritchie (sritchie)]

Ok so i completely screwed these up when i implemented them based on my interpretations of what the ServiceImpl should be doing;

What each checkPermissions should look like:

/**
 * Gets the Role of the current user, and the View they should see of the object passed to it
 * @param object the object to get the permissions for
 * @param pm the PersistenceManager
 * @return the Role of the current user and the ClientView it should see
private RoleAndView checkPermissions(ModelClass object, PersistenceManager pm) {
   //The currently logged in user
   UserData curUser = UserGetter.getCurrentUser(pm);
   /*TODO: look up the user that the object belongs to through the heirachy of relationships
    * For example suppose object is of type Game:
    * DevTeamData dtd = DevTeamGetter.getDevTeam(object.getDevTeamKey());
    * ClientView cv = ClientView.PUBLIC; //assume by default
    * for (Key k : dtd.getUsers()) {
    * if (k.getID() == curUser.getPrimaryKey() {
    * cv = ClientView.PRIVATE; //CurUser is part of the devteam the object belongs to
    * }
    * }
    */

   return new RoleAndView(curUser.getRole(), cv, curUser);
}

Then you pass the ClientView and Role from the returned RoleAndView to the ModelWrapper.getClient/DataClone calls which will construct/store the object based on the role and view recieved.
This should be in _EVERY_ ServerSideImpl that accesses/writes to the datastore.

Hope this helps.

[Matt Giuca (mgiuca)]

Hang on. Did you read the other thread? Your code above has removed the exception throw -- that is bad. It's still secure, but it fails silently (it reports success and only when you refresh do you realise that it didn't write).

Actually ... at the moment, for editing Game objects, you DO get an error if you are a non-admin trying to write to it, but it is completely unrelated to this bug. (GRR!!!!!) I am about to file ANOTHER bug on that one (which I can fix, but it's probably quite widespread. Wait till you see it.)

[Scott Ritchie (sritchie)]

the UserPrivledgeException should be thrown inside the ModelWrapper (if it
silently fails then it should be added to checking in ModelWrapper)

On Mon, May 23, 2011 at 8:33 PM, Matt Giuca <email address hidden>wrote:

> Hang on. Did you read the other thread? Your code above has removed the
> exception throw -- that is bad. It's still secure, but it fails silently
> (it reports success and only when you refresh do you realise that it
> didn't write).
>
> Actually ... at the moment, for editing Game objects, you DO get an
> error if you are a non-admin trying to write to it, but it is completely
> unrelated to this bug. (GRR!!!!!) I am about to file ANOTHER bug on that
> one (which I can fix, but it's probably quite widespread. Wait till you
> see it.)
>
> --
> You received this bug notification because you are subscribed to MUGLE.
> https://bugs.launchpad.net/bugs/786904
>
> Title:
> checkPermissions only lets admins write
>
> Status in Melbourne University Game-based Learning Environment:
> New
>
> Bug description:
> It's hard to demonstrate at the moment, but I believe that
> checkPermissions in all of the *ServiceImpl classes, except for
> UserService, will prevent non-admins from writing to objects, even if
> they own them.
>
> This is because all of the checks check the current user's key against
> the object in question's key (which obviously never match), and set it
> to private if they match, public otherwise. This means it will ALWAYS
> result in the public interface. Admins can still write to such an
> interface, but nobody else can.
>
> Therefore, each ServiceImpl class needs that logic to change to a real
> check to see whether the user owns that object (which is usually going
> to be the same condition as the exception check above).
>

[Matt Giuca (mgiuca)]

So in my opinion, the only fix required is to use the same comparison above as below. For instance, in GameVersionServiceImpl, replace:

        ClientView cv = (curUser.getPrimaryKey() == object.getPrimaryKey()) ?
                ClientView.PRIVATE : ClientView.PUBLIC;

with:

        ClientView cv = isDev ? ClientView.PRIVATE : ClientView.PUBLIC;

In other words, it's private if you are a developer of this game; otherwise it is public. That's all that should be required.

[Matt Giuca (mgiuca)]

> the UserPrivledgeException should be thrown inside the ModelWrapper (if it
> silently fails then it should be added to checking in ModelWrapper)

Yes, that is absolutely what *should* happen, but it isn't going to happen any time soon. I spent a couple of hours and made a complete analysis over on the other bug. See the last part of the discussion -- the checkPermissions functions have very weird overlapping functionality and I would prefer it be elsewhere but we can't fix it now.

So for now, we are going to keep those checkPermissions functions checking and throwing exceptions. We just need to get the logic right.

I have done what I suggested, and it worked -- I am now getting a PRIVATE view where previously I was getting a PUBLIC. But it is *still* failing silently to actually write to the datastore, no doubt due to yet another bug. I'm on it.

[Scott Ritchie (sritchie)]

Right. You may just want to check that my logic in each case for inDev is
correct

On Mon, May 23, 2011 at 8:43 PM, Matt Giuca <email address hidden>wrote:

> So in my opinion, the only fix required is to use the same comparison
> above as below. For instance, in GameVersionServiceImpl, replace:
>
> ClientView cv = (curUser.getPrimaryKey() == object.getPrimaryKey())
> ?
> ClientView.PRIVATE : ClientView.PUBLIC;
>
> with:
>
> ClientView cv = isDev ? ClientView.PRIVATE : ClientView.PUBLIC;
>
> In other words, it's private if you are a developer of this game;
> otherwise it is public. That's all that should be required.
>
> --
> You received this bug notification because you are subscribed to MUGLE.
> https://bugs.launchpad.net/bugs/786904
>
> Title:
> checkPermissions only lets admins write
>
> Status in Melbourne University Game-based Learning Environment:
> New
>
> Bug description:
> It's hard to demonstrate at the moment, but I believe that
> checkPermissions in all of the *ServiceImpl classes, except for
> UserService, will prevent non-admins from writing to objects, even if
> they own them.
>
> This is because all of the checks check the current user's key against
> the object in question's key (which obviously never match), and set it
> to private if they match, public otherwise. This means it will ALWAYS
> result in the public interface. Admins can still write to such an
> interface, but nobody else can.
>
> Therefore, each ServiceImpl class needs that logic to change to a real
> check to see whether the user owns that object (which is usually going
> to be the same condition as the exception check above).
>

[Matt Giuca (mgiuca)]

OK. Well, I fixed the bug that popped up after fixing this bug (bug #786935). Now it works for GameVersion. I'll go through each ServiceImpl carefully and check your logic as you suggest. I think it's right though -- I have already been through all of those once today (and it was the other logic that was the problem).

[Matt Giuca (mgiuca)]

Fixed in trunk r357.

[Matt Giuca (mgiuca)]

Oh: I just realised why it *has* to fail silently if you pass the checkPermissions and try to write an object. checkPermissions is checking on a per-object basis. Once you pass that stage, it is possible to write some fields but not others. Therefore, it is just going to take the fields which it can and write them, and throw the others away. It doesn't really make sense for it to raise an exception later on if one of the fields can't be written.