Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Zope: Coders

Confusing code in ObjectManager manage_FTPstat

 

 

Zope coders RSS feed   Index | Next | Previous | View Threaded


sidnei at enfoldsystems

Oct 31, 2005, 5:02 AM

Post #1 of 6 (7487 views)
Permalink
Confusing code in ObjectManager manage_FTPstat

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

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?

[1] http://tinyurl.com/86upc
[2] http://tinyurl.com/9adc5

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


jim at zope

Oct 31, 2005, 7:05 AM

Post #2 of 6 (7150 views)
Permalink
Re: Confusing code in ObjectManager manage_FTPstat [In reply to]

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 [at] zope 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 [at] zope
http://mail.zope.org/mailman/listinfo/zope-coders


sidnei at enfoldsystems

Oct 31, 2005, 7:20 AM

Post #3 of 6 (7183 views)
Permalink
Re: Re: Confusing code in ObjectManager manage_FTPstat [In reply to]

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 [at] zope
http://mail.zope.org/mailman/listinfo/zope-coders


jim at zope

Oct 31, 2005, 7:26 AM

Post #4 of 6 (7179 views)
Permalink
Re: Re: Confusing code in ObjectManager manage_FTPstat [In reply to]

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 [at] zope 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 [at] zope
http://mail.zope.org/mailman/listinfo/zope-coders


chrism at plope

Nov 1, 2005, 7:26 AM

Post #5 of 6 (7174 views)
Permalink
Re: Re: Confusing code in ObjectManager manage_FTPstat [In reply to]

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 [at] zope
http://mail.zope.org/mailman/listinfo/zope-coders


jim at zope

Nov 1, 2005, 7:57 AM

Post #6 of 6 (7169 views)
Permalink
Re: Re: Confusing code in ObjectManager manage_FTPstat [In reply to]

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 [at] zope 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 [at] zope
http://mail.zope.org/mailman/listinfo/zope-coders

Zope coders RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.