object-level security and tons of other small improvements to the wiki2 tutorial

[mmerickel]

1 subtle change: reversed the order of arguments to get_tm_session.

This PR is a WIP, haven't updated the tests or touched the tutorial. Wanted to get thoughts on the models package first.

ping @bertjwregeer, @ergo, @pauleveritt, @stevepiercy

[piotr-dobrogost]

Sorry for nitpicking but I'm always confused by such aliases. Why not call it session_maker instead of dbmaker? Ditto in other places where dbmaker is used.

[ergo]

The changes look good, I think @piotr-dobrogost has a point here, I left the naming initially but it was also a bit confusing for me. I would merge that back into the scaffold 👍

[mmerickel]

In my personal projects I define request.db and use a dbmaker so it aligns better. I'm not a huge fan of playing tricks like session_maker = sessionmaker() as if the 1 character absolves us of all ambiguity. The sessionmaker function is very poorly named as it's actually a factory function for sessionmaker objects. I'm open to suggestions but I'm not a huge fan of session_maker myself. Maybe session_factory or SessionFactory?

[ergo]

I'd go with "session_factory".

[digitalresistor]

please no camel casing.

[mmerickel]

Imagine how upset @bertjwregeer is gonna be when he realizes SignedCookieSessionFactory is a function!

Anyway I will change it to session_factory later tonight.

[ergo]

Twisted anyone? ;) Anyways pep8 has clear suggestions on function and variable naming :-)

[digitalresistor]

@mmerickel There are warts in almost all code bases.

[mmerickel]

@bertjwregeer I actually think it's appropriate to camel case factories like that. :-)

[mmerickel]

I've revamped the wording in the basiclayout chapter. Any comments would be greatly appreciated before I move on to the later chapters. I gotta say the fact that I'm only doing small tweaks here and there to the wording is a credit to everyone's amazing work up to this point.

[mmerickel]

@stevepiercy One quick trick.. I discovered the :lineno-match: sphinx directive which has greatly reduced the number of line numbers used, at least in the basiclayout chapter... hopefully in other chapters as well.

[digitalresistor]

@mmerickel I was joking :-P

For factories/classes I don't mind camel casing, but for variables names within a function I am not a big fan.

[stevepiercy]

@mmerickel nice find on :lineno-match:. That led me to this gem:

.. literalinclude:: example.py
   :diff: example.py.orig

I wish I had known when I was working on tutorials that feature had been added.

[stevepiercy]

I think breaking it into authentication and authorization is the better approach: who you are vs. what you can do while authenticated on specific objects. Authorization is ripe for discussing various methods and best practices, such as hashing passwords via a library.

This RBAC diagram from WikiPedia is helpful to visualize ACLs, although it is more detailed and complex than simple ACLs and terminology is slightly different.

[mmerickel]

Okay I added the codebase for simple authentication that actually does inline checks within the views. This should setup nicely to remove the inline checks in the authorization tutorial, replaced by factories. I'll start ripping the authorization text apart soon.

[mmerickel]

I've added a new authentication chapter. Next up is to work on updating the authorization chapter and codebase with context factories... This is getting pretty close to complete.

[stevepiercy]

@mmerickel travis-ci complains about a missing source file:

Warning, treated as error:
/home/travis/build/Pylons/pyramid/docs/tutorials/wiki2/authorization.rst:48: WARNING: Include file '/home/travis/build/Pylons/pyramid/docs/tutorials/wiki2/src/authorization/tutorial/security/default.py' not found or reading it failed

I see a file at wiki2/src/authorization/tutorial/security.py, which I think needs to be moved to wiki2/src/authorization/tutorial/security/default.py.

[mmerickel]

@stevepiercy I haven't worked on the authorization tutorial yet. There are errors there, yes. To build just turn them into warnings.

[mmerickel]

@pauleveritt Hey, I'm starting to update the authorization section of the tutorial now and would like your thoughts on the direction to go. The wiki2 tutorial needs 3 ACLs.

• viewing pages
• editing pages
• new pages

My natural breakdown is 2 factories/contexts. One that loads pages, page_factory, shared between edit/view and a second one that is either set as the default root factory or as a new_page_factory. My question is whether you have an approach to defining the factories and the resources that you like to use? I have my own but I don't want to just force that out there when you also have some patterns to share.

• Does the page_factory and new_page_factory make sense to you?
• Would you attach the ACLs directly to the Page model itself, or do you have a pattern for some wrapper/proxy object that you like to use (a PageResource)?
• For the new page a proxy makes sense (NewPageResource) because there is no Page object so rather we just need a resource that can be used to create new pages.

Here's my current simplest/best for the tutorial (all of this would go into routes.py):

config.add_route('view_page', '/{pagename}', factory=page_factory)
config.add_route('add_page', '/add_page/{pagename}', factory=new_page_factory)
config.add_route('edit_page', '/{pagename}/edit_page', factory=page_factory)

def page_factory(request):
    pagename = request.matchdict['pagename']
    page = request.dbsession.query(Page).filter_by(name=pagename).first()
    if page is None:
        raise HTTPNotFound
    return PageResource(page)

def new_page_factory(request):
    return NewPageResource()

