Bug 47156 - [4.3] HTTP-API import doesn't handle hyphen in class name
[4.3] HTTP-API import doesn't handle hyphen in class name
Status: CLOSED FIXED
Product: UCS@school
Classification: Unclassified
Component: HTTP-API (Kelvin)
UCS@school 4.2
Other Linux
: P5 normal (vote)
: UCS@school 4.3 v4
Assigned To: Daniel Tröder
Ole Schwiegert
:
Depends on:
Blocks: 45019 47157
  Show dependency treegraph
 
Reported: 2018-06-07 14:24 CEST by Daniel Tröder
Modified: 2018-07-04 18:08 CEST (History)
1 user (show)

See Also:
What kind of report is it?: Bug Report
What type of bug is this?: 5: Major Usability: Impairs usability in key 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.171
Enterprise Customer affected?:
School Customer affected?: Yes
ISV affected?:
Waiting Support:
Flags outvoted (downgraded) after PO Review:
Ticket number:
Bug group (optional):
Max CVSS v3 score:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Tröder univentionstaff 2018-06-07 14:24:30 CEST
When a CSV file containing class names with a hyphen is imported, the part before the hyphen is interpreted as school name and an error occurs, because the school is (hopefully) unknown. The school is however known beforehand, because the HTTP-API import requires it.
Furthermore an import to a different school than the one configured is not allowed. So this is actually a security breach!
Comment 1 Daniel Tröder univentionstaff 2018-06-07 14:41:34 CEST
In the configuration was missing the use of a CSV reader class. This CSV reader makes sure that the school name is always prepended to the class name.

[4.3] 80e24877 Bug #47156: HTTP-API must always prepend school name to class names
[4.3] b63e3571 Bug #47156: advisory

ucs-school-import (16.0.2-5)
Comment 2 Daniel Tröder univentionstaff 2018-06-07 14:55:49 CEST
TODO:
* Document class-without-school in documentation
* create a ucs-test for input data 
  1. without "$OU-" in the class column
  2. with correct "$OU-" in the class column
  3. with incorrect "$OU-" in the class column
