Item9318: New unit test for Func::saveAttachment

Found an issue with one of the plugins - adding a unit test.

-- GeorgeClark - 14 Jul 2010

Sorted

-- CrawfordCurrie - 14 Jul 2010

Reopening this. Further comparison against 1.0 shows other Func calls will not report errors that were caught in 1.0

Committing updates to moveTopic and moveAttachment - Need feedback on analysis of saveTopic, which is documented as not checking, but calls 1.0 store with user parameter which would actually result in the access checks.

-- GeorgeClark - 18 Jul 2010

Ouch.

OK. It goes against my better judgement to have these checks in Func, but the reality is that we have to maintain it as compatible as possible to previous releases, so the checks have to be restored. Note that I'm hoping over time to be able to replace the store functions of Func with calls to the meta object (what has been referred to as the "OO Func" interface), and the meta interface doesn't carry the burden of access checks, so we shouldn't get too tangled up in philosophising about this. So, wherever you find an incompatibility, just fix it in Func.

-- CrawfordCurrie - 19 Jul 2010

Re-opened because the checks in createWeb are wrong - you should not need to have to have change access on the root when adding a new subweb. You need to have change access on the parent web. Also, you don't b=need CHANGE in the baseweb - it's just copied, you only need VIEW.

I only discovered this when some unit tests in an extension started failing.

-- CrawfordCurrie - 20 Aug 2010

More problems; checkAccessPermission now requires a defined topic, making it impossible to check access permissions on webs

-- CrawfordCurrie - 20 Aug 2010

Committing a fix for the issues with createWeb permissions. The CHANGE/VIEW issue was a copy/paste typo. Threw correct exception for wrong check. Fixed the root/parent web check and added a unit test.

The checkAccessPermission requiring a topic is documented as requiring a topic. That wasn't changed relative to this task that I can see. Looks like this part would be a new feature request.

-- GeorgeClark - 20 Aug 2010

Create Item9512 for this issue (missing topic name).

-- GeorgeClark - 20 Aug 2010

And thus a documentation error becomes a specification.

AFAIK checkAccessPermission has always checked web permissions when passed a web. Common sense dictates that the doc is wrong, and the recent code changes that work from the doc are also wrong. A bug, not a feature.

Closing again, as George has addressed all my (current) concerns.

-- CrawfordCurrie - 21 Aug 2010

Had to re-open, because I just discovered you can't save a topic containg text that denies you change access.

Try using Foswiki::Func::saveTopic to save the topic text:

• Set DENYTOPICCHANGE = me

The problem is that the topic object used for the check is initialised with the text being saved, which is checked by the access control checks..... whoops.

-- CrawfordCurrie - 22 Aug 2010

Re-opening once more because the last change from Crawford, which was correct, exposed a bug in another test: FuncTests::test_saveTopic. This test used to rely on the fact that the permissions were read AFTER writing, which is no longer the case, as it was a bug.

Fixing the unit test to avoid permission checking, by passing ignorepermissions => 1 to the Func call, which he fixed in the original bug Item935. I'll just commit the perltidy + cleanup I made while digging then

-- OlivierRaginel - 02 Sep 2010

Commenting is disabled while not logged in