class PageResource(object):
    def __init__(self, page):
        self.page = page

    def __acl__(self):
        return [
            (Allow, Everyone, 'view'),
            (Allow, str(self.page.creator_id), 'edit'),
            (Allow, 'role:editor', 'edit'),
        ]

class NewPageResource(object):
    def __acl__(self):
        return [
            (Allow, 'role:basic', 'create'),
            (Allow, 'role:editor', 'create'),
        ]

[mmerickel]

I've added the first cut at the authorization code using the comments above. Waiting for a reply before working on the text though.

[pauleveritt]

Here is a gist that has what I did for O'Reilly. I can send the entire chapter with RST writeup if you'd like, but I doubt you want all that. :)

https://gist.github.com/pauleveritt/2a1d9040a585ee1b4acc

It includes class- and instance-level security, which might be more than what you want to show. It won't match the scenario you describe, as you want a different ACL for the collection (adding) vs. the items. I like your example more.

I guess the biggest difference is the one we previously discussed. You create separate classes and objects apart from the models, in routes, and put Pyramid-y stuff on them. I conflated things onto models and put the ACL there.

With your approach, you wouldn't be able to do has_permission filtering in a view, for example to filter a list of wiki pages you're allowed to see. (I did that in a tutorial step.) With my approach, since the Pyramid stuff like acl is on the SQLAlchemy model, a query returns objects that can be permission filtered.

I think it's the kind of thing where we're just solving different problems. I think you are putting this in routes because the only Pyramid machinery you want is on guarding a URL. I was trying to do more.

[mmerickel]

It includes class- and instance-level security, which might be more than what you want to show. It won't match the scenario you describe, as you want a different ACL for the collection (adding) vs. the items.

That's a clever little hack with def __acl__(self=None): that I've never seen before. I'm not sure that's a pattern I'm ready to push in this wiki2 tutorial but maybe.

I think where I'm at right now is that I could add a note that the separation is not required but it is done here to show the parts that Pyramid actually cares about separately from the model itself. At the end of the day you can return anything you want from the factory, including a model instance (or a model class?? I guess is where you're going with that). I will say that using a class as context feels like an anti-pattern to me in my constant quest to get people away from using global state. A class is just begging for global state if you need anything other than a static ACL. In the current code I only show __acl__ being used as a callable for this reason.

With my approach, since the Pyramid stuff like acl is on the SQLAlchemy model, a query returns objects that can be permission filtered.

Definitely this is an issue but not in the current wiki2 example. We could add an administrative-type page that showed all wiki pages and an edit button next to ones you could edit if we wanted to exercise that feature.

Alternatively the context could have just been updated with a method like find_related() that would return wrapped-versions of the pages. You basically put the API on the context instead of the model itself and treat the model as POD (hopefully making it more reusable). In an MVVM world you can think of the context as basically a View Model that is wrapping the Model into an object used in a View.

Anyway I've always come out against putting pyramid-y stuff on models but if it's simpler for people that aren't me then I'm okay with that. I'd like to present the most useful pattern I can for people but I don't want to make decisions that the community suffers for either. Which is why I'm asking someone with your tutorial experience for some guidance. :-)

Is there a middle ground here where I add the ACL to the Page model and then keep the NewPage shim? Or is that more confusing?

[pauleveritt]

All good points. I suspect the goal of a tutorial should be to limit the number of Cosmic Ideas, and instead keep as many things as possible familiar to the reader. I don't know if our audience would feel better with wrappers that act in an MVVM way, or a model object that gets some of the web framework injected into it.

I don't have good evidence on this. I think you are in favor of introducing the wrapper indirection. Since you're the one doing the work, you win. :)

[pauleveritt]

Other notes:

1. You're right about global state, but my "default_acl" thing was only present because I wanted to support instance ACLs. You don't so that can be eliminated.

2. Your wrappers only provide an acl method. It's a shame that views then have to get to the actual data via context.page, so that you can wrap the returned query instance's class. I couldn't find any way in SQLAlchemy to extend the returned class. OTOH, you might want them to put more on PageResource.

3. I was going for a re-usable "Resource" mix-in for models, which would add in things like class/instance ACLs, name, parent, etc. Something that could be shipped in a pyramid_sqlresource package (along with your other improvements.) But as that isn't your goal here, doing things in a mixin-friendly way is counter-productive.

[mmerickel]

I feel like I've seen many more examples of coupling the acls to the models than examples of keeping them separate. The issue with using has_permission on any arbitrary model is completely valid so I don't want to just ignore it.

The only real issue I have with adding the ACLs to the models themselves is the need for the NewPage model, and whether I should define that in the models package or where it is currently in the routes module. Then I need to import pyramid.security in different places!

[digitalresistor]

All of my projects are traversal, and I create proxy objects that then build up the traversal using __getitem__.

It's these proxy objects that have an __acl__ on them, because that ACL may or may not be static, or pulled from multiple sources in SQL/non-SQL.

That proxy object is used as the context in all of my views. It owns the DB object, and is passed to all of my pyramid_services which then has all the magic stuff on it that manipulate/pull data out and whatnot. My views are very thin little shims that don't really have a lot of logic in them, it's all in services.

[mmerickel]

Alright folks... first draft of revamp of revamp is complete. I'm gonna merge this goodness into the official alchemy-scaffold branch so that we can get some good review.