Mailing List Archive

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?

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?

Thanks,
Jon


--
Jon Jensen
End Point Corporation
https://www.endpoint.com/

_______________________________________________
interchange-users mailing list
interchange-users@icdevgroup.org
http://www.icdevgroup.org/mailman/listinfo/interchange-users

Quoting Jon Jensen (jon@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

_______________________________________________
interchange-users mailing list
interchange-users@icdevgroup.org
http://www.icdevgroup.org/mailman/listinfo/interchange-users

On Tue, 17 Jan 2017, Josh Lavin wrote:

> 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"?

In the simple case, what you are saying makes sense to me.

However: I haven't traced through it completely, but the login method
calls the get_values method which can load various user account values,
which can end up in values space or scratch or both, depending on
settings. So I think there is a possibility that without that update_user
call, some things may not get set in values space from the user's account
settings.

In other words, perhaps those form values getting copied over is just an
accidental, and perhaps, as you suspect, unneeded, side-effect. But other
values may be getting copied there besides what's in the form.

Since you say this logic predates version control it would be very helpful
if Mike can weigh in, if he remembers the reason for it or knows of any
code that depends on it. If he doesn't know, we should probably trace the
behavior closely in the face of user account settings being loaded with
various UserDB options and see how it behaves.

Jon


--
Jon Jensen
End Point Corporation
https://www.endpoint.com/

_______________________________________________
interchange-users mailing list
interchange-users@icdevgroup.org
http://www.icdevgroup.org/mailman/listinfo/interchange-users

I do not remember the context. This is code from 1997, no doubt.

On Tue, Jan 17, 2017 at 11:48 PM, Jon Jensen <jon@endpoint.com> wrote:

> On Tue, 17 Jan 2017, Josh Lavin wrote:
>
> 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"?
>>
>
> In the simple case, what you are saying makes sense to me.
>
> However: I haven't traced through it completely, but the login method
> calls the get_values method which can load various user account values,
> which can end up in values space or scratch or both, depending on settings.
> So I think there is a possibility that without that update_user call, some
> things may not get set in values space from the user's account settings.
>
> In other words, perhaps those form values getting copied over is just an
> accidental, and perhaps, as you suspect, unneeded, side-effect. But other
> values may be getting copied there besides what's in the form.
>
> Since you say this logic predates version control it would be very helpful
> if Mike can weigh in, if he remembers the reason for it or knows of any
> code that depends on it. If he doesn't know, we should probably trace the
> behavior closely in the face of user account settings being loaded with
> various UserDB options and see how it behaves.
>
> Jon
>
>
> --
> Jon Jensen
> End Point Corporation
> https://www.endpoint.com/
>
> _______________________________________________
> interchange-users mailing list
> interchange-users@icdevgroup.org
> http://www.icdevgroup.org/mailman/listinfo/interchange-users
>



--
The problem with Internet quotations is that many of them
are not genuine. -- Abraham Lincoln