Bug 47712 - validate type of keys (schools) for school_classes
Summary: validate type of keys (schools) for school_classes
Status: CLOSED FIXED
Alias: None
Product: UCS@school
Classification: Unclassified
Component: Ucsschool-lib
Version: UCS@school 4.3
Hardware: Other Linux
: P5 normal
Target Milestone: ---
Assignee: Daniel Tröder
QA Contact: Tobias Wenzel
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-09-03 21:30 CEST by Jürn Brodersen
Modified: 2023-06-12 15:39 CEST (History)
5 users (show)

See Also:
What kind of report is it?: Bug Report
What type of bug is this?: 4: Minor Usability: Impairs usability in secondary scenarios
Who will be affected by this bug?: 2: Will only affect a few installed domains
How will those affected feel about the bug?: 3: A User would likely not purchase the product
User Pain: 0.137
Enterprise Customer affected?:
School Customer affected?:
ISV affected?:
Waiting Support:
Flags outvoted (downgraded) after PO Review:
Ticket number:
Bug group (optional):
Customer ID:
Max CVSS v3 score:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jürn Brodersen univentionstaff 2018-09-03 21:30:58 CEST
validate keys (schools) for school_classes in import_user.py

I tried using ucs-school-testuser-import but still had http api config active. The result was that the school was set to None for school_classes and subsequent traceback.



Traceback (most recent call last):
  File "/usr/lib/pymodules/python2.7/ucsschool/importer/frontend/cmdline.py", line 120, in main
    self.do_import()
  File "/usr/lib/pymodules/python2.7/ucsschool/importer/frontend/cmdline.py", line 97, in do_import
    importer.mass_import()
  File "/usr/lib/pymodules/python2.7/ucsschool/importer/mass_import/mass_import.py", line 76, in mass_import
    self.import_users()
  File "/usr/lib/pymodules/python2.7/ucsschool/importer/mass_import/mass_import.py", line 106, in import_users
    user_import.create_and_modify_users(imported_users)  # 90% - 100%
  File "/usr/lib/pymodules/python2.7/ucsschool/importer/mass_import/user_import.py", line 201, in create_and_modify_users
    success = user.create(lo=self.connection)
  File "/usr/lib/pymodules/python2.7/ucsschool/importer/models/import_user.py", line 330, in create
    res = super(ImportUser, self).create(lo, validate)
  File "/usr/lib/pymodules/python2.7/ucsschool/lib/models/base.py", line 435, in create
    success = self.create_without_hooks(lo, validate)
  File "/usr/lib/pymodules/python2.7/ucsschool/lib/models/base.py", line 463, in create_without_hooks
    self.do_create(udm_obj, lo)
  File "/usr/lib/pymodules/python2.7/ucsschool/lib/models/user.py", line 235, in do_create
    udm_obj['groups'] = self.groups_used(lo)
  File "/usr/lib/pymodules/python2.7/ucsschool/lib/models/user.py", line 477, in groups_used
    self.get_or_create_group_udm_object(group_dn, lo)
  File "/usr/lib/pymodules/python2.7/ucsschool/lib/models/user.py", line 484, in get_or_create_group_udm_object
    school = cls.get_school_from_dn(group_dn)
  File "/usr/lib/pymodules/python2.7/ucsschool/lib/models/base.py", line 642, in get_school_from_dn
    return SchoolSearchBase.getOU(dn)
  File "/usr/lib/pymodules/python2.7/ucsschool/lib/schoolldap.py", line 203, in getOU
    school_dn = cls.getOUDN(dn)
  File "/usr/lib/pymodules/python2.7/ucsschool/lib/schoolldap.py", line 216, in getOUDN
    match = cls._RE_OUDN.search(dn)
TypeError: expected string or buffer
Comment 1 Ole Schwiegert univentionstaff 2018-09-04 09:02:30 CEST
I stumbled over the same problem while creating some test data for the http API
Comment 2 Toni Röhmeyer univentionstaff 2020-02-19 11:52:00 CET
I cannot reproduce this. Please provide the configuration and CSV file or the Python code you used.
Comment 3 Jürn Brodersen univentionstaff 2020-02-21 10:39:41 CET
Config for /var/lib/ucs-school-import/configs/user_import.json:

