idea

ACL Parsing should be moved into the security policy.

Currently, ACLs are parsed in Page::getACL(). This feels wrong, because they're actually used only by the security policy. Therefore, I propose moving the parsing there. -- JohannesBerg 2005-02-16 16:52:40

comments

This would straighten the current call paths and simplify the structure of MoinMoin's security system. -- AlexanderSchremmer 2005-02-16 16:54:12

ACL are NOT parsed in getACL they are parsed in wikiacl.

ACL has two modes:

1. text, in the page text (maybe in separate file in 1.4)

2. ACL object created from the text - the parsing of the text into rights dict is done in wikiacl, in the AccessControlList class.

The text mode of acl has nothing to do with the security policy. Security policy should not know anything about the storage of acls, it should do only policy, using ACL objects. The page class should be responsible to storage of the acl lines and retrieving them when needed. It should return an ACL object created from the acl lines. I don't see any problem here or any benefit with this idea.

The real problem with acl is the caching. To get page acl, the code access the disk to get the page version, only then it can get the acl object from the memory cache. -- NirSoffer 2005-02-16 21:09:39

• Woah. No need to shout at me. Now go check your call-stack: where is wikiacl imported and wikiacl.parseACL called? Right: in Page.getACL()! I don't care where the actual code resides, this is a pure implementation issue, but where it is used.

  Now, I agree with your statement that [t]he page class should be responsible to storage of the acl lines and retrieving them when needed. I don't, however, agree with having the Page class parse the ACL statements, because it is of no concern to the Page. This is no huge procedural program where you just stick functions in and be done with it -- the way I see it the Page class is responsible for storing data (and, maybe, using other objects to parse the page and sent it out when required). However, parsing the ACLs is just wrong because it doesn't ever use them. Only the security policy uses the ACLs, and thus it should only be using the Page as a storage for the ACL representation that is saved to disk. If you think that the security policy is only a policy, then maybe it should be renamed to reflect the fact that it, in essence, is responsible for the security. -- JohannesBerg 2005-02-17 08:57:35

  • Look in parseACL - this function does not really parse the acl, its just collect all acl lines, that starts with #acl, and then create an acl object with those lines. The acl object does the parsing internally. Actually the parseACL function belong to the page class, and should be removed from wikiacl, as its not related to acl at all, but to to page storage. It does not make sense that acl object will get page text and extract from it the acl lines, as page text is private thing of the page.

    I think we are wasting our time on these kind of debates, there is no real problem here, and there is no benefit by moving the code 2cm to the right or to the left. Think about the end users - what they get from this change? -- NirSoffer 2005-02-17 09:36:51

    • They get a rock-solid security model that does not delegate its internal subprocesses across various code parts. This should simplify writing extensions quite much and make MoinMoin safer. Just think shortly: if this change had been made earlier, we could have prevented a few security issues.

      • Sorry, I don't see how this will improve the security model, nor how this simplify writing plugins.
        • An example: An end user wants a macro to list users who can access the page. Is it easier one way or the other?
          • It is very easy today, you just call page.getACL(), then list the users and groups on in the acl object. It does not matter where is the code that parse the page acl lines.