Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Template is still rendered before redirect_to_login? #145

Open
codebykat opened this issue Sep 10, 2021 · 2 comments
Open

Template is still rendered before redirect_to_login? #145

codebykat opened this issue Sep 10, 2021 · 2 comments

Comments

@codebykat
Copy link
Contributor

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
Copy link
Member

choptastic commented Sep 10, 2021

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
Copy link
Contributor Author

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!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants