Skip to content

UserItem.CSVImport: several bugs prevent reliable use and block tabcmd delegation #1809

Description

@jacalata

Summary

UserItem.CSVImport was added to consolidate CSV user-import logic, but several bugs currently prevent it from being used as a reliable foundation. tabcmd duplicates the entire CSV stack independently (user_data.py) and cannot yet delegate to TSC without introducing regressions. This issue documents what needs to be fixed so that tabcmd can remove its duplicate code.


Bugs

1. Off-by-one on MAX makes the AUTH column unusable

ColumnType.AUTH = 7 and ColumnType.MAX = 7 have the same value. Both create_user_from_line and _validate_import_line_or_throw guard with len > MAX, which fires for any 8-element line — i.e. any line that includes the auth type column. The auth column defined in the spec is silently unreachable.

# user_item.py line ~446
if len(values) > UserItem.CSVImport.ColumnType.MAX:  # MAX=7, so 8-column lines always raise
    raise ValueError("Too many attributes for user import")

Fix: MAX should equal the number of columns (8), not the last index (7). Change to MAX = 8 and update bounds checks to >= MAX.


2. create_user_from_line lowercases the whole line including usernames

line = line.strip().lower()  # line ~442

This lowercases the username before assignment, which is destructive for case-sensitive authentication systems (e.g. LDAP) or mixed-case email-format usernames. Only the comparison-relevant fields (license, admin, publisher, auth) should be normalized.

Fix: lowercase per-field only for the fields that need it (license, admin, publisher, auth), not the entire line.


3. _valid_attributes auth column is missing TableauIDWithMFA

[UserItem.Auth.SAML, UserItem.Auth.OpenID, UserItem.Auth.ServerDefault],  # auth — line ~504

UserItem.Auth.TableauIDWithMFA is defined in the Auth class and is a valid value (documented in Tableau Online). It should be included in the validation list.


4. validate_file_for_import doesn't normalize case before validation

validate_file_for_import passes the raw (mixed-case) line directly to _validate_import_line_or_throw, whose _valid_attributes lists are all lowercase. So Viewer fails validation but viewer passes — but _evaluate_site_role handles both correctly. This means validate_file_for_import rejects CSVs that create_user_from_line would parse successfully.

Fix: lowercase the line (or individual non-username fields) in _validate_import_line_or_throw before comparison, matching the behaviour of create_user_from_line.


5. _set_values bypasses the @property_is_enum guard on auth_setting

create_user_from_line calls _set_values(...) which writes directly to self._auth_setting, skipping the @property_is_enum(Auth) setter. A CSV-parsed UserItem can therefore carry an invalid auth_setting that only fails at the API call, not at construction.

if auth_setting:
    self._auth_setting = auth_setting  # bypasses @property_is_enum

Fix: use self.auth_setting = auth_setting in _set_values, or validate explicitly before assignment. (Note: this requires fixing bug #3 first so TableauIDWithMFA is accepted.)


Context: tabcmd duplication

tabcmd (tabcmd/commands/user/user_data.py) contains a complete independent re-implementation of CSVImport: _parse_line, get_users_from_file, validate_file_for_import, _validate_user_or_throw, _validate_item, and evaluate_site_role. The role derivation function is copied character-for-character (including a # is this the expected outcome? comment on the Unlicensed fallback).

Once the above bugs are fixed, tabcmd can delete ~100 lines of duplicate code and delegate to UserItem.CSVImport. A follow-up tabcmd PR will do this once this issue is resolved.

Affected code

  • tableauserverclient/models/user_item.py — UserItem.CSVImport class (lines ~418–566)

Activity

  1. jacalata commented on Jul 28, 2026

    @jacalata
    ContributorAuthor

    Update: PR #1811 now covers all 5 bugs. Bug 5 (_set_values bypassing @property_is_enum) plus a related silent-failure bug discovered in review — create_user_from_line was silently setting auth_setting=None for unknown AUTH values instead of raising — both fixed in commit dfd5541.

    find_by_name was originally bundled here but has been moved out (closed as won't-fix; .filter(name=name) already covers it — see #1810 comment).

    Once #1811 merges, this issue can close. Filed #1836 for the follow-on bulk_add_from_file(filepath, strict=False) method that would let tabcmd delegate to TSC entirely.

  2. jacalata commented on Aug 17, 2026

    @jacalata
    ContributorAuthor

    PR #1811 fixes all six bugs. One trade-off worth recording explicitly on bug #5.

    The issue proposed routing _set_values through the @property_is_enum guard so any invalid auth_setting (from CSV or server response) would raise at construction. The PR started there, but reverted after Copilot flagged that server-parsed UserItems could break if Tableau ever introduces a new auth setting value not yet present in UserItem.Auth: parsing a response with the new value would raise instead of round-tripping.

    The final approach validates auth values at the CSV layer (_validate_import_line_or_throw + create_user_from_line) and leaves _set_values permissive so server responses continue to parse. Downside: a caller who constructs a UserItem manually and assigns via _set_values can still create an item with a garbage auth_setting. The public setter still enforces the enum, so user.auth_setting = "bogus" still raises.

    Net: CSV-import validation is now tight (goal of the issue); server-parse tolerance is preserved (goal of the compatibility guarantee); direct-write path via _set_values remains lenient by design.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions