Re: Strange setting of form vars to values upon login

Quoting Jon Jensen ([email protected]):
> On Tue, 17 Jan 2017, Josh Lavin wrote:
> 
> > I noticed something when submitting a login form, where the form
> > variables would make their way into Values space.
> > 
> > This happened whether or not the form action was "return" (should
> > update variables) or "back" (don't update variables).
> > 
> > The culprit lies in this code:
> > 
> >        if ($status = $user->login(%options) ) {
> >            ::update_user();
> >        }
> > 
> > line 2955 of UserDB.pm.
> > 
> > The update_user() sub is in Dispatch.pm, and it effectively adds items
> > to the cart and then updates values with its update_values() sub.
> > 
> > This has all been in the code since before CVS was added. :-)
> > 
> > This causes the following form variables to go to Values space:
> > 
> >    mv_session_id
> >    mv_username
> >    mv_form_charset
> >    destination
> >    mv_form_profile
> >    mv_action
> > 
> > which seems wrong to me.
> > 
> > The update_user() sub is used other places in the code, so the rational
> > solution to me seems to be to stop calling it upon login in UserDB.pm.
> > 
> > Or else just live with it, if it is OK to have these in Values.
> > 
> > Is this an issue?
> 
> What concretely are you proposing to change?

I propose in UserDB.pm:

-               if ($status = $user->login(%options) ) {
-                       ::update_user();
-               }
+               $status = $user->login(%options);

Basically, don't call update_user() when logging in. The update_user()
sub updates the values (this is exactly what the "return" action does;
snippet below).

This behavior pre-dates source-control, so I don't know why it was
added, but it seems wrong -- why would we want a login form to save its
parameters to Values space, when you could get that by calling the
mv_action of "return"?

Currently, the Strap login.html page uses mv_action=return, but that
could be changed to "back".

Unless we want a login form to be able to order items as well, which
update_user() allows if "mv_order_item" is passed. But again, you could
just use mv_action=return in your form if you really wanted that.

I'm not sure if the form variables listed above are a concern in Values
space, (although "mv_session_id" and "mv_username" seem odd to be in
Values). They aren't really appropriate there, but maybe they don't hurt
anything, in which case we don't have to do anything.

> You mentioned that those variables are saved to values whether the form
> action was "return" or "back". Do you propose changing the behavior of only
> "back", or of more?

No, as they exist in Dispatch.pm, "back" and "return" do what they
should:

    back    => sub { return 1 },
    return  => sub {
                    update_user();
                    ...
                },

-- 
Josh Lavin
End Point Corporation