Security branch (merge to trunk?)
Alec Thomas
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).
page = WikiPage(env, 'WikiStart')
context = Context.from_resource(env, req, page)
- 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.
[1] http://trac.edgewall.org/wiki/PermissionPolicy
[2] http://trac.edgewall.org/browser/sandbox/security/trac/resource.py#L73
--
Evolution: Taking care of those too stupid to take care of themselves.
John Hampton
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.
-John
Christian Boos
> 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).
>
> 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?
> - 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.
All that being said, great work so far, Alec!
-- Christian
(1) - http://trac.edgewall.org/browser/sandbox/trac-xref/trac/object.py
Alec Thomas
> +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.
Alec Thomas
> If I get some time, I'll try to test out the branch.
That would be much appreciated, thanks :)
Alec Thomas
> > > 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):
def __init__(self, env, name=None, version=None, db=None, req=None, abs_urls=False):
Resource.__init__(self, env, req, 'wiki', name, abs_urls=abs_urls, db=db, resource=self)
...
# Backwards compatibility "name" property
def _get_name(self):
return self.id
def _set_name(self, name):
"""Backwards compatibility property for old WikiPage model."""
self.id = name
name = property(_get_name, _set-name)
or
class Ticket(Resource):
def __init__(self, env, tkt_id=None, db=None, req=None, abs_urls=None):
Resource.__init__(self, env, req, 'ticket', tkt_id, db=db, req=req, abs_urls=abs_urls, resource=self)
...
Then you would be able to do lots of nice things:
>>> page = WikiPage(env, 'WikiStart', req=req, abs_urls=True)
>>> page.href
'http://example.org/trac/wiki/WikiStart'
>>> page.href('#Foo')
'http://example.org/trac/wiki/WikiStart#Foo'
>>> 'WIKI_ADMIN' in page.perm
False
and of course:
>>> page.wiki_to_html('Some [wiki:Wiki] text')
...
Christian Boos
> 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):
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."""
def get_resource_classes():
"""Generator yielding `(realm, resource_subclass)` pairs."""
def get_context_classes():
"""Generator yielding `(realm, context_subclass)` pairs."""
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:
def create_context(resource, req, *args, **kwargs):
rs = ResourceSystem(resource.env)
kwargs['resource'] = resource
return rs.create_context(req, resource.realm, resource.id, *args, **kwargs)
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 ;)
-- Christian
Christian Boos
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:
>
> def create_context(resource, req, *args, **kwargs):
> ...
>
... 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?
-- Christian
Alec Thomas
>
> 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.
Alec Thomas
> 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.
Manuzhai
> 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...
Regards,
Manuzhai
Alec Thomas
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.
Christian Boos
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 ...
-- Christian
Manuzhai
> 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.
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.
Regards,
Manuzhai
Christian Boos
> ...
> 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
-- Christian
Christian Boos
> ...
>> 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.
-- Christian
Christian Boos
> ... So I think the original goals of the PermissionPolicy plan can be
> fulfilled,
(see http://trac.edgewall.org/wiki/PermissionPolicy?version=20)
> ... The current API looks quite fine to me, and behaves quite well
And I just fixed the remaining unit-tests and merged with latest trunk,
so I'm now proposing to merge these changes to the trunk:
http://trac.edgewall.org/changeset?old_path=trunk&old=4610&new_path=sandbox%2Fsecurity&new=4610
-- Christian
Christopher Lenz
> Christian Boos wrote:
>> ... The current API looks quite fine to me, and behaves quite well
>
> And I just fixed the remaining unit-tests and merged with latest
> trunk, so I'm now proposing to merge these changes to the trunk:
>
> http://trac.edgewall.org/changeset?
> old_path=trunk&old=4610&new_path=sandbox%2Fsecurity&new=4610
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.
Thanks,
Chris
--
Christopher Lenz
cmlenz at gmx.de
http://www.cmlenz.net/
richar...@gmail.com
> 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.
Christian Boos
>
> Am 19.01.2007 um 22:41 schrieb Christian Boos:
>
> 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.
-- Christian
Christopher Lenz
> 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).
Related how?
Cheers,
Christian Boos
> 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.
-- Christian
Christian Boos
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.
Christian Boos wrote:
> Christopher Lenz wrote:
>
>> [...]
>> 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.
>
Fixed in http://trac.edgewall.org/changeset/4666/sandbox/security
>> 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.
-- Christian
solo turn
deserve a release ... even more as the impact on plugins quite big.
what would happen if 0.11 gets released immediately and the branch
merges happen immediately afterwards?
-solo.
On 1/14/07, Christian Boos <cb...@neuf.fr> wrote:
>