launchpad-dev team mailing list archive

[Jonathan Lange <jml@xxxxxxxxxxxxx>]

On Mon, Jul 26, 2010 at 9:10 AM, Abel Deuring
<abel.deuring@xxxxxxxxxxxxx> wrote:
> 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 ;)

Don't worry, these warnings will still annoy everyone. Just only once
per thing that needs fixing.

> 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.
>

That'll be even easier to do with the warning code I've added :)

...
> 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.

OK, thanks. I'll try to update the docs/warning to incorporate this.

jml