'''
{                                                                               
        "classes": {                                                            
                "reader": "ucsschool.importer.reader.http_api_csv_reader.HttpApiCsvReader"
        },                                                                      
        "csv": {                                                                
                "mapping": {                                                    
                        "Vorname": "firstname",                                 
                        "Nachname": "lastname",                                 
                        "Klassen": "school_classes",                            
                        "Beschreibung": "description",                          
                        "Telefon": "phone",                                     
                        "EMail": "email"                                        
                }                                                               
        },                                                                      
        "scheme": {                                                             
                "record_uid": "<firstname>.<lastname>",                         
                "username": {                                                   
                    "default": "<:umlauts><firstname>.<lastname><:lower>[COUNTER2]"
                }                                                               
        },                                                                      
        "source_uid": "TESTID",                                                 
        "verbose": false,                                                       
        "normalize": {                                                          
                "firstname": false,                                             
                "lastname": false                                               
        }                                                                       
}
'''

Use the the test import:
./ucs-school-testuser-import --students 1 --classes 1 $OU

You should get a traceback:
'''
'NoneType' object has no attribute 'lower'
Traceback (most recent call last):
  File "/usr/lib/pymodules/python2.7/ucsschool/importer/mass_import/mass_import.py", line 121, in import_users
    user_import.create_and_modify_users(imported_users)  # 90% - 100%
  File "/usr/lib/pymodules/python2.7/ucsschool/importer/mass_import/user_import.py", line 179, in create_and_modify_users
    success = user.create(lo=self.connection)
  File "/usr/lib/pymodules/python2.7/ucsschool/importer/models/import_user.py", line 321, in create
    res = super(ImportUser, self).create(lo, validate)
  File "/usr/lib/pymodules/python2.7/ucsschool/lib/models/base.py", line 478, in create
    success = self.create_without_hooks(lo, validate)
  File "/usr/lib/pymodules/python2.7/ucsschool/lib/models/base.py", line 491, in create_without_hooks
    self.validate(lo)
  File "/usr/lib/pymodules/python2.7/ucsschool/importer/models/import_user.py", line 1017, in validate
    super(ImportUser, self).validate(lo, validate_unlikely_changes)
  File "/usr/lib/pymodules/python2.7/ucsschool/lib/models/user.py", line 418, in validate
    if school.lower() not in (s.lower() for s in self.schools + [self.school]):
AttributeError: 'NoneType' object has no attribute 'lower'
'''

I changed the bug component, because I now think this should rather be validated in models/user.py
Comment 4 Daniel Tröder univentionstaff 2020-02-21 11:01:07 CET
I think we should check more generally that all keys (OUs) in user.school_classes must be in user.schools.

Something along:

assert user.school_classes == dict(
    (sc, cl)
    for sc, cl in user.school_classes.items()
    if sc in user.schools
)

Maybe replace "user.schools" with "user.schools + [user.school]" in case user.schools has not been updated yet...

