Entity status

[scott2b]

Adds a status field to Entity and new authz functions for checking entity authorization. Status is an integer with a threshold assigned to indicate published status. As coded, status >= 5 is published. Default status is 0.

Partly addresses issue #61. Does not address issue with respect to Relations. Does not modify entity views to utilize this new functionality.

[pudo]

I'm loving the work you've done on tests; will try to do it similarly from now on. The import error seems odd - are you perhaps using a newer version of Flask that requires a request context in more cases?

In terms of the implementation this seems very good, but doesn't cover entity listings (like here) - it'd be great to have a re-usable filter function (maybe in authz) that could be applied wherever that type of filter is needed. Or does this belong into the model?

[scott2b]

re: Flask version: Flask==0.10.1

I had not thought about the Entity lists use case. Is there precedence for this?

While I am inclined to think that this kind of access in general should be more the responsibility of either the dao, or an object manager, it seems that an authz function would be more consistent with the current implementation.

[pudo]

Ok, I'll try to reproduce the tests thing (same version here).

I'm not against moving this to a DAO, which in grano would probably be the logic set of modules - do you think we should pipe more of the query functions (entity gets and lists) through that?

[scott2b]

I don't know for sure. I'm still new here, so I have to assume that there
are motivations for things that I am not yet aware of. So, I don't know the
code well enough yet to say for sure.

Overall, I say if it's not broke don't fix it. I say get something working
within the current architecture -- something that handles both Entity
objects and Entity lists -- and probably handles Relationship permissions
as well. Then, when those are working maybe scrutinize and refactor if it
seems appropriate.

I think something like authz.all_entities(), or the like would be fine for
now. We can refactor later when we understand things better.

On Mon, Jun 2, 2014 at 12:17 PM, Friedrich Lindenberg <
[email protected]> wrote:

Ok, I'll try to reproduce the tests thing (same version here).

I'm not against moving this to a DAO, which in grano would probably be the
logic set of modules - do you think we should pipe more of the query
functions (entity gets and lists) through that?

—
Reply to this email directly or view it on GitHub
#68 (comment).

[pudo]

Ok, that sounds great - let's go for authz.all_entities().

[scott2b]

quick question: should authz.all_entities() take a project parameter -- should we be getting all permissable entities for the user, or just that user's permissable entities for a given project?

[pudo]

At the moment this would work for all projects, but we may have to move the API to have project-specific entity listings. How big is the difference in query complexity?

On 05 Jun 2014, at 17:42, Scott Bradley [email protected] wrote:

quick question: should authz.all_entities() take a project parameter -- should we be getting all permissable entities for the user, or just that user's permissable entities for a given project?

—
Reply to this email directly or view it on GitHub.

[scott2b]

I'm not sure. Haven't figured out the SQLAlchemy syntax for this. Does SQLAlchemy have dunder syntax for join queries? I don't quite know the correct syntax, but I imagine something like this:

adminable_entities = Entity.all().filter_by(project__permission__account=request.account, project__permission__admin=True)

editable_entities = Entity.all().filter_by(project__permission__account=request.account, project__permission__editor=True)

readable_entities = Entity.all().filter_by(project__permission__account=request.account, project__permission__reader=True, status__gte=PUBLISHED_THRESHOLD)

Project specific queries would add a project=project to all of these.

These aren't exactly pretty. I don't like those excessive joins. Doing all of the entities in one shot would take a big OR query I guess. I suppose if the joins become a problem, we would query out the projects separately -- or the single project in the case of being project specific.

[pudo]

I guess its a bit shorter, because admin always implied editor - this is handled in the logic layer bit. Given that we have most of this query here, I'd just tack on the threshold and extract it to authz?

[scott2b]

This filter is coded to assume that readers have access. It seems like to make this work you would need to union the filtered results of Entity(status >= THRESHOLD) and Entity(status < THRESHOLD, permission = editor).

[scott2b]

Here's what I have for the query. Let me know what you think.

def all_entities():
    q = Entity.all().\
        join(Project).\
        outerjoin(Permission)
    q = q.filter(or_(
        and_(
            Project.private==False,
            Entity.status>=PUBLISHED_THRESHOLD,
        ),
        and_(
            Permission.reader==True,
            Entity.status>=PUBLISHED_THRESHOLD,
            Permission.account==request.account
        ),
        and_(
            or_(
                Permission.editor==True,
                Permission.admin==True
            ),
            Permission.account==request.account
        )
    ))
    return q

Can't really speak to the efficiency. Here is the raw query that Alchemy generates:

SELECT <a bunch of fields>
FROM grano_entity
JOIN grano_project
    ON grano_project.id = grano_entity.project_id
LEFT OUTER JOIN grano_permission
    ON grano_project.id = grano_permission.project_id
LEFT OUTER JOIN grano_property AS grano_property_1
    ON
        grano_entity.id = grano_property_1.entity_id
        AND grano_property_1.obj IN (:obj_1)
