lingthio / Flask-User

Customizable User Authorization & User Management: Register, Confirm, Login, Change username/password, Forgot password and more.
http://flask-user.readthedocs.io/
MIT License
1.06k stars 294 forks source link

Problem with USER_AFTER_REGISTER_ENDPOINT. #177

Open carrete opened 7 years ago

carrete commented 7 years ago

As they say, a diff is worth a thousand words...

In views.py:register:


         # Redirect if USER_ENABLE_CONFIRM_EMAIL is set
         if user_manager.enable_confirm_email and require_email_confirmation:
-            safe_reg_next = user_manager.make_safe_url_function(register_form.reg_next.data)
             return redirect(safe_reg_next)

         # Auto-login after register or redirect to login page
-        if 'reg_next' in request.args:
-            safe_reg_next = user_manager.make_safe_url_function(register_form.reg_next.data)
-        else:
-            safe_reg_next = _endpoint_url(user_manager.after_confirm_endpoint)
         if user_manager.auto_login_after_register:
             return _do_login_user(user, safe_reg_next)                     # auto-login
         else:

safe_reg_next is computed at the start of views.py:register, then, if the request method is not POST, register_form.reg_next.data is set to equal safe_reg_next (none of which is shown in the diff above). Except that regsiter_form.reg_next.data will only have a value if 1) the request method is not POST or 2) someone has provided a different template that adds this value to the registration form or the form action endoint as a url parameter.

This:

    safe_reg_next = _get_safe_next_param('reg_next', user_manager.after_register_endpoint)

is how safe_reg_next is computed at the start of views.py:register. _get_safe_next_param looks for reg_next in request.args and if not found there will use USER_AFTER_REGISTER_ENDPOINT. Where safe_reg_next is needlessly computed in the diff above you'll see that USER_AFTER_REGISTER_ENDPOINT (a.k.a. user_manager.after_register_endpoint) is not used. This is the problem.

Also, please note in the diff above that:

-            safe_reg_next = _endpoint_url(user_manager.after_confirm_endpoint)

uses after_confirm_endpoint. I think this should have been after_register_endpoint, right?

I'm having a hard time following the setting of safe_reg_next. I'm not certain the diff is correct or complete. All I can say is that this works in my case where enable_confirm_email and require_email_confirmation are true and USER_AFTER_REGISTER_ENDPOINT has been set to something other than the default of ''.

The tests still pass but then they passed before. So please keep this in mind.

views.py:login may also suffer from the same problem.

lingthio commented 7 years ago

Hi @carrete ,

Though safe_reg_next is set near the top of the function (line 330), it is later passed to the register_form (line 355). At the end of the POST processing, this must then be retrieved from register_form.reg_next.data.

I think your issue is caused by the if statement: if 'reg_next' in request.args: which will be false if the POST url no longer contains the 'reg_next' query param.

The fix is: if register_form.reg_next.data

I'll look into this a bit more later, and will post the final code change here.

carrete commented 7 years ago

Hi @lingthio ,

Thanks for taking the time to look into this. Is there a reason why there should be four instances where safe_reg_next is calculated? Could this be simplified? I think there's an opportunity to reduce some complexity which may be the true source of this problem.

This is untested, but could we simplify this to look like:

unsafe_reg_next = None
if request.method == 'GET':
    unsafe_reg_next = request.params.get('reg_next', None)
else:
    unsafe_reg_next = register_form.reg_next.data
if not unsafe_reg_next:
    unsafe_reg_next = url_for(user_manager.after_register_endpoint)
safe_reg_next = _make_safe_url(unsafe_reg_next)

Then replace the occurrences of register_form.reg_next.data that follow with safe_reg_next.

Would this work?