[ic] Strange setting of form vars to values upon login
Josh Lavin
jlavin at endpoint.com
Wed Jan 18 00:23:17 UTC 2017
Quoting Jon Jensen (jon at endpoint.com):
> 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
More information about the interchange-users
mailing list