Dev053 - Template render controller #678
Conversation
| {search, ["search"], controller_template, [ {template, "page.tpl"} ]}, | ||
|
|
||
| % Render a template the path is the template, pass arguments via the query post or get | ||
| {spa_render, ["render-template", '*'], controller_ginger_spa_template, []} |
There was a problem hiding this comment.
Should the url have api in it? Like example.com/api/template/render or api/part/render? To avoid potential clashes with project domain paths.
There was a problem hiding this comment.
Better not /api/... as that could clash with the api dispatch rule and controller.
| is_viewable("/" ++ _, _Context) -> false; | ||
| is_viewable("public/" ++ _, _Context) -> true; | ||
| is_viewable("member/" ++ _, Context) -> z_auth:is_auth(Context); | ||
| is_viewable(T, Context) when is_list(T) -> z_acl:is_admin(Context). |
There was a problem hiding this comment.
I think I understand why we need an extra authorisation layer here; because templates can have side-effects.
Possibly this template rendering is only effective and beneficial if we can force it to be immutable data-wise, and transparent to all templates. Part of the efficiency lies in being able to reuse existing templates.
A good alternative mechanism (also to immutability) might be to enforce that the template is reachable by url (full pages) or it resides in a parts or partials dir (I'd prefer those over public because it might not be public).
Probably any module should be able to have a parts/partials dir, and its use could be restricted by the module's usage acl rule. Maybe we can implement (per module) authorisation by allowing acl rules for partials to be added to the acl list in admin?
There was a problem hiding this comment.
The parts / partials is ok with me.
In general templates don't have and shouldn't have side effects.
But... there are some admin templates that show config values, those are the ones that need to be protected.
In the 1.x we solved this with access control on all models, which we can't add to the 0.x
We can check the module permissions, as we could do a template lookup and then check the use permission on the module the template belongs to.
This is not fool-proof, but can come quite far.
| @@ -0,0 +1,2 @@ | |||
| {% debug %} | |||
| {% print q.v %} | |||
There was a problem hiding this comment.
Maybe this should be less dangerous (-looking) default content, without debug? I Imagine you'll easily forget it is there - wait..maybe if we change the default dispatch rule to explicitly reference test.tpl, you'd be bound to remove or fix it in an actual project.
There was a problem hiding this comment.
The debug just echos what is passed, and afaik all data is escaped.
| {search, ["search"], controller_template, [ {template, "page.tpl"} ]}, | ||
|
|
||
| % Render a template the path is the template, pass arguments via the query post or get | ||
| {spa_render, ["api", "render-template", "public", '*' ], controller_ginger_spa_template, []} |
There was a problem hiding this comment.
She earlier comment on public/test.tpl, maybe we should make the default spa_render rule specific, for discoverability of the default test template.
| acceptance -> | ||
| acc; | ||
| _ -> | ||
| dev |
There was a problem hiding this comment.
Is this change extra or needed for the template render controller?
There was a problem hiding this comment.
This smuggled in - for compatibility between the Ginger environment and the Zotonic environment.
| Properties = case m_rsc:get_visible(Id, Context) of | ||
| undefined -> []; | ||
| Props -> Props | ||
| end, |
There was a problem hiding this comment.
Just for the record, from our slack conversation: This rdf change was sneaked in, unrelated to template render controller. Maybe we need to apply this in a separate pull request; maybe it has been lying around for too long because it potentially fixes bugs?
There was a problem hiding this comment.
It potentially fixes bugs.
Ideally it should have been a separate merge request.
| ?WM_REPLY(true, Context2). | ||
|
|
||
| process_post(ReqData, Context) -> | ||
| Context1 = ?WM_REQ(ReqData, Context), |
There was a problem hiding this comment.
Is this strictly necessary? Would returning Context2 in resource_exists (which is visited before process_post) not already have the ReqData WM_REQ'ed in?
| {Data, _} = z_template:render_to_iolist(Template1, Vars, Context), | ||
| iolist_to_binary(Data); | ||
| {error, _} -> | ||
| lager:info("[~p] Denied render of template \"~s\"", [ z_context:site(Context), m_req:get(path, Context) ]), |
There was a problem hiding this comment.
Is denied the right wording? If the template is in the url, any raised error would be caused by that there is no matching dispatch rule.
There was a problem hiding this comment.
Might be anything, but I think this is good enough.
| {error, enoent} | ||
| end; | ||
| Template -> | ||
| {ok, Template} |
There was a problem hiding this comment.
It doesn't do the dispatch check when the template path is in the template context property - is that as intended, or a loophole?
There was a problem hiding this comment.
yes, then it is hard coded in the dispatch rule, so the responsibility of the person programming that dispatch rule.
This to use the same arg as in other template routines.
Add template rendering to mod_ginger_spa