Mailing List Archive

On Wed, 2010-11-24 at 12:08 +0100, Markus Tallowitz wrote:
> hi,
>
> i wrote a patch for 0.9.9.3, so that inactive user are not able to log in any
> more. This feature was broken for me.

Hi Markus,

I just tested here and setting a user inactive meant that they could no
longer log in either via the web interface or to fetch calendar data, so
I fail to see what changes are needed.

If there is a bug in handling this in some circumstances then I think
I'd like to understand it better before we develop a patch which would
hopefully be much smaller than this one. I don't believe there should
be any need to write a function to retrieve a single field from the
database, when we must already be retrieving the whole row during the
password checking process.


> The patch also makes sure, that inactive
> user can't access any calendars. I tested it with lightning.
>
> == HTTPAuthSession.php:
> Assign session details only, if the user is authenticated and not inactive.

In line 237 $usr->active is already included in the tests before we even
check their password. If we doubt that, then should it not just wrap
line 124/125 in another if, like:

if ( $u->active ) {
$this->AssignSessionDetails($u);
return;
}


> == Session.php:
> I changed
> - the structure of the code a bit, so the Code for "Invalid username or
> password" only have to be there once.
> - the position of the temporary_password check (but i didn't test it)
> - the condition for checking if the user is active

The code for "Invalid username or password" is really two tests, one for
"Invalid username" and another for "Invalid password". If debugging is
enabled the differences are identified in the log, although normally
both cases are identified as "Invalid username or password".

If needed, the check for inactive could be done by adding " AND active"
into the SQL on line 441, couldn't it? I wonder if that would address
the whole problem here, since you're hacking on the code for LSIDLogin()
(where they set a cookie for automatic login) rather than in Login()
(where they just entered a username/password) which already has " AND
active" on the end of it's SQL to fetch the usr record.


> == User.php:
> Not sure if this is right - The code reads, if the user is really active

This is the one I believe is least necessary. We must already have that
information to hand during the session setup.

Thanks,
Andrew.
--
------------------------------------------------------------------------
andrew (AT) morphoss (DOT) com +64(272)DEBIAN
Powering the .NZ namespace with Open Source Software
------------------------------------------------------------------------

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <http://lists.morphoss.com/pipermail/davical-dev/attachments/20101127/63b6490c/attachment.pgp>

Hi Andrew,

Am 27.11.2010 04:09, schrieb Andrew McMillan:
> On Wed, 2010-11-24 at 12:08 +0100, Markus Tallowitz wrote:
>> hi,
>>
>> i wrote a patch for 0.9.9.3, so that inactive user are not able to log in any
>> more. This feature was broken for me.
> Hi Markus,
>
> I just tested here and setting a user inactive meant that they could no
> longer log in either via the web interface or to fetch calendar data, so
> I fail to see what changes are needed.
>
Ok, maybe i better had mentioned that i use ldap for authentication. sorry.
And with the original davical and libawl sources it is possible for inactive
users to log in. With the patch for Session.php it is not possible any more.
But the inactive users are still able to download calendar-data. Therefore i
wrote the other patches.

The patch for Session.php was not very small, so i attached a much smaller
patch for Session.php in this email, which is enough to prevent inactive users
to login into the webinterface. In the original Session.php the code checks
for "AND Active" in the database if $usr is not set already. But $usr is
already set when the authentication module says that the user login is
correct. So the database query for "...AND Active" is never executed. And no
authentication module cares for the ACTIVE-flag in the Database.

> If there is a bug in handling this in some circumstances then I think
> I'd like to understand it better before we develop a patch which would
> hopefully be much smaller than this one. I don't believe there should
> be any need to write a function to retrieve a single field from the
> database, when we must already be retrieving the whole row during the
> password checking process.
>
you are right
>> The patch also makes sure, that inactive
>> user can't access any calendars. I tested it with lightning.
>>
>> == HTTPAuthSession.php:
>> Assign session details only, if the user is authenticated and not inactive.
> In line 237 $usr->active is already included in the tests before we even
> check their password.
this line in this function is never reached, because the function only runs
the authentication module (ldap)

> If we doubt that, then should it not just wrap
> line 124/125 in another if, like:
>
> if ( $u->active ) {
> $this->AssignSessionDetails($u);
> return;
> }
>
this works. but there are more calls of the function AssignSessionDetails in
this file, so i tried to check the active-flag on only one position.
>> == Session.php:
>> I changed
>> - the structure of the code a bit, so the Code for "Invalid username or
>> password" only have to be there once.
>> - the position of the temporary_password check (but i didn't test it)
>> - the condition for checking if the user is active
> The code for "Invalid username or password" is really two tests, one for
> "Invalid username" and another for "Invalid password".
this makes sense

> If debugging is
> enabled the differences are identified in the log, although normally
> both cases are identified as "Invalid username or password".
>
> If needed, the check for inactive could be done by adding " AND active"
> into the SQL on line 441, couldn't it?
no - Session.php / line 441: $qry = new PgQuery( "SELECT * FROM usr WHERE
md5(user_no::text)=? AND active;", $md5_user_no );

> I wonder if that would address
> the whole problem here, since you're hacking on the code for LSIDLogin()
in the file Session.php i never changed LSIDLogin, i only changed Login
> (where they set a cookie for automatic login) rather than in Login()
> (where they just entered a username/password) which already has " AND
> active" on the end of it's SQL to fetch the usr record.
Yes, in Login(), there is already the check for "AND active", but this is only
executed, when there is no authentication module. In my mail i forgot to say
that i am using ldap. sorry.
>> == User.php:
>> Not sure if this is right - The code reads, if the user is really active
> This is the one I believe is least necessary. We must already have that
> information to hand during the session setup.
>
> Thanks,
> Andrew.

Yes, this patch is not necessary. sorry for that.

Is it the job of the authentication-modules (ldap, pam,...) to check whether a
user is set inactive in postgres-database by an administrator? Until I saw
your commit
(http://repo.or.cz/w/davical.git/commit/413618749cb86e828d108a0a34c6360865bcbb9b)
i thought not, but I attached a patch for the ldap-driver, which checks the
davical-database for inactive/active users. But what about other plugins?

When someone removes a user from ldap, than the user can not login. And the
user gets deactivated in davical database. That is ok. But when someone is
setting the user manually inactive, then ldap don't know something about that
inactive-flag. so it should be checked somewhere in the code. maybe in the
drivers, in Session.php or somewhere else.



-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: Session.php.diff
URL: <http://lists.morphoss.com/pipermail/davical-dev/attachments/20101129/69d93131/attachment.txt>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: drivers_ldap.php.diff
URL: <http://lists.morphoss.com/pipermail/davical-dev/attachments/20101129/69d93131/attachment.asc>