Only case 1 is correct.
Comment 3 Daniel Tröder univentionstaff 2018-06-07 21:40:53 CEST
(In reply to Daniel Tröder from comment #2)
> TODO:
> * Document class-without-school in documentation

[4.3] 58c4ecff Bug #47156: add section about CSV file format and warning about class names and school

ucs-school-import (16.0.2-6)
Comment 4 Daniel Tröder univentionstaff 2018-06-07 22:07:01 CEST
(In reply to Daniel Tröder from comment #3)
> [4.3] 58c4ecff Bug #47156: add section about CSV file format and warning
> about class names and school
> 
> ucs-school-import (16.0.2-6)
See https://billy.knut.univention.de/~dtroeder/http-api-doc/file-locations.html
Comment 5 Daniel Tröder univentionstaff 2018-06-11 13:15:01 CEST
The reader class for CSV files is now configured by /usr/share/ucs-school-import/configs/user_import_http-api.json, overwritable by /var/lib/ucs-school-import/configs/user_import_http-api.json, which are now both always read.

Additionally the config is checked at runtime, to verify that the used reader is ucsschool.importer.reader.http_api_csv_reader.HttpApiCsvReader or a cubcloass of it.

(In reply to Daniel Tröder from comment #2)
> * create a ucs-test for input data 
>   1. without "$OU-" in the class column
>   2. with correct "$OU-" in the class column
>   3. with incorrect "$OU-" in the class column
> Only case 1 is correct.
The test 302_http-api_class_column_no_school was added.

[4.3] 936487f3 Bug #47156: create HTTP-API test class
[4.3] f5c45906 Bug #47156: add check that school names in classes column are not used
[4.3] 1ea9f24f Bug #47156: add docstring
[4.3] 2f52a69e Bug #47156: force use of HttpApiCsvReader
[4.3] 904f3cd4 Bug #47156: changelog
[4.3] 4e6971aa Bug #47156: always use default and custom user_import_http-api.json, check class of active CSV reader
[4.3] 60b3d0f5 Bug #47156: changelog
[4.3] 5fea7d63 Bug #47156: advisory

ucs-school-import (16.0.2-8)
ucs-test-ucsschool (5.0.2-56)
Comment 6 Ole Schwiegert univentionstaff 2018-06-12 10:38:27 CEST
Tested with version 16.0.2-9A~4.3.0.201806111817

REOP /var/lib/ucs-school-import/configs/user_import_http_api.json.json is created
--> one .json to much I presume

REOP You can still have the school name in the class name. Example:

Orig file:

"Benutzertyp","Schule","Vorname","Nachname","Klassen","Beschreibung","Telefon","EMail"
"student","AE","Otfrid","Kuester","1a","A student.","+22-345-721521","otfridm.kuesterm@realm2.intranet"
"student","AE","Arda","Skewes","1b","A student.","+08-386-37468","ardam.skewesm@realm2.intranet"

Report:

"line","success","error","action","role","username","schools","firstname","lastname","birthday","email","disabled","classes","source_uid","record_uid","error_msg"
"2","1","0","A","student","otfrid.kuest","AE","Otfrid","Kuester","","otfridm.kuesterm@realm2.intranet","0","AE-AE-1a","ae-student","Otfrid.Kuester",""
"3","1","0","A","student","arda.skewes","AE","Arda","Skewes","","ardam.skewesm@realm2.intranet","0","AE-AE-1b","ae-student","Arda.Skewes",""

Is this correct (so we only warn about prepending the school name anyway) or was the dry-run supposed to fail if the school name was contained in the class name?
Comment 7 Ole Schwiegert univentionstaff 2018-06-12 10:41:42 CEST
Also my custom /var/lib/ucs-school-import/configs/user_import_http-api.json was not read in the import process

2018-06-12 10:30:43 INFO  cmdline.main:106  ------ UCS@school import tool starting ------
2018-06-12 10:30:43 INFO  http_api_import_frontend.configuration_files:145  Copied '/usr/share/ucs-school-import/configs/global_defaults.json' to u'/var/lib/ucs-school-impo$
2018-06-12 10:30:43 INFO  http_api_import_frontend.configuration_files:145  Copied '/var/lib/ucs-school-import/configs/global.json' to u'/var/lib/ucs-school-import/jobs/201$
2018-06-12 10:30:43 INFO  http_api_import_frontend.configuration_files:145  Copied '/usr/share/ucs-school-import/configs/user_import_defaults.json' to u'/var/lib/ucs-school$
2018-06-12 10:30:43 INFO  http_api_import_frontend.configuration_files:145  Copied '/var/lib/ucs-school-import/configs/user_import.json' to u'/var/lib/ucs-school-import/job$
2018-06-12 10:30:43 INFO  http_api_import_frontend.configuration_files:145  Copied u'/usr/share/ucs-school-import/configs/user_import_http-api.json' to u'/var/lib/ucs-schoo$
2018-06-12 10:30:43 WARNING http_api_import_frontend.configuration_files:149  Ignoring not existing configuration file u'/var/lib/ucs-school-import/configs/user_import_http$
2018-06-12 10:30:43 INFO  http_api_import_frontend.configuration_files:162  No school specific configuration found (u'/var/lib/ucs-school-import/configs/ae.json').
2018-06-12 10:30:43 INFO  configuration.read:87  Reading configuration from u'/var/lib/ucs-school-import/jobs/2018/3/0_global_defaults.json'...
2018-06-12 10:30:43 INFO  configuration.read:87  Reading configuration from u'/var/lib/ucs-school-import/jobs/2018/3/1_global.json'...
2018-06-12 10:30:43 INFO  configuration.read:87  Reading configuration from u'/var/lib/ucs-school-import/jobs/2018/3/2_user_import_defaults.json'...
2018-06-12 10:30:43 INFO  configuration.read:87  Reading configuration from u'/var/lib/ucs-school-import/jobs/2018/3/3_user_import.json'...
2018-06-12 10:30:43 INFO  configuration.read:87  Reading configuration from u'/var/lib/ucs-school-import/jobs/2018/3/4_user_import_http-api.json'...
Comment 8 Daniel Tröder univentionstaff 2018-06-12 12:07:33 CEST
(In reply to Ole Schwiegert from comment #6)
> REOP /var/lib/ucs-school-import/configs/user_import_http_api.json.json is
> created
> --> one .json to much I presume
fixed

> REOP You can still have the school name in the class name. Example:
The school name is allowed in the class name. We cannot know what class names are used. The bug is about handling such a case correctly.
Two requirements:
* don't unnecessarily shrink namespace for classes
* don't allow to import a class into a school which is not the one configured for the current import job

So we simply prepend the school name to whatever we find in the class column.

> Is this correct (so we only warn about prepending the school name anyway) or
> was the dry-run supposed to fail if the school name was contained in the
> class name?
IMHO the school name should be allowed in a class name. But a warning is a good idea. I will add it to Bug #45715 comment8.

[4.3] 5020c6e0 Bug #47156: fix double .json, strip whitespace from class names, remove test class
[4.3] 5e7980ed Bug #47156: changelog
[4.3] 250e7a1e Bug #47156: advisory update

ucs-school-import (16.0.2-11)

(In reply to Ole Schwiegert from comment #7)
> Also my custom /var/lib/ucs-school-import/configs/user_import_http-api.json
> was not read in the import process
> 
[..]
> 2018-06-12 10:30:43 WARNING http_api_import_frontend.configuration_files:149
> Ignoring not existing configuration file
> u'/var/lib/ucs-school-import/configs/user_import_http$

The terminal copy&paste was cut off. Was it the .json.json file?
Comment 9 Ole Schwiegert univentionstaff 2018-06-14 09:54:31 CEST
I do not have the original log anymore, sorry.

But with the new version I can confirm, that
the log file is now properly created and imported OK
school name is added as prefix OK
import ran successfully
Manual was adapted OK
reader checked OK
changelog OK
advisory OK
Comment 10 Sönke Schwardt-Krummrich univentionstaff 2018-07-04 18:08:49 CEST
UCS@school 4.3 v4 has been released.

https://docs.software-univention.de/changelog-ucsschool-4.3v4-de.html

If this error occurs again, please clone this bug.