nitrogen / nitrogen_core

The core Nitrogen library.
http://nitrogenproject.com
MIT License
72 stars 66 forks source link

Template is still rendered before `redirect_to_login`? #145

Open codebykat opened 3 years ago

codebykat commented 3 years ago

I'm not sure if there's a bug here or if I'm doing something wrong.

We have a fairly basic custom security handler for access control. Here's the init function:

init(_Config, State) ->
    Module = wf_context:page_module(),

    case check_access(Module) of
        allowed -> ok;
        login_required ->
            % use login template to prevent rendering the protected template briefly
            % TODO it still seems like we should not have to do this
            wf_context:page_module(web_login),
            wf:redirect_to_login("/login");
        not_allowed ->
            wf_context:page_module(web_404),
                        wf_context:path_info([])
        end,

    {ok, State}.

As the comment says, we've found that if we don't change the page_module to something innocuous, the protected template will still render, allowing it to be visible in a brief flash before the redirect to login. (Or, in some cases, allowing it to crash, if it's expecting session data to exist.)

I found this ancient StackOverflow example, in which a user advises a custom security handler "Instead of having the main/0 logic you describe in each of your page handlers". The example code provided simply calls redirect_to_login without otherwise changing the state. I did also try setting a status code of 401, resulting in the server happily rendering the entire protected template, with a 401 status code.

It's my impression that with a custom security handler, we shouldn't have to double-check for access in the main function of every page handler. What am I missing? Is there a better workaround than setting a dummy page handler? Or perhaps some way to cancel the in-progress page load and immediately execute the redirect?

choptastic commented 3 years ago

Hi Kat,

This is a good question.

If I'm understanding you, your current custom security handler works, but you'd rather not have to do the wf_context:page_module/1 call.

Unfortunately, there's not a way, currently, to short-circuit a request (though if ever there was an argument for it, a redirect is probably the strongest argument that exists).

Without a short-circuit to end request dead, you'll need to change the page module, however, to eliminate any amount of flashing, you could do the following:

  1. add a main() -> "". function to your security handler. This will allow it to be a target page module
  2. In your login_required clause instead of the redirect_to_login line, replace it with: wf:header(location, action_redirect:login_redirect_url("/login")), wf:status_code(302);

I just made a quick change to the action_redirect module to export the login_redirect_url function (otherwise, you'll get an undef).

This should solve that problem for you.

======

That said, the longer-term argument should be toward pushing to add a short-circuit option. Implementing this would be a few steps long:

In action_redirect:redirect/1, check if the wf_context:type() == first_request. If it does:

  1. Do the above headers and status code.
  2. Maybe set a value in the process_dictionary like put(wf_short_circuit_request, true)
  3. Refactor wf_core:call_init_on_handlers/0 to check for that process dictionary value after each wf_handler:init/1 call. (or maybe just refactor wf_handler:init/1 alone - either one works, whichever feels cleanest).
  4. Finally, in wf_core:finish_dynamic_request/0, the we'd need to check once more for the short_circuit before rendering elements, and end up calling just build_first_response("", "") (to produce a blank response body)

Doing that slight rework should allow wf:redirect and its siblings (wf:redirect_to_login, etc) to cancel any renderings or actions, and just jump right to returning an empty response with some redirect headers.

If you're comfortable making those changes (I know it's getting a little deep in the request flow), have at 'er and I'll gladly review and pull a PR. If not, I'm happy to do this.

codebykat commented 3 years ago

Heya @choptastic, sorry for the delay getting back to this, I've been a bit swamped. I appreciate the reply and the update to allow short-circuiting the request!

It hadn't occurred to me to use an empty main() function and have the security handler set itself as the page module. That's a pretty elegant answer.

The code now looks like this, and seems to work nicely:

main() -> "".

init(_Config, State) ->
    case check_access(wf_context:page_module()) of
        allowed -> ok;
        login_required ->
            % immediately redirect to login page
            % using web_security as the module prevents rendering the forbidden template at all
            wf_context:page_module(web_security),
            wf:header(location, action_redirect:login_redirect_url("/login")),
            wf:status_code(302);
        not_allowed ->
            wf_context:page_module(web_404),
            wf_context:path_info([])
        end,

    {ok, State}.

I haven't had a chance to look at doing a PR for the request flow. It's looking like I won't have time to do that anytime soon, so don't set the task aside for me or anything! But if you haven't updated it by whenever I have some free cycles again, I will revisit it.

Thanks!!