I've recently (re)started work on the security branch [1], using Christian's recent Context additions. Given that the Context contains all of the information necessary to enforce a policy decision, I think this is a good approach.
In the security sandbox I've made a couple of minor changes and would like to make a few more.
What I've done so far, and why:
- Added a trac.resource module - Moved trac.wiki.api.Context to trac.resource. I did this because I think the Context is more generalised, and some auxiliary interfaces/components are required which don't make sense being in trac.wiki. - Created a trac.resource.IResourceMapper interface for mapping between resources and contexts. - Added the trac.resource.ResourceSystem component for inspecting and converting resources. - Added a trac.perm.IPermissionPolicy interface for making security policy decisions, and converted PermissionSystem and PermissionCache to use this. - Added convenience classmethod Context.from_resource(env, req, resource) for creating a Context from a resource, such as a WikiPage object (actually anything that can be identified via IResourceMapper).
- Added Context.object, which lazily fetches the actual object referred to by the Context (I'd like to rename this from "object" to "resource" - more info further down). - Added Context.perm which is a PermissionCache object (same as req.perm) but specific to the current context. eg.
if 'WIKI_MODIFY' in context.perm: ...
The context object is passed to template pages, where appropriate, and can be used to control access:
<py:if test="'WIKI_ADMIN' in context.perm"> ... <py:/if>
So far I've only converted the Wiki module, but it is working perfectly so far.
Changes I would like to make:
- Rename the "resource" member of Contact to "realm" [2]. "resource" currently refers to "wiki", "ticket", etc. I think it should refer to the actual object identified (eg. WikiPage(env, 'WikiStart')). - Merge into trunk. - Continue converting modules.
If amenable, I'd like to merge the branch into trunk as it is currently.
While I haven't actually tested the code, I'd love to see the security branch be merged sooner, rather than later, as I am chomping at the bit for the functionality.
If I get some time, I'll try to test out the branch.
Alec Thomas wrote: > I've recently (re)started work on the security branch [1], using > Christian's recent Context additions. Given that the Context contains all > of the information necessary to enforce a policy decision, I think this > is a good approach.
> In the security sandbox I've made a couple of minor changes and would > like to make a few more.
> What I've done so far, and why:
> - Added a trac.resource module > - Moved trac.wiki.api.Context to trac.resource. I did this because > I think the Context is more generalised, and some auxiliary > interfaces/components are required which don't make sense being in > trac.wiki.
Well, I also originally had a trac.context which contain this class, but I didn't have a use case for the Context class besides the wiki formatting. But I agree the idea is more general, so I'm all for that change.
Note that the Context is the kind of "resource descriptor" object I was looking for to complement the resource itself. In the original TracObject idea (or even its implementation in the xref branch (1)), both the model part and the description part were mixed in the same class (the "cheap" or "complete" TracObject distinction of r2278).
Now the Context is a way to address only the "description" part of the resource, not bothering about its actual data model. I hope this addresses the heavy reluctance that some had to make the resource itself aware of the "req"uest object (at least jonas is OK with that). With your proposed change, the context is now able to retrieve its corresponding data model object (ctx.object for now, soon to be renamed ctx.resource) and from the data model, it's possible to build a descriptor for it (adding the req info), so that's very good.
Let me first continue by quoting the last part of your mail:
> Changes I would like to make:
> - Rename the "resource" member of Contact to "realm" [2]. "resource" > currently refers to "wiki", "ticket", etc. I think it should refer > to the actual object identified (eg. WikiPage(env, 'WikiStart')).
+1, that could be done ASAP on trunk, together with the move to trac.resource, I think.
> - Merge into trunk.
Well, I see a few things that could be improved upon, and maybe done differently before that happens.
> - Continue converting modules.
In particular, I'd like to see how we'll convert the attachment module, in a way that would allow generic attachments (see #3068). This shouldn't require any additional interface (i.e. /not/ the IAttachmentPointProvider way) but should result from the ability to add attachment to any kind of resource (this doesn't mean we'll add an UI to do this for every kind of resource!).
Now about the few modifications to what you've already done:
> - Created a trac.resource.IResourceMapper interface for mapping > between resources and contexts. > - Added the trac.resource.ResourceSystem component for inspecting and > converting resources.
Note that most of the time we /already/ have the resource at hand when building the Context, so care must be taken to provide it to the context when possible (as you already do in wiki/web_ui.py), and to keep it while creating similar contexts (i.e. ctxt(abs_urls=True)).
> - Added a trac.perm.IPermissionPolicy interface for making security policy > decisions, and converted PermissionSystem and PermissionCache to use this. > - Added convenience classmethod Context.from_resource(env, req, > resource) for creating a Context from a resource, such as a WikiPage > object (actually anything that can be identified via IResourceMapper).
... or simply `context = page.get_context(req)`, instead of the more complex hops through the IResourceMapper implementations which will end in the `identify_resource(req, resource)` of the module?
> - Added Context.object, which lazily fetches the actual object > referred to by the Context (I'd like to rename this from "object" > to "resource" - more info further down). > - Added Context.perm which is a PermissionCache object (same as > req.perm) but specific to the current context. eg.
> if 'WIKI_MODIFY' in context.perm: > ...
> The context object is passed to template pages, where appropriate, > and can be used to control access:
> <py:if test="'WIKI_ADMIN' in context.perm"> > ... > <py:/if>
Ok. Also, what about allowing to check for "unqualified" permission names? E.g.<py:if test="'ADMIN' in context.perm"> which would translate to WIKI_ADMIN if context's realm is 'wiki', TICKET_ADMIN if the context's realm is 'ticket', etc.
On Thu, Jan 11, 2007 at 05:36:01PM +0100, Christian Boos wrote: > +1, that could be done ASAP on trunk, together with the move to > trac.resource, I think.
Excellent.
> In particular, I'd like to see how we'll convert the attachment module, > in a way that would allow generic attachments (see #3068). This > shouldn't require any additional interface (i.e. /not/ the > IAttachmentPointProvider way) but should result from the ability to add > attachment to any kind of resource (this doesn't mean we'll add an UI to > do this for every kind of resource!).
I agree with the idea, but I don't think this need be done before merging the branch. This is functionality additional to the main security features. That being said, I'd be happy to work on this after the merge, as it would be very useful. Or maybe Noah could? He's been keen for this feature for some time I believe?
I think the main difficulty will be the permissions. TICKET_APPEND is required for tickets, while WIKI_MODIFY for wiki pages. I guess we can maybe just assume that every other object requires `'MODIFY' in context.perm`.
> Note that most of the time we /already/ have the resource at hand when > building the Context, so care must be taken to provide it to the context > when possible (as you already do in wiki/web_ui.py), and to keep it > while creating similar contexts (i.e. ctxt(abs_urls=True)).
100% agreed.
> > page = WikiPage(env, 'WikiStart') > > context = Context.from_resource(env, req, page) > ... or simply `context = page.get_context(req)`, instead of the more > complex hops through the IResourceMapper implementations which will end > in the `identify_resource(req, resource)` of the module?
The only real reason I didn't do that was that it would ideally be in a base "Trac Object" and, if we have that, lots of other stuff could/should go in it, just like your old TracObject branch. I don't really have a problem with adding it, but...
> Ok. Also, what about allowing to check for "unqualified" permission names? > E.g.<py:if test="'ADMIN' in context.perm"> which would translate to > WIKI_ADMIN if context's realm is 'wiki', TICKET_ADMIN if the context's > realm is 'ticket', etc.
Agreed. Good idea, and this could be the first step towards getting rid of the current permission naming system entirely.
-- Evolution: Taking care of those too stupid to take care of themselves.
On Fri, Jan 12, 2007 at 09:56:57AM +1100, Alec Thomas wrote: > > > page = WikiPage(env, 'WikiStart') > > > context = Context.from_resource(env, req, page) > > ... or simply `context = page.get_context(req)`, instead of the more > > complex hops through the IResourceMapper implementations which will end > > in the `identify_resource(req, resource)` of the module?
> The only real reason I didn't do that was that it would ideally be in a > base "Trac Object" and, if we have that, lots of other stuff could/should > go in it, just like your old TracObject branch. I don't really have a > problem with adding it, but...
I had a look at this, and I'd like to understand the actual objections that were raised regarding TracObjects. To me it makes sense to be able to obtain information about a Trac resource from the resource itself. In fact, with the current Context object you could almost rename "Context" to "Resource" and do this:
Alec Thomas wrote: > On Fri, Jan 12, 2007 at 09:56:57AM +1100, Alec Thomas wrote: >>>> page = WikiPage(env, 'WikiStart') >>>> context = Context.from_resource(env, req, page) >>> ... or simply `context = page.get_context(req)`, instead of the more >>> complex hops through the IResourceMapper implementations which will end >>> in the `identify_resource(req, resource)` of the module? >> The only real reason I didn't do that was that it would ideally be in a >> base "Trac Object" and, if we have that, lots of other stuff could/should >> go in it, just like your old TracObject branch. I don't really have a >> problem with adding it, but...
> I had a look at this, and I'd like to understand the actual objections > that were raised regarding TracObjects. To me it makes sense to be able > to obtain information about a Trac resource from the resource itself. In > fact, with the current Context object you could almost rename "Context" > to "Resource" and do this:
> class WikiPage(Resource): > ... snip of a TracObject rewrite ;)
Well, I'm not sure if I'm the best person to advocate the objections against the TracObject/Resource idea, but here's my take.
The main objection as I understood it was that it was bad design to make the data model layer aware of the web layer (the req).
A second issue is that it is somewhat costly to build a data model object. In my early attempt, I distinguished between a loaded/unloaded TracObject, but it was somehow cumbersome to use right. I much prefer the (upcoming) distinction between (1) a Context, which acts as a kind of resource descriptor and also knows about the /context of use/ of that resource (e.g. the ''parent'' context) and (2) a Resource, which is the data model for that resource.
That way it's always clear what is what, and you can go easily from the "cheap" resource descriptor to the actual resource object, and back.
Now another strong point of the TracObject proposal was the possibility to override some base methods. I think the Context is amenable to that as well. Take the example of a "title" method that could be used to fill title attribute of a link to a given resource. That would be "<realm>:<id>" in the general case. But for tickets, we'd like to have something like "<ticket summary> (<status>[:<resolution if status=='closed'])". This of course would require to get access to the data model.
This could be implemented like this:
class TicketContext(Context):
def title(self): status = self.resource['status'] if status == 'closed': status += ':' + self.resource['resolution'] return "%s: (%s)" % (self.resource['summary'], status)
In the above, self.resource is accessed many times transparently. As a context is able to "cache" its data model object, it's not a problem, the access to the complete representation can be done efficiently and "on demand".
That also means that for a given realm, we need a mechanism to create the appropriate Resource subclass and the appropriate Context subclass. In my previous mail, I said that for getting a context from a resource, I wanted to use a Resource.get_context() method instead of going through the ResourceSystem. But after experimenting a bit, it seems as easy to use a helper function to achieve that, and this is less susceptible to be hit by the "mixing of layers" argument!
So this results in the following:
class IResourceMapper(Interface): """Map between Trac resources and their contexts."""
class ResourceSystem(Component): resource_mappers = ExtensionPoint(IResourceMapper)
def create_context(self, req, realm, *args, **kwargs): if not self._context_map: # build a dict of realm keys to Context subclasses values context_class = self._context_map.get(realm, Context) return context_class(self.env, req, realm, *args, **kwargs)
def fetch_resource(self, context): if not self._resource_map: # build a dict of realm keys to Resource subclasses values resource_class = self._resource_map.get(realm) if resource_class: return resource_class(self.env, realm, context) # Note that passing a context object to the Resource constructor is arguable: # (+) constructor can fetch additional info if needed (e.g. version) # (-) Resource should never store references to the Context, to avoid # circular refs. So ideally, Resources shouldn't know about Contexts
class Context(object): """Base class for resource descriptors.""" ... # now, creating a sub-context (__call__) needs to be modified to use the # ResourceSystem as well, in order to be able to create instances of the # appropriate subclass
class Resource(object): """Base class for resources."""
realm = 'unknown' # needs to be overridden
# Note that an `.id` property needs to be set for the instances as well
And here's the helper function I was talking about:
Well, that's was too much coding in Thunderbird, let's do it for real ;)
So again, about the merge in trunk: - I'd like we sort out the above first - I'd like to merge my 3 other branches (blame and the 2 -tmp ones) before ;)
A quick follow-up, I've implemented the above ideas in r4556 (which also finishes the renaming started in r4552). This also reverted r4551 for the reasons explained in the previous mail.
Christian Boos wrote: > ... > class Resource(object): > """Base class for resources."""
> realm = 'unknown' # needs to be overridden
> # Note that an `.id` property needs to be set for the instances as well
Of course, at this point, no real Resource class is actually needed. This will probably change once we start with the GenericTrac stuff, but for now all what is required are the additional realm and id properties in each data model class.
> And here's the helper function I was talking about:
... and since then, I've discovered Context.from_resource(), which can perfectly be used instead of the above ;)
One thing I'm not totally pleased with is the way you changed the PermissionCache. Is there any reason for not having a permission cache at the level of a request?
On Fri, Jan 12, 2007 at 08:03:47PM +0100, Christian Boos wrote:
> A quick follow-up, I've implemented the above ideas in r4556 (which also > finishes the renaming started in r4552). > This also reverted r4551 for the reasons explained in the previous mail.
Excellent. Looks good to me :)
> ... and since then, I've discovered Context.from_resource(), which can > perfectly be used instead of the above ;)
Aha. I wondered about that...
> One thing I'm not totally pleased with is the way you changed the > PermissionCache. Is there any reason for not having a permission > cache at the level of a request?
This was a deliberate omission. The caching mechanism was left out while the design was in flux and was to be added later once stabilised.
-- Evolution: Taking care of those too stupid to take care of themselves.
On Thu, Jan 11, 2007 at 05:36:01PM +0100, Christian Boos wrote: > In particular, I'd like to see how we'll convert the attachment module, > in a way that would allow generic attachments (see #3068). This > shouldn't require any additional interface (i.e. /not/ the > IAttachmentPointProvider way) but should result from the ability to add > attachment to any kind of resource (this doesn't mean we'll add an UI to > do this for every kind of resource!).
After contemplating this for a while, I think the most elegant solution is to add an _ATTACH permission. This makes more sense anyway, IMO, as attaching something to an object is distinct from MODIFYing it.
So there could then be WIKI_ATTACH, TICKET_ATTACH, MILESTONE_ATTACH, etc.
----------- Future rant -------------
Once the permissions are more uniform, we can begin removing the reliance on having a permission for each action on each object and move to a small set of permissions:
VIEW CREATE DELETE MODIFY APPEND ATTACH GRANT (maybe? for allowing a user to grant other users non-ADMIN permissions) ADMIN (all of the above)
Here's a patch: http://swapoff.org/files/new-permissions.diff. This converts the wiki module to the new permission system, but also adds a LegacyPermission component for backwards compatibility, which simply maps all the new style permissions with `realm.upper() + '_' perm`.
The only slight "wart" is that the module itself defines the permissions, even of things like ATTACH which perhaps should be defined in other modules, but perhaps not. I think this is difficult to avoid though, because the attachment module and its parent are co-reliant.
-- Evolution: Taking care of those too stupid to take care of themselves.
> So again, about the merge in trunk: > - I'd like we sort out the above first > - I'd like to merge my 3 other branches (blame and the 2 -tmp ones) > before ;)
And after that, maybe there should be some effort towards releasing 0.11? It seems merging these branches pretty much fixes all of the goals stated for 0.11 in the roadmap...
On Sat, Jan 13, 2007 at 07:31:08PM +1100, Alec Thomas wrote: > Once the permissions are more uniform, we can begin removing the > reliance on having a permission for each action on each object and move > to a small set of permissions:
> VIEW > CREATE > DELETE > MODIFY > APPEND > ATTACH > GRANT (maybe? for allowing a user to grant other users non-ADMIN permissions) > ADMIN (all of the above)
> The only slight "wart" is that the module itself defines the > permissions, even of things like ATTACH which perhaps should be defined > in other modules, but perhaps not. I think this is difficult to avoid > though, because the attachment module and its parent are co-reliant.
To make this work we'll need to have "sub-realms" for each realm that supports attachments.
eg.
wiki wiki//attachment ticket//attachment
(or whatever syntax is decided on for separating contexts/realms)
That way a user can individually grant attachment permissions on different parent objects. Actually, thinking about it, this will alleviate the need for an ATTACH permission altogether, as "CREATE" in the wiki:attachment security realm would provide this permission.
-- Evolution: Taking care of those too stupid to take care of themselves.
Manuzhai wrote: > On 1/12/07, Christian Boos <cb...@neuf.fr> wrote:
>> So again, about the merge in trunk: >> - I'd like we sort out the above first >> - I'd like to merge my 3 other branches (blame and the 2 -tmp ones) >> before ;)
> And after that, maybe there should be some effort towards releasing > 0.11? It seems merging these branches pretty much fixes all of the > goals stated for 0.11 in the roadmap...
Not really... there's still:
* Use setuptools for setup (see [source:sandbox/setuptools]) * ''Refactoring of the Mimeview API (see #3332, [source:sandbox/mimeview])'' * ''Flexible/extensible ticket workflow (see WorkFlow, [source:sandbox/workflow])'' * ''Refactoring of the Wiki Engine to use the Genshi builder API (''experimental'', cboos)''
1. is really mandatory. 2. is not a big deal, I think 3. is a somewhat bigger deal, agreed 4. likewise, still not started. Not /that/ mandatory for 0.11 but would be nice.
Besides those stated broad goals, there are lots of worthy tickets still opened, not to mention those for 0.10.4 ...
Not to mention that thanks to the rework of the PermissionPolicy, interesting things are happening in the resource/Context area, which is a kind of "unlocker" for lots of long standing issues ...
> * Use setuptools for setup (see [source:sandbox/setuptools]) > * ''Refactoring of the Mimeview API (see #3332, > [source:sandbox/mimeview])'' > * ''Flexible/extensible ticket workflow (see WorkFlow, > [source:sandbox/workflow])'' > * ''Refactoring of the Wiki Engine to use the Genshi builder API > (''experimental'', cboos)''
> 1. is really mandatory. > 2. is not a big deal, I think > 3. is a somewhat bigger deal, agreed > 4. likewise, still not started. Not /that/ mandatory for 0.11 but would > be nice.
With regards to setuptools, I agree that it needs to be merged first; I just always get confused when there's a notice about a merge (whether it is about merging the branch up to latest trunk or merging the branch *to* the trunk). I agree that the mimeview stuff should also be in there, it seems (though sandbox/mimeview doesn't exist?). It seems to me, though, that the workflow part, while nice, will take to long to finish before 0.11. I have no clue how hard your number 4 is, but it seems like it's not that essential.
> Besides those stated broad goals, there are lots of worthy tickets still > opened, not to mention those for 0.10.4 ...
> Not to mention that thanks to the rework of the PermissionPolicy, > interesting things are happening in the resource/Context area, which is > a kind of "unlocker" for lots of long standing issues ...
Well, alright, but I think now would be a good time to start triaging tickets for 0.11. Pretty much all tickets are now targeted for either 0.10.4 or 0.11, and I think it would be a good time to start moving most of those over to 0.12, except for the ones that are actually viable for 0.11. I also think 0.11 is in a pretty good state right now, and while it may be a little low on user-facing features (Genshi and Pygments are interesting to developers, but not so much for users of the system; WebAdmin was already there, setting your timezone is not that big of a deal for most people, I presume -- but the blame support is pretty cool), I think it would make for a good incremental release kind of thing.
Manuzhai wrote: > ... > I just always get confused when there's a notice about a merge > (whether it is about merging the branch up to latest trunk or merging > the branch *to* the trunk).
A notice on the mailing list or in changesets? When we mention a merge here, it's always about merging the feature in trunk, as the other way round is just technical to keep a branch current, it's never mentioned here. The latter changesets are always labelled "<branch name>: merged trunk" or trunk replaced with the appropriate log:trunk@from:to TracLinks.
> It seems to me, though, that the workflow part, while nice, will take > to long to finish before 0.11.
Probably, yes.
> I have no clue how hard your number 4 > is, but it seems like it's not that essential.
Ok, but we'll see if the mimeview fixes will eventually trigger something to happen there ...
> ... I think it would be a good time to start moving > most of those over to 0.12, except for the ones that are actually > viable for 0.11.
I see this to happen next month maybe, not now.
> I also think 0.11 is in a pretty good state right > now, and while it may be a little low on user-facing features (Genshi > and Pygments are interesting to developers, but not so much for users > of the system;
Not true for Pygments; 1) it's easy to install compared with the other highlighters 2) it's much more powerful. 3) the user can select the rendering style she likes most...
> WebAdmin was already there, setting your timezone is > not that big of a deal for most people, I presume
A related change I'd like to see would be the ability to select the way date and time are displayed.
> -- but the blame support is pretty cool), I think it would make for a > good incremental > release kind of thing.
Exactly, I think that with this we have a good incentive for people to upgrade, even if the workflow is _still_ not there. The features in the other pending branches will also provide a few nice user-visible features: - timeline-link-resolver adds links back to the timeline from most displayed dates and ages - property-renderers adds the ability to implement custom property renderers, with some useful default implementations (wiki renderer, svn renderer for svn:externals and svn:needs-lock) - last but not least, the security branch will also bring/enable a lot of enhancements
Alec Thomas wrote: > ... >> because the attachment module and its parent are co-reliant.
> To make this work we'll need to have "sub-realms" for each realm that > supports attachments.
> eg.
> wiki > wiki//attachment > ticket//attachment
> (or whatever syntax is decided on for separating contexts/realms)
> That way a user can individually grant attachment permissions on > different parent objects. Actually, thinking about it, this will > alleviate the need for an ATTACH permission altogether, as "CREATE" in > the wiki:attachment security realm would provide this permission.
Using the 'VIEW', 'CREATE' and 'DELETE' permission actions for the attachment itself works quite well, given that a context for an attachment is always parented in another context (of a wiki page, a ticket, ...).
I wanted to be sure that the approach we were taking in that branch could be extended to attachments, and this seems indeed to be the case. It's also quite trivial to add security policies for things that are not really Trac resources, as it's easy to create contexts for anything (see e.g.[4599]).
So I think the original goals of the PermissionPolicy plan can be fulfilled, except for the minimal impact on the API part ;) The current API looks quite fine to me, and behaves quite well: for example, when some attachments are protected, they can't be seen in the timeline, same for private tickets (not showing up in searches, shown as missing when written as TracLinks), etc.
Finally, the permission cache has been set back to the level of the request, where it belongs (see r4601).
I think it's a good time to get some more feedback on the branch.
Please give me a couple more days to review and provide feedback on that branch. I'm not yet entirely sure I like a number of aspects of the design. Some of my concerns are actually about the wiki-context stuff that trac.resource is based upon... I need to catch up a bit, here.
> That way a user can individually grant attachment permissions on > different parent objects. Actually, thinking about it, this will > alleviate the need for an ATTACH permission altogether, as "CREATE" in > the wiki:attachment security realm would provide this permission.
This could immediately provide a lot of workflow functionality, simply be declaring that assigning a ticket to a milestone requires ATTACH permission on the milestone, and the same for components, resolutions (WONTFIX/ASSIGNED/CLOSED/etc), and ticket owners.
Whether hiding the options any given user doesn't have permission for is feasible I don't know, doing that many permission checks would be expensive. But checking permissions when the user tries to make a change would still be very useful.
> Am 19.01.2007 um 22:41 schrieb Christian Boos: >> ... I'm now proposing to merge these changes to the trunk
> Please give me a couple more days to review and provide feedback on > that branch.
No problem.
> I'm not yet entirely sure I like a number of aspects of the design. > Some of my concerns are actually about the wiki-context stuff that > trac.resource is based upon... I need to catch up a bit, here.
Then be sure to read:
- http://trac.edgewall.org/wiki/WikiContext, for the design of the Context w.r.t. wiki processing. Basically, it's a way to keep all the information needed to generate correct output in one place. One aspect that I still want to refine are the various wiki_to_ methods, which should be made more flexible. This improvement is related to the mimeview refactoring and the wiki engine refactoring (parsing/formatting split).
- http://groups.google.com/group/trac-dev/msg/44ebc5156ec6e7bc, where I try to explain the reticences against the TracObject ideas. I explain how the use of Context as "resource descriptor" can overcome the concerns about the separation of layers. That way, the data model objects don't need to know anything about the web front-end layer or the presentation details.
> Christopher Lenz wrote: >> I'm not yet entirely sure I like a number of aspects of the design. >> Some of my concerns are actually about the wiki-context stuff that >> trac.resource is based upon... I need to catch up a bit, here.
> Then be sure to read:
> - http://trac.edgewall.org/wiki/WikiContext, for the design of the > Context w.r.t. wiki processing. Basically, it's a way to keep all the > information needed to generate correct output in one place.
Okay, I've been looking into the changes, so here's some first feedback/questions...
Why is it possible to have a context instance not associated with any resource? Why can a context be associated with just a realm but no particular resourse? Or: shouldn't a context always be connected to a resource?
self_href() is an *awful* name. Even after reading the docstring for a couple of times, I still don't really get why it's there and what it does. Looking at the invocations, there's apparently only *one* place that actually passes an argument to it, and that's in formatter.py. I can see how you chose the name because the Context.href() slot is already occupied; but why do we need a "global" href object attached to the context?
Why is local_url() defined on the Context class? It's only used by formatter.py, so why not keep it there? It's just about how external links are rendered, it shouldn't concern anyone outside the formatter.
What's the reason for moving the DB connection handle from the formatter to the context?
Re: Context.__call__, is there really ever a need to *copy* a Context? I.e. why not just return `self`? Talking about __call__(), I think code creating sub-contexts isn't all that readable. Using a different, more explicit way to create a subcontext might be better.
And a more fundamental kind of question: is it really necessary that the Context knows the req (and abs_urls, by extension)? I.e. why must a Context be able to produce links? Couldn't we just have continued to pass the req to e.g. render_macro() in addition to the context? I feel like I'm missing a fundamental point here.
My general feeling is that the Context class has too much responsibility. When we talked about the design before you started working on it, *my* impression was that a wiki-context should encapsulate information about the resource to which the text-being- wikified belongs. Your idea was obviously different: "A Wiki rendering context contains all the information needed for properly rendering Wiki text". That's a whole lot broader.
I'm not saying that the design is "wrong", but in general I prefer functions/classes that have clear focus and responsibility. Trying to grok the scope of the wiki-context changes today, I couldn't help but feel that it introduced a certain level of "spaghettification". So I'd like to understand more about *why* the design looks what it looks like.
> One aspect > that I still want to refine are the various wiki_to_ methods, which > should be made more flexible.
How so?
> This improvement is related to the > mimeview refactoring and the wiki engine refactoring (parsing/ > formatting > split).
Christopher Lenz wrote: > Am 20.01.2007 um 09:36 schrieb Christian Boos:
>> Christopher Lenz wrote:
>>> I'm not yet entirely sure I like a number of aspects of the design. >>> Some of my concerns are actually about the wiki-context stuff that >>> trac.resource is based upon... I need to catch up a bit, here.
>> Then be sure to read:
>> - http://trac.edgewall.org/wiki/WikiContext, for the design of the >> Context w.r.t. wiki processing. Basically, it's a way to keep all the >> information needed to generate correct output in one place.
> Okay, I've been looking into the changes, so here's some first > feedback/questions...
Before going into the details, let me clarify a bit more the overall design.
Initially, the main point of the Wiki context was to be able to generate proper Wiki content. So there are actually two "levels" or notions of context: - the context to which the wiki source belongs. This was needed for properly locating relative links. - the context of access to that text. This was needed to be able to generate proper links.
Then, as things went on, the two sides proved equally useful, and saw further uses. In the security branch, the "resource context" part evolved so that there are now a few methods to let the resources describe themselves (by the way of subclasses, when appropriate), and the "access context" part was improved to act as a proxy for permission requests. One could indeed think about splitting the two notions.
But let's first go through your questions.
> Why is it possible to have a context instance not associated with any > resource?
That's needed for creating a top-level access context. Creating sub-contexts from such a toplevel context makes it easy (and transparent) to create the proper subclass for the more specific context (security branch only).
Plus, a toplevel context is a convenient way to encapsulate the env, req and db parameters that are scattered through the code.
> Why can a context be associated with just a realm but no > particular resourse? Or: shouldn't a context always be connected to a > resource
This makes it easier to create a context for a specific resource in that realm, i.e.
context = parentcontext('attachment') for filename in ...: ctx = context(id=filename)
Another situation is when you want to refer to some general context which doesn't have clearly an "id" attached to it, for example: ctxt('query'). The id _could_ be the query string itself, but right now we don't really care. But the knowledge that we're in a sub-context of some 'query' can be useful (think about a TicketQuery with description, called from within a ticket description...).
> self_href() is an *awful* name. Even after reading the docstring for > a couple of times, I still don't really get why it's there and what > it does. Looking at the invocations, there's apparently only *one* > place that actually passes an argument to it, and that's in > formatter.py.
Agreed, I already wanted to change that a few times. 'resource_href' would probably be more appropriate. self_href (or resource_href) is used to produce the "canonical" URL for accessing the resource that the context describes. Also, I know the implementation is a bit messy, as it is now. I'll simplify that.
> I can see how you chose the name because the > Context.href() slot is already occupied; but why do we need a > "global" href object attached to the context?
This is related to the access context part of the context, and the possibility to set the context as absolute or relative (abs_urls=True). When one wants to produce an href, one would use context.href and it will be either a relative or an absolute href, as appropriate. This is similar to the use of formatter.href, but we don't always have a formatter at hand.
> Why is local_url() defined on the Context class? It's only used by > formatter.py, so why not keep it there? It's just about how external > links are rendered, it shouldn't concern anyone outside the formatter.
Yes, I originally thought it could have more uses, but finally it didn't, so I think we can put that back in the formatter.
> What's the reason for moving the DB connection handle from the > formatter to the context?
Because you often use a Context without a Formatter object, but when you use the formatter, you always have a Context.
> Re: Context.__call__, is there really ever a need to *copy* a > Context? I.e. why not just return `self`?
Well, it's precisely when we don't want to modify self. Say you have a Context for a Wiki page, ctx = context('wiki', 'ThePage'), then you create v1 = ctx(version=1), the Context for the version 1 of that page. You expect that ctx still refers to the latest version of the page, not that it suddenly points to version 1... The __call__ way makes it clear that it's a functional style. It can also be seen as very similar to a constructor call (and actually it is very similar).
> Talking about __call__(), I > think code creating sub-contexts isn't all that readable. Using a > different, more explicit way to create a subcontext might be better.
Well, you could always add an alias for it (subcontext? but then it's not always a subcontext...), but I personally find the __call__ notation to be very clear, like in, e.g. att = maincontext('wiki', 'WikiStart')('attachment', 'filename.txt').
> And a more fundamental kind of question: is it really necessary that > the Context knows the req (and abs_urls, by extension)? I.e. why must > a Context be able to produce links? Couldn't we just have continued > to pass the req to e.g. render_macro() in addition to the context? I > feel like I'm missing a fundamental point here.
I think so. How would a macro know that it's called from within a RSS output or an regular HTML page? IOW, how would it know that it has to use req.abs_href or req.href? Before, we used to check for format == 'rss' everywhere, but that was more a hack... Now macros can use context.href (or formatter.href, but it's the same).
> My general feeling is that the Context class has too much > responsibility. When we talked about the design before you started > working on it, *my* impression was that a wiki-context should > encapsulate information about the resource to which the text-being- > wikified belongs. Your idea was obviously different: "A Wiki > rendering context contains all the information needed for properly > rendering Wiki text". That's a whole lot broader.
Right, the two aspects are related, as I explained in the beginning. In order to properly render a wiki text, one must know: - *how* the text is accessed (the req part) in order to be able to generate correct links and - *where* that text belong (i.e. the resource) in order to correctly locate relative references See also the mail answering eblot's question "Context in a wiki macro", where I said
In a way, the Context is a kind of "normalization" of the request, a way to /specify/ what resource is accessed and how it is accessed.
The req part is even more fundamental in the security branch, as depending on *who* accesses the wiki text, the rendering can be completely different (restrictions ranging from no access at all, to omitting a few "private" event from the timeline or search results, or to make some ticket links look like "missing" ones and censuring their summary).
> I'm not saying that the design is "wrong", but in general I prefer > functions/classes that have clear focus and responsibility. Trying to > grok the scope of the wiki-context changes today, I couldn't help but > feel that it introduced a certain level of "spaghettification". So > I'd like to understand more about *why* the design looks what it > looks like.
Well, I'm sure that things can still be improved further, and I'm willing to do so, of course. I agree that there's at least *two* notions that are addressed by the Context, one being the AccessContext part, the other being the ResourceDescriptor part. Maybe it's possible to separate them, maybe not, I feel like they are intimately linked. There are two faces on a coin, you don't necessarily want to separate them because of that ;)
The introduction of the Context as it stands now has already helped to solve numerous issues, and its advanced use in the security branch feels very natural. It has already offered an good deal of simplification of the code and at the same time provided the fine grained permission scheme we were looking for.
>> One aspect >> that I still want to refine are the various wiki_to_ methods, which >> should be made more flexible.
> How so?
Well, using the wiki_to_... methods, we hardcode (only) 4 different ways to format or process the wiki text. Say we want to add a text wiki renderer (for the notification e-mails), we'd need to add a wiki_to_text method. I'd like to have something more modular, but don't know exactly how at this point (e.g. it could well be a WikiText(context, fieldname, text) object, which will do the parsing once and could be given to different formatter objects...). Also, a context should probably also know about the mime-type which is currently being rendered (think about a context.wiki_to_html() call within a template, that template being rendered by a mimeview wiki processor ... how would the formatter now about the requested mime-type?).
Well, at least those are a few open points about which I'm still thinking.
A few updates on the topic, as I really want to get through this. I'm willing to make changes as needed, but for that, I first need to better understand cmlenz concerns.
>> [...] >> self_href() is an *awful* name. Even after reading the docstring for >> a couple of times, I still don't really get why it's there and what >> it does. Looking at the invocations, there's apparently only *one* >> place that actually passes an argument to it, and that's in >> formatter.py.
> Agreed, I already wanted to change that a few times. 'resource_href' > would probably be more appropriate. > ... resource_href ... is used to produce the "canonical" URL for > accessing the resource that the context describes.
>> Why is local_url() defined on the Context class? It's only used by >> formatter.py, so why not keep it there? It's just about how external >> links are rendered, it shouldn't concern anyone outside the formatter.
> Yes, I originally thought it could have more uses, but finally it > didn't, so I think we can put that back in the formatter.
Also done in r4666.
>> What's the reason for moving the DB connection handle from the >> formatter to the context?
> Because you often use a Context without a Formatter object, but when you > use the formatter, you always have a Context.
I need to think a bit further on this one, particularly w.r.t. the impact on the db connection pool, as this change could lead to retain the connections outside of the pool for a longer period of time than necessary.
>> And a more fundamental kind of question: is it really necessary that >> the Context knows the req (and abs_urls, by extension)? I.e. why must >> a Context be able to produce links? Couldn't we just have continued >> to pass the req to e.g. render_macro() in addition to the context? I >> feel like I'm missing a fundamental point here.
> I think so. How would a macro know that it's called from within a RSS > output or an regular HTML page? IOW, how would it know that it has to > use req.abs_href or req.href? Before, we used to check for format == > 'rss' everywhere, but that was more a hack... Now macros can use > context.href (or formatter.href, but it's the same).
... and btw, that's what we used to do with the Formatter since the beginning. The context is only a bit more general than the Formatter.
Now of course there could have been other approaches, with different trade-offs (e.g. having RenderingContext and ResourceDescriptor classes). The point is that I don't think it's necessarily a good idea to strive for an hypothetical "perfect" solution and the "ideal" design... The current approach is already more powerful than what we had before, and nothing prevents anyone to propose a better scheme *later*.
Of course, if deficiencies are clearly pointed out now, I'm willing to address them, but please provide me with some concrete topics where I can act upon.
Also, this is not (should not be) only a discussion between cmlenz and me. Others opinions would be helpful.
> Manuzhai wrote: > > On 1/12/07, Christian Boos <cb...@neuf.fr> wrote:
> >> So again, about the merge in trunk: > >> - I'd like we sort out the above first > >> - I'd like to merge my 3 other branches (blame and the 2 -tmp ones) > >> before ;)
> > And after that, maybe there should be some effort towards releasing > > 0.11? It seems merging these branches pretty much fixes all of the > > goals stated for 0.11 in the roadmap...
> Not really... there's still:
> * Use setuptools for setup (see [source:sandbox/setuptools]) > * ''Refactoring of the Mimeview API (see #3332, > [source:sandbox/mimeview])'' > * ''Flexible/extensible ticket workflow (see WorkFlow, > [source:sandbox/workflow])'' > * ''Refactoring of the Wiki Engine to use the Genshi builder API > (''experimental'', cboos)''
> 1. is really mandatory. > 2. is not a big deal, I think > 3. is a somewhat bigger deal, agreed > 4. likewise, still not started. Not /that/ mandatory for 0.11 but would > be nice.
> Besides those stated broad goals, there are lots of worthy tickets still > opened, not to mention those for 0.10.4 ...
> Not to mention that thanks to the rework of the PermissionPolicy, > interesting things are happening in the resource/Context area, which is > a kind of "unlocker" for lots of long standing issues ...
|