WHERE 
    grano_project.private = 0 AND grano_entity.status >= :status_1
    OR 
        grano_permission.reader = 1
        AND grano_entity.status >= :status_2
        AND :param_1 = grano_permission.account_id
    OR 
        (grano_permission.editor = 1 OR grano_permission.admin = 1)
        AND :param_2 = grano_permission.account_id
ORDER BY grano_property_1.created_at DESC

status_1 and _2 here are the publish threshold.

[scott2b]

Note, I probably need a distinct on there. Not sure.

[scott2b]

Ok, revised to handle project and other args like filter_query. Also, would you prefer that this go into views.util instead of authz? Since there is a lot of overlap with filter_query, it might be easier to spot refactorings that way.

def all_entities(**kwargs):
    q = Entity.all().\
        join(Project).\
        outerjoin(Permission)
    q = q.filter(or_(
        and_(
            Project.private==False,
            Entity.status>=PUBLISHED_THRESHOLD,
        ),
        and_(
            Permission.reader==True,
            Entity.status>=PUBLISHED_THRESHOLD,
            Permission.account==request.account
        ),
        and_(
            or_(
                Permission.editor==True,
                Permission.admin==True
            ),
            Permission.account==request.account
        )
    ))
    if 'project' in kwargs:
        q = q.filter(Project.slug==kwargs.get('project'))
    for prop, value, only_active in property_filters(kwargs):
        attributes = Attribute.all_named(prop)
        q = Entity._filter_property(q, attributes, value,
                only_active=only_active)
    q = q.distinct()
    return q

[pudo]

Sorry for the lag in responding, again. The final query looks good, although I also think that this is on the outer edge of where we may want to go in terms of complexity, before we split up the entity listings into project-specific endpoints. In such an implementation, user permissions would be known, and the query would be much simpler. Maybe that time has come?

In any case, one simplification would be in the third and_ clause: the or_ is unnecessary as the saving logic will make sure that editor always toggles admin on.

[pudo]

On second thought, doing project-specific entity endpoints now would be yak shaving. Let's not.

It might be worth to include Entity.same_as==None as a filter, that will not include de-duplicated entities any more.

[scott2b]

Thanks! Revised per above. Also, let me know if you think this belongs more in views.util than in authz. It kind of feels that way to me based on filter_query being in views.util.

def all_entities(**kwargs):
    q = Entity.all().\
        join(Project).\
        outerjoin(Permission)
    q = q.filter(Entity.same_as==None)
    q = q.filter(or_(
        and_(
            Project.private==False,
            Entity.status>=PUBLISHED_THRESHOLD,
        ),
        and_(
            Permission.reader==True,
            Entity.status>=PUBLISHED_THRESHOLD,
            Permission.account==request.account
        ),
        and_(
            Permission.editor==True,
            Permission.account==request.account
        )
    ))
    if 'project' in kwargs:
        q = q.filter(Project.slug==kwargs.get('project'))
    for prop, value, only_active in property_filters(kwargs):
        attributes = Attribute.all_named(prop)
        q = Entity._filter_property(q, attributes, value,
                only_active=only_active)
    q = q.distinct()
    return q

[pudo]

Yeah, I agree - utils it is!

On 10 Jun 2014, at 20:46, Scott Bradley [email protected] wrote:

Thanks! Revised per above. Also, let me know if you think this belongs more in views.util than in authz. It kind of feels that way to me based on filter_query being in views.util.

def all_entities(**kwargs):
q = Entity.all().
join(Project).
outerjoin(Permission)
q = q.filter(Entity.same_as==None)
q = q.filter(or_(
and_(
Project.private==False,
Entity.status>=PUBLISHED_THRESHOLD,
),
and_(
Permission.reader==True,
Entity.status>=PUBLISHED_THRESHOLD,
Permission.account==request.account
),
and_(
Permission.editor==True,
Permission.account==request.account
)
))
if 'project' in kwargs:
q = q.filter(Project.slug==kwargs.get('project'))
for prop, value, only_active in property_filters(kwargs):
attributes = Attribute.all_named(prop)
q = Entity._filter_property(q, attributes, value,
only_active=only_active)
q = q.distinct()
return q
—
Reply to this email directly or view it on GitHub.

[scott2b]

Pull request now includes all_entities utility and changes to the entity views to use new permissions.

[pudo]

Looks very good. Do you think it may make sense to merge this some time, or keep going until we also have relations in there?

[pudo]

btw, the single_arg function would fit nicely into grano.lib.args.

[scott2b]

Ok, let me get the single_arg function moved. After that, I think it would be good to merge and to do relations separately.

[pudo]

Ok, should I give merging this a stab, or would you like to do it?

[scott2b]

I thought maybe you would want to do it, but whatever you think works.

[pudo]

Ok, I’ll give it a shot!

On 17 Jun 2014, at 18:21, Scott Bradley [email protected] wrote:

I thought maybe you would want to do it, but whatever you think works.

—
Reply to this email directly or view it on GitHub.