Bug 1121748 - The "Loaned To" field allows space before user name but user not able to access system
Summary: The "Loaned To" field allows space before user name but user not able to acce...
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Beaker
Classification: Community
Component: web UI
Version: 0.17
Hardware: All
OS: Linux
unspecified
high vote
Target Milestone: 0.17.3
Assignee: Dan Callaghan
QA Contact: tools-bugs
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2014-07-21 19:00 UTC by Arthur Benoit
Modified: 2018-02-06 00:41 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2014-08-14 04:50:27 UTC


Attachments (Terms of Use)

Description Arthur Benoit 2014-07-21 19:00:01 UTC
Description of problem:
The "Loaned To" field allows space before user name but user not able to access system

Version-Release number of selected component (if applicable):
.17

How reproducible:
Always

Steps to Reproduce:
1. Add system loan with space before the users name
2. Assign Access Policy that user belongs to with "Loan to self"  ACL  
3. Have user attempt to return the loan or run a job.

Actual results:
Job will not start and or user can't return the system.

Expected results:
Beaker should not allow space character before names or it should strip out
blank spaces.


Additional info:

Comment 2 Dan Callaghan 2014-07-24 02:10:43 UTC
Okay it turns out this one is quite nasty... The problem is Beaker tries looking up a username like "   dcallagh" in the database, which doesn't exist, but then it tries looking it up in LDAP. A mistake with quoting in the LDAP filter means that this actually matches "dcallagh", so Beaker creates a *new* user account with the username "   dcallagh" based on the LDAP lookup for "dcallagh".

This is particularly bad because now there will be two user accounts with different user names that look the same in Beaker's UI. So those will need to be cleaned up.

Prior to 0.17 the UNIQUE constraint on e-mail address would have ultimately prevented Beaker from creating the duplicate whitespace username (the whole request would have just exploded with a 500).

To fix this we should:

* Add SQLAlchemy validation to reject usernames with leading or trailing whitespace (or possibly, usernames with any whitespace at all?)

* Fix the LDAP look-up code in User.by_user_name to correctly quote the uid value

* Optionally, in the web layer trim leading/trailing whitespace from usernames that are submitted by the user

* Figure out a strategy to deal with the duplicate user accounts (ideally some SQL which will merge them back together)

Comment 4 Nick Coghlan 2014-07-24 03:58:20 UTC
For this bug, let's just track the "keep it from happening again" fix. I agree we also need to write and publish the SQL to fix installations that were already affected by the bug, but we can track that separately.

Comment 6 Dan Callaghan 2014-08-04 05:40:22 UTC
(In reply to Dan Callaghan from comment #2)
> * Fix the LDAP look-up code in User.by_user_name to correctly quote the uid
> value

Actually it turns out there is no mistake in the quoting, it's done correctly using the filter_format() function.

The actual problem is that LDAP's rules for whitespace normalisation [1] mean that usernames which are distinct from Beaker's point of view will be normalised to the same value in LDAP.

Unfortunately it seems like python-ldap doesn't have any code to do the normalisation, although it probably should since OpenStack have written their own version [2] and we will have to do the same...

[1] http://tools.ietf.org/html/rfc4518#section-2.6.1
[2] http://docs.openstack.org/developer/keystone/api/keystone.common.ldap.html#keystone.common.ldap.core.prep_case_insensitive

Comment 7 Dan Callaghan 2014-08-04 07:35:36 UTC
On Gerrit: http://gerrit.beaker-project.org/3230

Comment 9 Dan Callaghan 2014-08-06 06:10:57 UTC
The following query can be used to check if your Beaker database is affected by this issue:

SELECT old_user.user_name, old_user.user_id, new_user.user_name, new_user.user_id
FROM tg_user old_user
INNER JOIN tg_user new_user
    ON TRIM(old_user.user_name) = new_user.user_name
    AND old_user.user_name != new_user.user_name;

If any rows are returned, they are duplicates with leading/trailing whitespace.

(Note that it doesn't cover cases where a username has multiple consecutive inner whitespace characters, which would be normalized to a single space. I'm not covering that case because I think whitespace in usernames is uncommon since it doesn't meet the constraints of posixAccount.)

Comment 10 Dan Callaghan 2014-08-06 06:52:53 UTC
The following UPDATE statements may be useful for fixing foreign keys currently pointing at a duplicate tg_user row, to make them refer to the original tg_user row instead.

Beware that these statements *will not* work correctly in cases where there is already a row referencing the original tg_user row as well.


UPDATE submission_delegate
INNER JOIN tg_user old_user
    ON submission_delegate.delegate_id = old_user.user_id
INNER JOIN tg_user new_user
    ON TRIM(old_user.user_name) = new_user.user_name
    AND old_user.user_name != new_user.user_name
SET submission_delegate.delegate_id = new_user.user_id;

UPDATE system
INNER JOIN tg_user old_user
    ON system.loan_id = old_user.user_id
INNER JOIN tg_user new_user
    ON TRIM(old_user.user_name) = new_user.user_name
    AND old_user.user_name != new_user.user_name
SET system.loan_id = new_user.user_id;

UPDATE system_access_policy_rule
INNER JOIN tg_user old_user
    ON system_access_policy_rule.user_id = old_user.user_id
INNER JOIN tg_user new_user
    ON TRIM(old_user.user_name) = new_user.user_name
    AND old_user.user_name != new_user.user_name
SET system_access_policy_rule.user_id = new_user.user_id;

UPDATE user_group
INNER JOIN tg_user old_user
    ON user_group.user_id = old_user.user_id
INNER JOIN tg_user new_user
    ON TRIM(old_user.user_name) = new_user.user_name
    AND old_user.user_name != new_user.user_name
SET user_group.user_id = new_user.user_id;

Comment 12 Dan Callaghan 2014-08-07 23:48:24 UTC
(In reply to Dan Callaghan from comment #10)
> The following UPDATE statements may be useful for fixing foreign keys
> currently pointing at a duplicate tg_user row, to make them refer to the
> original tg_user row instead.

Another one, for system.owner_id:

UPDATE system
INNER JOIN tg_user old_user
    ON system.owner_id = old_user.user_id
INNER JOIN tg_user new_user
    ON TRIM(old_user.user_name) = new_user.user_name
    AND old_user.user_name != new_user.user_name
SET system.owner_id = new_user.user_id;

Comment 14 Amit Saha 2014-08-14 04:50:27 UTC
Beaker 0.17.3 has been released (https://beaker-project.org/docs/whats-new/release-0.17.html#beaker-0-17-3)


Note You need to log in before you can comment on or make changes to this bug.