diff --git a/CHANGELOG.md b/CHANGELOG.md index 943436b27..71d3a0a01 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,16 @@ hierarchy path (e.g. `"Marketing/Q1 Reports"`). The walk is performed level by level using the REST API name filter, so a path with *n* components issues *n* requests. Returns the matching `ProjectItem` or `None` if no project is found. +* **Behavior change**: `UserItem.CSVImport.create_user_from_line` no longer + lowercases the entire CSV line before parsing. Previously the whole line, + including the username, display name, fullname, and email fields, was + lowercased destructively (e.g. `JSmith` became `jsmith`). Case is now + preserved for those fields; only the comparison-relevant fields (license, + admin_level, publisher, auth_setting) are normalized internally for + validation. Callers relying on the previous lowercased output — e.g. dict + lookups keyed on `user.name`, or assertions against lowercased values — + need to update. This unblocks CSV imports for LDAP and other case-sensitive + auth backends where mixed-case usernames must be preserved. ## 0.18.0 (6 April 2022) * Switched to using defused_xml for xml attack protection diff --git a/tableauserverclient/models/user_item.py b/tableauserverclient/models/user_item.py index 0ba1e8eb2..7309632cf 100644 --- a/tableauserverclient/models/user_item.py +++ b/tableauserverclient/models/user_item.py @@ -308,7 +308,10 @@ def _set_values( if email: self.email = email if auth_setting: - self._auth_setting = auth_setting + # Go through the @property_is_enum(Auth) setter rather than writing + # to _auth_setting directly, so CSV-parsed users can't carry an + # invalid auth_setting that only fails later at the API call. + self.auth_setting = auth_setting if domain_name: self._domain_name = domain_name if locale: @@ -432,36 +435,59 @@ class ColumnType(IntEnum): EMAIL = 6 AUTH = 7 - MAX = 7 + # Total number of columns supported by the import format. Held outside + # the ColumnType enum so it can't be mistaken for a real column index. + COLUMN_COUNT = 8 + + # Lowercase -> canonical form mapping for the AUTH column. Class-level + # so the dict isn't rebuilt on every call to create_user_from_line / + # _validate_import_line_or_throw. The set of accepted values is derived + # from this map (see _valid_attributes[AUTH]) so there's a single + # source of truth. + _AUTH_CANONICAL: dict[str, str] = { + "saml": "SAML", + "openid": "OpenID", + "serverdefault": "ServerDefault", + "tableauidwithmfa": "TableauIDWithMFA", + } # Read a csv line and create a user item populated by the given attributes @staticmethod def create_user_from_line(line: str): if line is None or line is False or line == "\n" or line == "": return None - line = line.strip().lower() - values: list[str] = list(map(str.strip, line.split(","))) - user = UserItem(values[UserItem.CSVImport.ColumnType.USERNAME]) + values: list[str] = list(map(str.strip, line.strip().split(","))) + if len(values) > UserItem.CSVImport.COLUMN_COUNT: + raise ValueError("Too many attributes for user import") + username = values[UserItem.CSVImport.ColumnType.USERNAME] + user = UserItem(username) if len(values) > 1: - if len(values) > UserItem.CSVImport.ColumnType.MAX: - raise ValueError("Too many attributes for user import") - while len(values) <= UserItem.CSVImport.ColumnType.MAX: + while len(values) < UserItem.CSVImport.COLUMN_COUNT: values.append("") site_role = UserItem.CSVImport._evaluate_site_role( values[UserItem.CSVImport.ColumnType.LICENSE], values[UserItem.CSVImport.ColumnType.ADMIN], values[UserItem.CSVImport.ColumnType.PUBLISHER], ) - + raw_auth = values[UserItem.CSVImport.ColumnType.AUTH] + if raw_auth: + auth = UserItem.CSVImport._AUTH_CANONICAL.get(raw_auth.lower()) + if auth is None: + raise ValueError( + f"Unknown auth setting: {raw_auth!r}. " + f"Valid values: {sorted(UserItem.CSVImport._AUTH_CANONICAL.values())}" + ) + else: + auth = None user._set_values( None, - values[UserItem.CSVImport.ColumnType.USERNAME], + username, site_role, None, None, values[UserItem.CSVImport.ColumnType.DISPLAY_NAME], values[UserItem.CSVImport.ColumnType.EMAIL], - values[UserItem.CSVImport.ColumnType.AUTH], + auth, None, None, None, @@ -493,6 +519,8 @@ def validate_file_for_import(csv_file: io.TextIOWrapper, logger) -> tuple[int, l # Iterate through each field and validate the given value against hardcoded constraints @staticmethod def _validate_import_line_or_throw(incoming, logger) -> None: + # AUTH column's valid set is derived from _AUTH_CANONICAL so there's + # one source of truth for the accepted values. _valid_attributes: list[list[str]] = [ [], [], @@ -501,20 +529,26 @@ def _validate_import_line_or_throw(incoming, logger) -> None: ["system", "site", "none", "no"], # admin ["yes", "true", "1", "no", "false", "0"], # publisher [], - [UserItem.Auth.SAML, UserItem.Auth.OpenID, UserItem.Auth.ServerDefault], # auth + list(UserItem.CSVImport._AUTH_CANONICAL.values()), # auth — normalized before comparison ] line = list(map(str.strip, incoming.split(","))) - if len(line) > UserItem.CSVImport.ColumnType.MAX: + if len(line) > UserItem.CSVImport.COLUMN_COUNT: raise AttributeError("Too many attributes in line") username = line[UserItem.CSVImport.ColumnType.USERNAME.value] logger.debug(f"> details - {username}") UserItem.validate_username_or_throw(username) for i in range(1, len(line)): - logger.debug(f"column {UserItem.CSVImport.ColumnType(i).name}: {line[i]}") - UserItem.CSVImport._validate_attribute_value( - line[i], _valid_attributes[i], UserItem.CSVImport.ColumnType(i) - ) + value = line[i] + valid = _valid_attributes[i] + # normalize case for fields with a restricted value set + if valid: + if i == UserItem.CSVImport.ColumnType.AUTH: + value = UserItem.CSVImport._AUTH_CANONICAL.get(value.lower(), value) + else: + value = value.lower() + logger.debug(f"column {UserItem.CSVImport.ColumnType(i).name}: {value}") + UserItem.CSVImport._validate_attribute_value(value, valid, UserItem.CSVImport.ColumnType(i)) # Given a restricted set of possible values, confirm the item is in that set @staticmethod diff --git a/test/test_user_model.py b/test/test_user_model.py index 49e8dc25c..212b82518 100644 --- a/test/test_user_model.py +++ b/test/test_user_model.py @@ -136,3 +136,58 @@ def test_validate_usernames_file() -> None: test_data = _mock_file_content(usernames) valid, invalid = TSC.UserItem.CSVImport.validate_file_for_import(test_data, logger) assert valid == 5, f"Exactly 5 of the lines were valid, counted {valid + len(invalid)}" + + +def test_validate_mixed_case_license() -> None: + # Regression: issue #1809 — 'Viewer' (capital V) was rejected by case-sensitive check + TSC.UserItem.CSVImport._validate_import_line_or_throw("username, pword, fname, Viewer, None, no, email", logger) + TSC.UserItem.CSVImport._validate_import_line_or_throw("username, pword, fname, Creator, Site, yes, email", logger) + TSC.UserItem.CSVImport._validate_import_line_or_throw("username, pword, fname, EXPLORER, NONE, YES, email", logger) + + +def test_validate_tableauid_with_mfa_auth() -> None: + # TableauIDWithMFA is a valid auth value and must not be rejected + TSC.UserItem.CSVImport._validate_import_line_or_throw( + "username, pword, fname, creator, none, yes, email, TableauIDWithMFA", logger + ) + + +def test_create_user_preserves_username_case() -> None: + # Username must not be lowercased — case matters for LDAP and email-format usernames + user = TSC.UserItem.CSVImport.create_user_from_line("JSmith, pword, John Smith, creator, none, yes, j@example.com") + assert user is not None + assert user.name == "JSmith", f"Username was lowercased: {user.name}" + + +def test_create_user_with_auth_column() -> None: + # AUTH column (position 7) must be parsed — was broken by MAX=7 off-by-one + user = TSC.UserItem.CSVImport.create_user_from_line("username, pword, fname, creator, none, yes, email, SAML") + assert user is not None + assert user.auth_setting == "SAML", f"Expected SAML, got {user.auth_setting}" + + +def test_too_many_columns_raises() -> None: + with pytest.raises(ValueError): + TSC.UserItem.CSVImport.create_user_from_line("u, p, n, creator, none, yes, email, SAML, extra") + + +def test_create_user_with_unknown_auth_raises() -> None: + # Unknown AUTH values must raise, not silently produce a UserItem with auth_setting=None. + # A caller can catch this if lenient behavior is wanted. + with pytest.raises(ValueError, match="Unknown auth setting"): + TSC.UserItem.CSVImport.create_user_from_line("username, pword, fname, creator, none, yes, email, NotAnAuthType") + + +def test_create_user_with_lowercase_auth_accepted() -> None: + # AUTH values are canonicalized case-insensitively — 'saml' should produce 'SAML'. + user = TSC.UserItem.CSVImport.create_user_from_line("username, pword, fname, creator, none, yes, email, saml") + assert user is not None + assert user.auth_setting == "SAML" + + +def test_set_values_rejects_invalid_auth_setting() -> None: + # _set_values must route auth_setting through the @property_is_enum guard. + # Attempting to set an invalid auth string must raise, not silently succeed. + user = TSC.UserItem("someone") + with pytest.raises(ValueError): + user._set_values(None, None, None, None, None, None, None, "NotAnAuthType", None, None, None, None)