Hi Christopher, It looks like we're not that close to reach an agreement, after all... So, first, let me answer to the different points you raised, about:
A. The fine grained permissions B. The rendering context C. The resource and model Then, I'll propose a way forward. A. The fine grained permissions On Oct 5, 7:10 pm, Christopher Lenz <[EMAIL PROTECTED]> wrote: > To quote: > > "As fine-grained security and permission checks are becoming the > rule in Trac, one shouldn't be able to access a resource without the > appropriate credentials." > > I *strongly* disagree with this. That *may* be something to think > about for a future release, but most definitely not for 0.11. Well, there are compelling arguments in favor of doing it this way, as brought up by osimons in http://trac.edgewall.org/wiki/TracDev/ContextRefactoring#osimons. Excerpt: "A ticket today can be accessed through navigation/request to ticket, queries, macros, links and countless other ways. In each and every instance I need to remember to check permissions. Always. And in the same manner. Some sort of custom ticket box macro that gets this wrong will spill all the beans." > And the whole concept of "resources have to be created through the > permission system" is wrong IMO. "wrong"? - well, not according to the reasoning above. Having to go through the permission system for handling resources has two great benefits: - it enables us to have /complete/ coverage of the permission checks, e.g. it can't be partially done in the ticket module and not done in some area of the milestone or query module. - it helps to ensure that any external plugin will follow the same rules (they won't be able to accidentally forget a permission check) Of course, the data model layer could still be accessed directly for now, so that would be a way to bypass the permission checks. But that part can be covered later (for Trac 0.12) if we pass the permission cache down to the model, in the IResourceManager.\ get_model(resource) call. That will make it possible to have the data model layer perform itself the relevant checks for other permissions (TICKET_MODIFY in the Ticket.save_changes, TICKET_CREATE in Ticket.insert, etc.). Therefore, I think this is a coherent and future-proof approach. > But that's not the point. The point > is that we should not be thinking about doing anything like that for > 0.11. Both Alec and myself have stated early on that we think this > was out of scope for the release. I think that moving the permission checks down to the data model layer is something that can be done after 0.11, yes. However, I do think that providing an API for ensuring that all accesses to the resources are subject to fine grained permission checks is precisely what we wanted to have for 0.11. > My idea (and I think I'm not alone here) was that we should go back > and fix Context by removing/simplifying. What you're proposing here > seems to go in a totally different direction. If by "fix Context by removing/simplifying", you mean that we should remove all the enhancements brought by introducing more support for generic resources in Trac, then I think it's more a conflict of vision. More on this below in B. and C. If you're talking about accessing the resources from the permission system, then you're right, it's a different direction than the one taken earlier during 0.11dev, but it's simply because no one thought about that idea before. As we're talking about introducing a new API anyway, I don't see the problem about doing things that way if there are compelling reasons for it. The old way of performing the global checks still works unchanged and the proposed one is /very/ close to what Alec did: req.perm('wiki', 'WikiStart') will simply return a Resource or None, instead of /another/ throw-away PermissionCache object only used for performing the checks. B. The rendering context > > On Sep 27, 7:15 pm, Christopher Lenz <[EMAIL PROTECTED]> wrote: > Anyway, I strongly doubt we'll need to identify the "resource/ > context" in which some other resource is being rendered. The only use > case I can think of are relative links (as in ../foo instead of /bar/ > foo), and those are more trouble than they're worth. That's not to > say that the idea of a "rendering context" isn't needed, just that > that context doesn't need to know about the (pseudo-)resource in > which it's being used. Right now, the rendering context needs to know about the current resource for the following reasons: - rendering relative links (./Child, ../Sibling and #Anchor) - access to the resource (and therefore the model if needed) when expanding a macro without the need to re-parse the req.path_info again (like the current hack in the Image macro), therefore removing one dependency on the request in the rendering subsystem - avoid to do "recursive" rendering when embedding one content in another (e.g. currently possible within a ticket description if it contains a ticket query in verbose table mode which matches that ticket) - retrieve the resource and corresponding model within the blame annotator when rendering source content I probably forget some. Besides, this is also laying ground for future improvements, especially concerning relative links: - All TracLinks will in the future be "relative" links, when multiple project comes into play. InterTrac links are the only ones who specify explicitly a project, all the other ones are implicitly referring to resources belonging to the same project as the currently rendered resource (i.e. the context.resource). - Even without multi-project support, simple Wiki page name links could eventually become relative ones - first look up a page name at the current level of the hierarchy, if not found, lookup above. (./ThatPage would always refer to the current level and prevent the lookup at the above level) - For source and changeset commenting, being able to easily reference line numbers will be very useful - The blame annotator which currently only work for source resources could be extended to /any/ Trac resources provided we add some kind of uniform versioning API on the resources. This is certainly doable for the Wiki pages right now, and for tickets as well at the cost of some complexity, until an unification is done at the data layer model at which point it will become possible for /any/ kind of resource. - More generally, the rendering context will be used to progressively get rid of the Request object in the rendering layer, and be used to simplify the mimeview API Those points correspond to issues raised in numerous tickets on trac.edgewall.org, tickets to which numerous users contributed and for which I proposed a solution. > >> Furthermore, I think the argument that we'd need to be creating and > >> manipulating these things in a generic way very often, which would > >> demand a more convenient construction/manipulation API, is also > >> incorrect AFAICT. In many cases, we already have a model object at > >> hand, and doing something like ticket.identifier to get the resource > >> identifier is as convenient as it gets. > > > Well, data model instance creation is costly especially in the > > timeline event providers and search providers, but also in report and > > query results. > > Well, elsewhere in my mail I said that there should be ways to get > display_name etc without going through a DB lookup. Just adding a > get_resource_info() method (or similar) to the IResourceManager > interface I proposed would do the trick, while still not requiring > the use of resource "descriptors" as opposed to simple identifiers. This is mainly a matter of convenience and future extensibility. We could of course have a `resource_url(resource_id, href)` function instead of a `resource.url(href)` method, but I think we will have more and more code working at the generic level, and this convenience will pay off then (I think it already does). It is true that /when/ we have the model around, we already have some kind of facilities to access the id, but not in an uniform way: ticket.id for tickets, page.name for wiki pages. And we don't even have a way to get the realm information! We /do/ have some modules that specifically handle /any/ resources: those are the timeline and search modules (and more recently the attachment module). However they have the advantages of being "core" or "well-known" modules, so that it's the responsibility of resource managers to implement the interfaces offered by those modules. But what if you want to provide as a plugin another module that handles resources in a generic way? Even if you could write adapters for the core/well-known resources, you can't expect to cover all kind of resources this way. Also, a lot of Trac internals could be greatly simplified if there would be more ways to handle the resources in generic ways: wiki editing and versioning, custom fields, comments, adding more kinds of resource (like component, project and repository resources to name a few) without having to duplicate code to handle them. So I clearly don't buy the argument that we don't need to manipulate the resources in a generic way very often... Therefore, the "Resource" are for convenient handling of any kind of resources, and the resource.model gives access to the specific details. There's also a clear goal of unifying the underlying models even more (generalized fields, generalized workflows, generalized content versioning, etc.). The Resource class could then be expanded as needed, like for providing the "fields" types or property descriptions which are appropriate for the realm. The above is clearly the vision for Trac's future to which I adhere fully. By my experience of user feedback, on trac-users and tickets on trac.edgewall.org, I think this is a vision that is largely shared, or at the very least, which will be very well received when it will come through. The way forward? I propose to create a sandbox/context-split branch, where I finalize my current version of the changes. Most of the work is already done (about 1700 "hunks"), there's no "Context" class left, it works reasonably well and there's full coverage for fine grained permissions on all the modules. /If/ we decide to go for tuple-based resource identifiers, using that branch as a basis will make the work significantly simpler than starting again from trunk. I'd very much appreciate if we could go on discussing the points A, B, and C. above, as one of us will eventually manage to convince the other, but I do realize how it's sometimes difficult and frustrating to see that the other doesn't just "get it"... (and I'm sure that feeling /is/ shared ;-) ). I'm just afraid that the situation will repeat itself for 0.12, as from what I understand, you're probably opposed to some other major design proposals of mine, like the generic Trac stuff. This will then lead to the question of which approach will the project take, the one I defend or the one you defend (which, I admit, I know little about). -- Christian --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Trac Development" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/trac-dev?hl=en -~----------~----~----~----~------~----~------~--~---