As None is never in user.schools, it would fix this specific bug (None as key in school_classes) as a side effect of checking school_classes consistency.
Comment 5 Jürn Brodersen univentionstaff 2020-02-21 11:42:08 CET
(In reply to Daniel Tröder from comment #4)
> I think we should check more generally that all keys (OUs) in
> user.school_classes must be in user.schools.
> 
> Something along:
> 
> assert user.school_classes == dict(
>     (sc, cl)
>     for sc, cl in user.school_classes.items()
>     if sc in user.schools
> )
> 
> Maybe replace "user.schools" with "user.schools + [user.school]" in case
> user.schools has not been updated yet...
> 
> As None is never in user.schools, it would fix this specific bug (None as
> key in school_classes) as a side effect of checking school_classes
> consistency.

I think that is already done here. The problem is the lower method which is called before the is in school_classes check even happens.
Comment 6 Daniel Tröder univentionstaff 2020-02-24 11:00:03 CET
(In reply to Jürn Brodersen from comment #5)
> I think that is already done here. The problem is the lower method which is
> called before the is in school_classes check even happens.
Oh, you're right - that is exactly what is done here.

I fixed the validation code in branch dtroeder/47712_school_classes_key_None:

It now first checks all entries in "schools" (as the super-method did for "school") and in school_classes, if they are existing OUs.

It then ignores the case of a non-string school in the following check (which is the one that crashed above).

[dtroeder/47712_school_classes_key_None 953b3cd46] Bug #47712: fix validation when None in school_classes keys
Comment 7 Tobias Wenzel univentionstaff 2020-03-10 15:39:02 CET
UCS version: 4.4-3 errata456

I used the suggested /var/lib/ucs-school-import/configs/user_import.json
and called 

./ucs-school-testuser-import --students 1 --classes 1 DEMOSCHOOL

Before [dtroeder/47712_school_classes_key_None 953b3cd46] I got the following expected result:


'NoneType' object has no attribute 'lower'
Traceback (most recent call last):

(...)

  File "/usr/lib/pymodules/python2.7/ucsschool/importer/models/import_user.py", line 1017, in validate
    super(ImportUser, self).validate(lo, validate_unlikely_changes)
  File "/usr/lib/pymodules/python2.7/ucsschool/lib/models/user.py", line 418, in validate
    if school.lower() not in (s.lower() for s in self.schools + [self.school]):
AttributeError: 'NoneType' object has no attribute 'lower'


with [dtroeder/47712_school_classes_key_None 953b3cd46] I got the following expected result:

Entry #0: ValidationError when adding ImportStudent(name='siegtrud.her', school='DEMOSCHOOL', dn='uid=siegtrud.her,cn=schueler,cn=users,ou=DEMOSCHOOL,dc=wenzel-univention,dc=intranet', old_dn=None) (source_uid:TESTID record_uid: Siegtrud.Herft): {'school_classes': ['The school None does not exist. Please choose an existing one or create it.']} ValidationError({'school_classes': ['The school None does not exist. Please choose an existing one or create it.']},)
Traceback (most recent call last):
  File "/usr/lib/pymodules/python2.7/ucsschool/importer/mass_import/user_import.py", line 179, in create_and_modify_users
    success = user.create(lo=self.connection)
  File "/usr/lib/pymodules/python2.7/ucsschool/importer/models/import_user.py", line 321, in create
    res = super(ImportUser, self).create(lo, validate)
  File "/usr/lib/pymodules/python2.7/ucsschool/lib/models/base.py", line 481, in create
    success = self.create_without_hooks(lo, validate)
  File "/usr/lib/pymodules/python2.7/ucsschool/lib/models/base.py", line 496, in create_without_hooks
    raise ValidationError(self.errors.copy())
UserValidationError: ValidationError when adding ImportStudent(name='siegtrud.her', school='DEMOSCHOOL', dn='uid=siegtrud.her,cn=schueler,cn=users,ou=DEMOSCHOOL,dc=wenzel-univention,dc=intranet', old_dn=None) (source_uid:TESTID record_uid: Siegtrud.Herft): {'school_classes': ['The school None does not exist. Please choose an existing one or create it.']} ValidationError({'school_classes': ['The school None does not exist. Please choose an existing one or create it.']},)
More than 0 errors.
Traceback (most recent call last):
  File "/usr/lib/pymodules/python2.7/ucsschool/importer/mass_import/mass_import.py", line 121, in import_users
    user_import.create_and_modify_users(imported_users)  # 90% - 100%
  File "/usr/lib/pymodules/python2.7/ucsschool/importer/mass_import/user_import.py", line 213, in create_and_modify_users
    self._add_error(exc)
  File "/usr/lib/pymodules/python2.7/ucsschool/importer/mass_import/user_import.py", line 630, in _add_error
    raise TooManyErrors("More than {} errors.".format(self.config["tolerate_errors"]), self.errors)