Mailing List Archive

Sidnei da Silva wrote:
> Found some lovely piece of code deep into the FTP parts of Zope 2 last
> saturday, one of them is truely ugly. It's listing the contents of
> the current and parent folders for no apparent reason (or at least, it
> didn't make sense either to me or Chris McDonough).

There's a comment stating the intent.

> The code in question is in the ``manage_FTPstat`` method of
> ``OFS.ObjectManager``. Tracing back the source of this code, it seems
> to have been (surprisingly) introduced by Jim Fulton, back in
> 1999. [1]
>
> Later on, Amos Latteier changed part of the code (namely
> ``manage_FTPlist``) to use a slightly more friendlier, yet equally
> confusing ``is_acquired`` method. [2]
>
> I'm now sitting here, trying to make sense of this code and wondering
> what was the original intention in the first place. Would anyone have
> a clue?

I think the comments make this pretyu clear. The intent is to avoid
providing listings of acquired objects. In fact, the intent of both
bits of code, I think is to prevent FTP access to acquired objects.

I suspect that there is a clearer way to implement the intent.

Jim

--
Jim Fulton mailto:jim@zope.com Python Powered!
CTO (540) 361-1714 http://www.python.org
Zope Corporation http://www.zope.com http://www.zope.org

_______________________________________________
Zope-Coders mailing list
Zope-Coders@zope.org
http://mail.zope.org/mailman/listinfo/zope-coders

On Mon, Oct 31, 2005 at 10:05:45AM -0500, Jim Fulton wrote:
| Sidnei da Silva wrote:
| >Found some lovely piece of code deep into the FTP parts of Zope 2 last
| >saturday, one of them is truely ugly. It's listing the contents of
| >the current and parent folders for no apparent reason (or at least, it
| >didn't make sense either to me or Chris McDonough).
|
| There's a comment stating the intent.

Yes, but the original code does the check in a truely obscure way, at
least to me. I've thought of spelling:

# check to see if we are acquiring our objectValues or not
if not (len(REQUEST.PARENTS) > 1 and
self.objectValues() == REQUEST.PARENTS[1].objectValues()):

As:

if self.objectValues.im_self is not self:

However I'm not sure that would be the be the correct
replacement. There doesn't seem to exist any test for this.

| >The code in question is in the ``manage_FTPstat`` method of
| >``OFS.ObjectManager``. Tracing back the source of this code, it seems
| >to have been (surprisingly) introduced by Jim Fulton, back in
| >1999. [1]
| >
| >Later on, Amos Latteier changed part of the code (namely
| >``manage_FTPlist``) to use a slightly more friendlier, yet equally
| >confusing ``is_acquired`` method. [2]
| >
| >I'm now sitting here, trying to make sense of this code and wondering
| >what was the original intention in the first place. Would anyone have
| >a clue?
|
| I think the comments make this pretyu clear. The intent is to avoid
| providing listings of acquired objects. In fact, the intent of both
| bits of code, I think is to prevent FTP access to acquired objects.

Wouldn't that be better implemented by making the same check as it's
done for WebDAV and set 'maybe_webdav_client' (abusing the name)
so that it sets the 'no_acquire_flag'?

| I suspect that there is a clearer way to implement the intent.

I have three suggestions right now:

1. change manage_FTPstat to use the same thing as manage_FTPlist
(using App.Common.is_acquired, and possibly rewriting is_acquired
to a simpler check)

OR

2. Throw away the `is_acquired` check and replace it by a much
simpler check

OR

3. Setting 'maybe_webdav_client' in the request object so the object
is never acquired for FTP, the same way as it's done for WebDAV.

--
Sidnei da Silva
Enfold Systems, LLC.
http://enfoldsystems.com
_______________________________________________
Zope-Coders mailing list
Zope-Coders@zope.org
http://mail.zope.org/mailman/listinfo/zope-coders

Sidnei da Silva wrote:
> On Mon, Oct 31, 2005 at 10:05:45AM -0500, Jim Fulton wrote:
> | Sidnei da Silva wrote:
> | >Found some lovely piece of code deep into the FTP parts of Zope 2 last
> | >saturday, one of them is truely ugly. It's listing the contents of
> | >the current and parent folders for no apparent reason (or at least, it
> | >didn't make sense either to me or Chris McDonough).
> |
> | There's a comment stating the intent.
>
> Yes, but the original code does the check in a truely obscure way, at
> least to me. I've thought of spelling:
>
> # check to see if we are acquiring our objectValues or not
> if not (len(REQUEST.PARENTS) > 1 and
> self.objectValues() == REQUEST.PARENTS[1].objectValues()):
>
> As:
>
> if self.objectValues.im_self is not self:

That might be better.

> However I'm not sure that would be the be the correct
> replacement. There doesn't seem to exist any test for this.

Of course, there's no test for this. We weren't doing automated
testing back then. Feel free to write one.


> | >The code in question is in the ``manage_FTPstat`` method of
> | >``OFS.ObjectManager``. Tracing back the source of this code, it seems
> | >to have been (surprisingly) introduced by Jim Fulton, back in
> | >1999. [1]
> | >
> | >Later on, Amos Latteier changed part of the code (namely
> | >``manage_FTPlist``) to use a slightly more friendlier, yet equally
> | >confusing ``is_acquired`` method. [2]
> | >
> | >I'm now sitting here, trying to make sense of this code and wondering
> | >what was the original intention in the first place. Would anyone have
> | >a clue?
> |
> | I think the comments make this pretyu clear. The intent is to avoid
> | providing listings of acquired objects. In fact, the intent of both
> | bits of code, I think is to prevent FTP access to acquired objects.
>
> Wouldn't that be better implemented by making the same check as it's
> done for WebDAV and set 'maybe_webdav_client' (abusing the name)
> so that it sets the 'no_acquire_flag'?

Possibly.

> | I suspect that there is a clearer way to implement the intent.
>
> I have three suggestions right now:
>
> 1. change manage_FTPstat to use the same thing as manage_FTPlist
> (using App.Common.is_acquired, and possibly rewriting is_acquired
> to a simpler check)

That's an improvement,

> OR
>
> 2. Throw away the `is_acquired` check and replace it by a much
> simpler check

Sounds good, doing so consistently.

> OR
>
> 3. Setting 'maybe_webdav_client' in the request object so the object
> is never acquired for FTP, the same way as it's done for WebDAV.

Possibly. I don't have time to look at that.

Jim


--
Jim Fulton mailto:jim@zope.com Python Powered!
CTO (540) 361-1714 http://www.python.org
Zope Corporation http://www.zope.com http://www.zope.org
_______________________________________________
Zope-Coders mailing list
Zope-Coders@zope.org
http://mail.zope.org/mailman/listinfo/zope-coders

On Oct 31, 2005, at 10:26 AM, Jim Fulton wrote:

> Sidnei da Silva wrote:
>> On Mon, Oct 31, 2005 at 10:05:45AM -0500, Jim Fulton wrote:
>> | Sidnei da Silva wrote:
>> | >Found some lovely piece of code deep into the FTP parts of Zope
>> 2 last
>> | >saturday, one of them is truely ugly. It's listing the contents of
>> | >the current and parent folders for no apparent reason (or at
>> least, it
>> | >didn't make sense either to me or Chris McDonough).
>> | | There's a comment stating the intent.
>> Yes, but the original code does the check in a truely obscure way, at
>> least to me. I've thought of spelling:
>> # check to see if we are acquiring our objectValues or not
>> if not (len(REQUEST.PARENTS) > 1 and
>> self.objectValues() == REQUEST.PARENTS
>> [1].objectValues()):
>> As:
>> if self.objectValues.im_self is not self:
>
> That might be better.

Is that what an equivalent test? It's difficult to know the intent
here.

- C


_______________________________________________
Zope-Coders mailing list
Zope-Coders@zope.org
http://mail.zope.org/mailman/listinfo/zope-coders

Chris McDonough wrote:
>
> On Oct 31, 2005, at 10:26 AM, Jim Fulton wrote:
>
>> Sidnei da Silva wrote:
>>
>>> On Mon, Oct 31, 2005 at 10:05:45AM -0500, Jim Fulton wrote:
>>> | Sidnei da Silva wrote:
>>> | >Found some lovely piece of code deep into the FTP parts of Zope 2
>>> last
>>> | >saturday, one of them is truely ugly. It's listing the contents of
>>> | >the current and parent folders for no apparent reason (or at
>>> least, it
>>> | >didn't make sense either to me or Chris McDonough).
>>> | | There's a comment stating the intent.
>>> Yes, but the original code does the check in a truely obscure way, at
>>> least to me. I've thought of spelling:
>>> # check to see if we are acquiring our objectValues or not
>>> if not (len(REQUEST.PARENTS) > 1 and
>>> self.objectValues() == REQUEST.PARENTS
>>> [1].objectValues()):
>>> As:
>>> if self.objectValues.im_self is not self:
>>
>>
>> That might be better.
>
>
> Is that what an equivalent test? It's difficult to know the intent here.

Suppose we have folders:

f1/
f11/
f12/
f2/

We don't want to allow someone to access paths like:

f1/f11/f1
f1/f11/f2
f2/f1

and so on.

Jim

--
Jim Fulton mailto:jim@zope.com Python Powered!
CTO (540) 361-1714 http://www.python.org
Zope Corporation http://www.zope.com http://www.zope.org
_______________________________________________
Zope-Coders mailing list
Zope-Coders@zope.org
http://mail.zope.org/mailman/listinfo/zope-coders