Non-admin users cannot edit own profile (production)

Bug #788479 reported by Matt Giuca on 2011-05-26
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
MUGLE
High
Matt Giuca

Bug Description

Non-admin users have userCanEdit set to false (can't see the edit button or access the edit page) for their own User objects. This is not a problem for devteams or games, only users. It only shows up on the production server; the testing server (including the gwtc compiled code) does not exhibit this problem.

Related branches

Matt Giuca (mgiuca) wrote :

Wow, we're a bit screwed here. The problem is this: new Long(4) == new Long(4) is false.

Because all of our primary keys in the client classes are Longs, not longs, comparing them with == is doing a reference check to see whether the Long container is equal to the other Long container. Somehow, this works on the dev mode, but it doesn't on production.

In this particular case, the offending line is in UserServiceImpl.getByName, line 57:
        ClientView cv = (curUser.getPrimaryKey() == ud.getPrimaryKey()) ?
                                        ClientView.PRIVATE : ClientView.PUBLIC;

That needs to be
         ClientView cv = (curUser.getPrimaryKey().equals(ud.getPrimaryKey())) ?
                                        ClientView.PRIVATE : ClientView.PUBLIC;

That fixes this particular problem on production, but who knows what else is buggy because of these sort of comparisons. (Basically all permissions are checked using ==.)

Matt Giuca (mgiuca) wrote :

Okay, that wasn't nearly as hard as I thought. I grepped and carefully read through every single "==" operation in all of Mugle, and it turns out there were just four comparisons between two boxed numbers, all of them in the user service (which explains why this wasn't a problem for teams or games).

Fixed in trunk r459 -- now use .equals for all comparisons between boxed numbers.

Changed in mugle:
status: In Progress → Fix Committed

Perhaps we can do a find and replace of all instances of ==, since
most of the comparisons will be based on references like this.

On 26/05/2011, at 4:01 PM, Matt Giuca <email address hidden> wrote:

> Wow, we're a bit screwed here. The problem is this: new Long(4) == new
> Long(4) is false.
>
> Because all of our primary keys in the client classes are Longs, not
> longs, comparing them with == is doing a reference check to see
> whether
> the Long container is equal to the other Long container. Somehow, this
> works on the dev mode, but it doesn't on production.
>
> In this particular case, the offending line is in
> UserServiceImpl.getByName, line 57:
> ClientView cv = (curUser.getPrimaryKey() == ud.getPrimaryKey
> ()) ?
> ClientView.PRIVATE :
> ClientView.PUBLIC;
>
> That needs to be
> ClientView cv = (curUser.getPrimaryKey().equals
> (ud.getPrimaryKey())) ?
> ClientView.PRIVATE :
> ClientView.PUBLIC;
>
> That fixes this particular problem on production, but who knows what
> else is buggy because of these sort of comparisons. (Basically all
> permissions are checked using ==.)
>
> --
> You received this bug notification because you are subscribed to
> MUGLE.
> https://bugs.launchpad.net/bugs/788479
>
> Title:
> Non-admin users cannot edit own profile (production)
>
> Status in Melbourne University Game-based Learning Environment:
> In Progress
>
> Bug description:
> Non-admin users have userCanEdit set to false (can't see the edit
> button or access the edit page) for their own User objects. This is
> not a problem for devteams or games, only users. It only shows up on
> the production server; the testing server (including the gwtc
> compiled
> code) does not exhibit this problem.

Matt Giuca (mgiuca) wrote :

Forgot to check for instances of !=. Fixed a few more bad comparisons in trunk r461.

Matt Giuca (mgiuca) wrote :

@Scott: Nope, only a handful, it turns out. The majority of them are == null, and some of them are in database queries.

Scott Ritchie (sritchie) wrote :

Won't this still be a problem with the database queries? I'm confused

On 26/05/2011, at 4:36 PM, Matt Giuca <email address hidden> wrote:

> @Scott: Nope, only a handful, it turns out. The majority of them are
> ==
> null, and some of them are in database queries.
>
> --
> You received this bug notification because you are subscribed to
> MUGLE.
> https://bugs.launchpad.net/bugs/788479
>
> Title:
> Non-admin users cannot edit own profile (production)
>
> Status in Melbourne University Game-based Learning Environment:
> Fix Committed
>
> Bug description:
> Non-admin users have userCanEdit set to false (can't see the edit
> button or access the edit page) for their own User objects. This is
> not a problem for devteams or games, only users. It only shows up on
> the production server; the testing server (including the gwtc
> compiled
> code) does not exhibit this problem.

Matt Giuca (mgiuca) wrote :

No, database queries is a separate language (JDO Query Language); in that language the numbers are primitives, not boxed (for them to be boxed, they would have to have their own database table), so it couldn't possibly be doing a reference equality. It's only in the Java code that it's a problem, since == checks the pointers to the boxes.

Matt Giuca (mgiuca) on 2011-05-26
Changed in mugle:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers