launchpad-dev team mailing list archive

[Abel Deuring <abel.deuring@xxxxxxxxxxxxx>]

On 25.07.2010 15:31, Jonathan Lange wrote:
> On Wed, Jul 21, 2010 at 5:54 PM, Abel Deuring
> <abel.deuring@xxxxxxxxxxxxx> wrote:
>> The branch lp:~adeuring/launchpad/security-guarded-test-object-factory-1
>> is at present in ec2 and will land soon.
>>
>> Its main change affects LaunchpadObjectFactory: This class has at
>> present ca 20 makeWhatever() methods which return objects without a
>> security proxy. If this happens, LaunchpadObjectFactory will now print a
>> warning.
>>
> 
> I've got a branch up that tweaks this a little to actually emit
> warnings, rather than printing straight to stderr:
>   https://code.edge.launchpad.net/~jml/launchpad/warnings-for-unproxied/+merge/30891

Sounds good. But:

1. I don't know why we raised errors for non-silenced warnings. In
general, we should aim to get rid of all warnings in our code/tests. I
would even consider the warnings introduced by my branch to be errors --
the point is that they occur far too often to be fixed in a single
branch, so I opted instead to annoy everbody who looks at the output of
affected tests ;)
2. I intend to replace the first warning ("PLEASE FIX:...") by an
exception once I added security wrappers to all objects returned by
methods of LPObjectFactory.

> 
> ...
>> As a simple fix/workaround, I added a function
>> remove_security_proxy_and_shout_at_engineer(obj) which just returns
>> removeSecurityProxy(obj) but it also prints a warning to stderr.
>>
> 
> The text of the warning isn't particularly clear to me. If I start
> seeing this warning when running my tests, what should I do about it?

It indicates that there is a bad workaround to prevent a test failure
that is caused by a permission problem. So, when we work on an test that
shows this warning, I think we should spend a few minutes to see how we
can fix it:

  - consciously call removeSecurityProxy() because it is apparent that
    the test is not about access to the affected attribute by the
    current user. Might be reasonable for example in a setUp() method.
    (Though it is probably better to use the new with_person_logged_in)
  - login() as the right person
  - move an existing login() call a few lines up or down.
  - if the current user should have access to the attribute but hasn't,
    fix the permission.

Abel