[ZCM] [ZC] 1774/ 4 Comment "fix for checkPermission"

[Collector: Zope Bugs, Features, and Patches ...]

Issue #1774 Update (Comment) "fix for checkPermission"
 Status Pending, Zope/bug+solution medium
To followup, visit:
  http://www.zope.org/Collectors/Zope/1774

==============================================================
= Comment - Entry #4 by tseaver on May 12, 2005 8:29 am


Uploaded:  "AccessControl.patch"
 - http://www.zope.org/Collectors/Zope/1774/AccessControl.patch/view
Uploading an updated patch from the submitter, received via
e-mail:

  Here's the updated patch.  It's been manually tested on our end,
  but we haven't written tests for it, obviously. I'll see if I
  can someone working on that.  It is pretty darn close to the
  code in validate (95% I would say) though...

The patch applies cleanly to the head of the 2.7 branch, and does
not cause any tests to fail.  It does *not* apply cleanly to the
SVN trunk, but that isn't too surprising at this point:

 [/home/tseaver/projects/Zope-CVS/Zope-SVN-trunk/lib/python/AccessControl]
 $ patch --dry-run -p1 < /tmp/AccessControl.patch
 patching file ImplPython.py
 Hunk #1 succeeded at 342 (offset 4 lines).
 patching file cAccessControl.c
 Hunk #1 succeeded at 1298 (offset 22 lines).
 Hunk #2 FAILED at 1318.
 1 out of 2 hunks FAILED -- saving rejects to file cAccessControl.c.rej

________________________________________
= Comment - Entry #3 by tseaver on May 6, 2005 10:48 am

Another "substantive" note:

  - Patching the tests with a testcase which fails before the
    main patch, but succeeds afterwards, ensures that the fix
    doesn't get regressed later.  Especially for the securit
    machinery, such testing is probably critical, and it will
    take longer for somebody who doesn't understand the intent
    to write than for the original author of the patch.
________________________________________
= Comment - Entry #2 by tseaver on May 6, 2005 10:46 am

Thanks for the patch.  A couple of "mechanical" things I've noted which will help with future sumisisons:

  - Please don't use tab characters in Zope's Python source.

  - If you can, creating a unified diff or a context diff makes
    the patch more comprehensible.

And a substantive one:

  - The 'ownerous' check should either be dropped, or it should
    be enforced across the board (e.g., not short-circuited
    by 'context.user_allowed').  The point of that machinery is
    to prevent "Trojan horse" attacks, where a malicious-but-
    unprivileged user tricks a more privileged user into viewing
    a page.

    In this case, I'm inclined to drop the fix;  the
    'checkPermission' API is used only by code which is explicitly
    security conscious, and should be expected to handle such
    things itself.
________________________________________
= Request - Entry #1 by tmclaugh on May 6, 2005 8:31 am


Uploaded:  "checkPermission.zip"
 - http://www.zope.org/Collectors/Zope/1774/checkPermission.zip/view
Patches for ImplPython.py and cAccessControl.c to make checkPermission honor Role proxies.  Patches against 2.7.5.
==============================================================