| 150 | 150 | * [#ContextFactoriesandSubclasses Context Factories and Subclasses]:[[br]]We need some kind of dynamic description of resources, for all the parts that are generic about resources in Trac: the attachments, the history and diffs of resource content (#2945), the generic reports, etc.[[br]]Those things have been built some time after the `WikiContext` integration, so it's true that they are separate enhancements. But I think that they are very worthwhile ones that have to be kept and probably expanded upon (generic comments, content change annotation, etc.)[[br]]If there's really a need to decouple the way resources can be identified from the way resources can be described, then we could eventually have simple Resource objects and "rendering" methods from a manager component like the one suggested. That would actually make some parts of the implementation simpler, as we could create very simple Resource objects which could delegate their dynamic aspects to their managing component. |
| 151 | 151 | * [#WikiRenderingContext Wiki Rendering Context]:[[br]]We can't simply move all the responsibilities of the rendering context to the Formatter class, as Formatter objects are only present when explicitly rendering Wiki text, not generally available when rendering "content", in the mimeview module (the "content" which is rendered is coming from resources and that content may be wiki content). So we need something (a `RenderingContext` class?) that would be made available to the content renderers, and from there to the formatter, and from the formatter conveyed to the wiki processors. For example, the "blame" kind of annotation, now available for repository source file, later for other resources (#5821) is done at the mimeview layer, and doesn't depend on the wiki layer, so using a Formatter here is not appropriate. Another example of the use of a context which knowns about the "parent" context is to provide a way to guard against recursive content display. This is not yet that common for now, as the possibility to embed one content into another are limited so far (the !TicketQuery macro in table mode could lead to such a situation, though). So the `RenderingContext` is really something that belongs to the `trac.mimeview` layer and could be used in further developments to refactor/simplify that API as well (see #3332). |
| 152 | | * [#PermissionSystem Permission System]:[[br]] if the permission system is not to be bound to the req, then I see no good reason why we use req.perm as its main entry point (other than backward compatibility issues). The rendering context seems to be a possible place to store this information (who is accessing the content?). Alternatively, we could introduce an User object, for now as simple as a wrapper for the authname and a place to store the authentification cache. |
| 153 | | * About [#osimons osimons] points 2. and 3.: I think this is an interesting proposal. In order to get a resource descriptor at all, the accessor should at least have read access to it. This could for example be enforced by requiring a mandatory user argument when creating a resource descriptor: `ResourceManager(self.env, user).ticket(42)`.[[br]]Also, regardless from how the `PermissionCache` is obtained (currently req.perm), I think using it as the principal way to produce the resource identifiers is a good idea. |
| 154 | | * In general, I'm fully for ditching out the req out of the picture, of both the resource descriptors (of course) and the rendering contexts. As osimons said, it would be really nice to be able to use the rendering stuff of Trac in an off-line context. We don't necessarily need the req in all of the rendering chain, but we do need some of the information so far only available from the req, most notably the user information (req.authname). Other important things that are currently stored in the Request is the Chrome data (the parts of the template data consumed by layout.html). |
| 155 | | |
| 156 | | Now, the above ideas have been confronted to some actual coding and I'm getting to the point of having a working prototype. Several details are still in need of additional work (especially the rendering context par), but I think I'm getting there.[[br]] |
| | 152 | * [#PermissionSystem Permission System]:[[br]] |
| | 153 | * About [#osimons osimons] points 2. and 3.: I think this is an interesting proposal. In order to get a resource descriptor at all, the accessor should at least have read access to it. I can't think of a reason when one should be able to manipulate a resource without having at least read permission. If that's ''really'' needed, that should still be possible, but it's all but the most common case. So the point would be to find a nice and simple way to create resource identifiers/descriptors for authorized resources. [[br]] The `PermissionCache` seems to be the best place to stick this convenient resource creation mechanism, for a couple of reasons: |
| | 154 | - there's already a `PermissionCache` is available in most place of the code where we already use it (`req.perm`), |
| | 155 | - it's convenient to create when there's no `req` available (`PermissionCache(env, username)`) |
| | 156 | - we're already using it for enforcing read permissions (`if 'WIKI_VIEW' in req.perm: ...`), even for fine-grained permissions (`if 'WIKI_VIEW' in req.perm('wiki', 'WikiStart'): ...`) |
| | 157 | * In general, I'm fully for ditching out the req out of the picture, of both the resource descriptors (of course) and the rendering contexts. As osimons said, it would be really nice to be able to use the rendering stuff of Trac in an off-line context. We don't necessarily need the req in all of the rendering chain, but we do need some of the information so far only available from the req, most notably the user information (req.authname). Other important things that are currently stored in the Request is the Chrome data (the parts of the template data consumed by layout.html). Overall, the current code base relies too much on the `req` object in the rendering process, so removing the request from the rendering should be left for later iterations. OTOH, the request is now removed from the resource and permission sides completely. See osimons point 1. and cmlenz first comment and paragraph in "Goals". |
| | 158 | |
| | 159 | |
| | 160 | Now, the above ideas have been confronted to some actual coding and I'm getting to the point of having a working prototype (wiki and attachment module mostly done, others on the way). |
| | 179 | In case there's no read permission for that resource, `None` is returned instead of an object. |
| | 180 | |
| | 181 | Alternatively, the equivalent to `req.perm.require('wiki', 'WikiStart')` can be achieved using: |
| | 182 | {{{ |
| | 183 | #!python |
| | 184 | page = req.perm.assert_resource('wiki', 'WikiStart') |
| | 185 | }}} |
| | 186 | Here, in case there's no read permission, a `PermissionError` exception is raised. |
| | 187 | See [#osimons osimons] point 2. |
| | 188 | |
| | 189 | Note that if for some reason there's really a need to create a Resource object without the proper credentials, it's of course possible to do so, using directly the `Resource` constructor - simply it's not the "advised" way. |
| | 190 | |
| | 191 | In addition to the above, it should also be possible to duplicate resource objects using convenient ways (e.g. `page(version=req.args.get('version'))` in order to get a specific version of an already available resource). When doing this, the permissions are checked again for the newly created resource, and `None` is returned if there's no read permission. The alternative `copy()` method can be used if one would like to trigger a `PermissionError` instead ''(or should that be `assert_copy()` to mirror `assert_resource()`?)''. |
| | 192 | |
| | 193 | |
| 177 | | wiki = req.perm.resource('wiki') |
| 178 | | }}} |
| 179 | | Note that from this point, the Request object is put out of the picture for permission checks and is not otherwise used by resources.[[br]] |
| 180 | | (See osimons point 1. and cmlenz first comment and paragraph in "Goals") |
| 181 | | If the req.perm credentials don't provide the <REALM>_VIEW permission for the resource, it is simply not possible to build a `Resource` instance for the specified resource and the call will fail, triggering a `PermissionError`. (See osimons point 2.)[[br]] |
| 182 | | There's an alternate method, which can be used when one does not want to get a `PermissionError`, which will simply return `None` if there's no permission to access the resource (the "get" is reminiscent of the dictionary lookup get). |
| 183 | | {{{ |
| 184 | | #!python |
| 185 | | wiki = req.perm.get_resource('wiki') |
| 186 | | if wiki: |
| 187 | | # do something with it |
| 188 | | }}} |
| | 197 | wiki_realm = req.perm('wiki') |
| | 198 | }}} |
| | 199 | |
| 196 | | Any kind of primary resources could be created using a `PermissionCache`, obtained either from `req.perm` or `<someresource>.perm`, but in some cases there are even more convenient ways to create a new resource.[[br]] |
| 197 | | For example, one can create a resource by copying an existing one. This is convenient when one has only to change the id or the version of an existing resource identifier. So when staying within the same realm, resources can be created using the copy() method: |
| 198 | | {{{ |
| 199 | | #!python |
| 200 | | |
| 201 | | versioned_page = page.copy(version=version) |
| 202 | | }}} |
| 203 | | Like for resource(), when dealing with a large number of resources one could use an alternative "get" form, in order to get the authorized resources without putting exceptions into play: |
| 204 | | {{{ |
| 205 | | #!python |
| 206 | | pages = [] |
| 207 | | for name in WikiSystem(env).get_all_pages(prefix): |
| 208 | | page = wiki.get_copy(id=name) |
| 209 | | if page: |
| 210 | | pages.append(page) |
| 211 | | }}} |
| 212 | | The `get_copy(id)` method will return `None` if there's no access right to resource with the given id.[[br]] |
| | 207 | ''('''Note:''' maybe we could even not expose the `perm` property, and write the above like that:)'' |
| | 208 | {{{ |
| | 209 | page.require('WIKI_CREATE') |
| | 210 | if 'WIKI_ADMIN' in page: |
| | 211 | continue |
| | 212 | }}} |
| | 213 | |
| 216 | | attachment = parent.get_child('attachment', filename) |
| 217 | | }}} |
| 218 | | |
| 219 | | After a few iterations of the prototype implementation, I finally came to something very similar to the `IResourceManager` interface from '''Context Factories and Subclasses''' chapter above, mainly for efficiency concerns and simplicity of the implementation. But for convenience purpose, the `Resource` class has still the `name`, `shortname` and `summary` properties, as well as a `model` property and an `url(href)` method. Those properties and method simply delegate the actual work to the resource's manager (i.e. the Component implementing the `IResourceManager ` interface): |
| | 217 | attachment = parent.child('attachment', filename) |
| | 218 | }}} |
| | 219 | ''('''Note''': this will probably soon become `child()/assert_child()` for symmetry with the other methods)'' |
| | 220 | |
| | 221 | After a few iterations of the prototype implementation, I finally came to something very similar to the `IResourceManager` interface described in the [#ContextFactoriesandSubclasses Context Factories and Subclasses] section above, mainly for efficiency concerns and simplicity of the implementation. But for convenience purpose, the `Resource` class has still the `name`, `shortname` and `summary` properties, as well as a `model` property and an `url(href)` method. Those properties and method simply delegate the actual work to the resource's manager which is the Component implementing the `IResourceManager ` interface which has claimed to manage the resource's realm (see `get_resource_realms()` below): |
