Skip to content

Commit 117cbac

Browse files
jacalataclaude
andcommitted
refactor: CSVImport MAX/auth cleanup from adversarial review
Three small cleanups on UserItem.CSVImport surfaced by an adversarial code review of the earlier bug fixes: - MAX renamed to COLUMN_COUNT and moved out of the ColumnType IntEnum. ColumnType(8) used to return ColumnType.MAX, a fake column mixed in with real column indices. Now the count is a class-level constant. - _auth_canonical() no longer rebuilds its dict on every call. Promoted to _AUTH_CANONICAL class attribute. - _valid_attributes[AUTH] no longer hardcodes the accepted auth values. Derived from _AUTH_CANONICAL.values() instead so there's a single source of truth for what AUTH strings are accepted. No behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 95c75da commit 117cbac

1 file changed

Lines changed: 24 additions & 23 deletions

File tree

tableauserverclient/models/user_item.py

Lines changed: 24 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -435,20 +435,34 @@ class ColumnType(IntEnum):
435435
EMAIL = 6
436436
AUTH = 7
437437

438-
MAX = 8 # total number of columns (not last index)
438+
# Total number of columns supported by the import format. Held outside
439+
# the ColumnType enum so it can't be mistaken for a real column index.
440+
COLUMN_COUNT = 8
441+
442+
# Lowercase -> canonical form mapping for the AUTH column. Class-level
443+
# so the dict isn't rebuilt on every call to create_user_from_line /
444+
# _validate_import_line_or_throw. The set of accepted values is derived
445+
# from this map (see _valid_attributes[AUTH]) so there's a single
446+
# source of truth.
447+
_AUTH_CANONICAL: dict[str, str] = {
448+
"saml": "SAML",
449+
"openid": "OpenID",
450+
"serverdefault": "ServerDefault",
451+
"tableauidwithmfa": "TableauIDWithMFA",
452+
}
439453

440454
# Read a csv line and create a user item populated by the given attributes
441455
@staticmethod
442456
def create_user_from_line(line: str):
443457
if line is None or line is False or line == "\n" or line == "":
444458
return None
445459
values: list[str] = list(map(str.strip, line.strip().split(",")))
446-
if len(values) > UserItem.CSVImport.ColumnType.MAX:
460+
if len(values) > UserItem.CSVImport.COLUMN_COUNT:
447461
raise ValueError("Too many attributes for user import")
448462
username = values[UserItem.CSVImport.ColumnType.USERNAME]
449463
user = UserItem(username)
450464
if len(values) > 1:
451-
while len(values) < UserItem.CSVImport.ColumnType.MAX:
465+
while len(values) < UserItem.CSVImport.COLUMN_COUNT:
452466
values.append("")
453467
site_role = UserItem.CSVImport._evaluate_site_role(
454468
values[UserItem.CSVImport.ColumnType.LICENSE],
@@ -457,11 +471,11 @@ def create_user_from_line(line: str):
457471
)
458472
raw_auth = values[UserItem.CSVImport.ColumnType.AUTH]
459473
if raw_auth:
460-
auth = UserItem.CSVImport._auth_canonical().get(raw_auth.lower())
474+
auth = UserItem.CSVImport._AUTH_CANONICAL.get(raw_auth.lower())
461475
if auth is None:
462476
raise ValueError(
463477
f"Unknown auth setting: {raw_auth!r}. "
464-
f"Valid values: {sorted(UserItem.CSVImport._auth_canonical().values())}"
478+
f"Valid values: {sorted(UserItem.CSVImport._AUTH_CANONICAL.values())}"
465479
)
466480
else:
467481
auth = None
@@ -503,18 +517,10 @@ def validate_file_for_import(csv_file: io.TextIOWrapper, logger) -> tuple[int, l
503517

504518
# Some fields in the import file are restricted to specific values
505519
# Iterate through each field and validate the given value against hardcoded constraints
506-
@staticmethod
507-
def _auth_canonical() -> dict[str, str]:
508-
"""Lowercase → canonical form mapping for Auth values."""
509-
return {
510-
"saml": "SAML",
511-
"openid": "OpenID",
512-
"serverdefault": "ServerDefault",
513-
"tableauidwithmfa": "TableauIDWithMFA",
514-
}
515-
516520
@staticmethod
517521
def _validate_import_line_or_throw(incoming, logger) -> None:
522+
# AUTH column's valid set is derived from _AUTH_CANONICAL so there's
523+
# one source of truth for the accepted values.
518524
_valid_attributes: list[list[str]] = [
519525
[],
520526
[],
@@ -523,16 +529,11 @@ def _validate_import_line_or_throw(incoming, logger) -> None:
523529
["system", "site", "none", "no"], # admin
524530
["yes", "true", "1", "no", "false", "0"], # publisher
525531
[],
526-
[
527-
"SAML",
528-
"OpenID",
529-
"ServerDefault",
530-
"TableauIDWithMFA",
531-
], # auth — normalized by _auth_canonical before comparison
532+
list(UserItem.CSVImport._AUTH_CANONICAL.values()), # auth — normalized before comparison
532533
]
533534

534535
line = list(map(str.strip, incoming.split(",")))
535-
if len(line) > UserItem.CSVImport.ColumnType.MAX:
536+
if len(line) > UserItem.CSVImport.COLUMN_COUNT:
536537
raise AttributeError("Too many attributes in line")
537538
username = line[UserItem.CSVImport.ColumnType.USERNAME.value]
538539
logger.debug(f"> details - {username}")
@@ -543,7 +544,7 @@ def _validate_import_line_or_throw(incoming, logger) -> None:
543544
# normalize case for fields with a restricted value set
544545
if valid:
545546
if i == UserItem.CSVImport.ColumnType.AUTH:
546-
value = UserItem.CSVImport._auth_canonical().get(value.lower(), value)
547+
value = UserItem.CSVImport._AUTH_CANONICAL.get(value.lower(), value)
547548
else:
548549
value = value.lower()
549550
logger.debug(f"column {UserItem.CSVImport.ColumnType(i).name}: {value}")

0 commit comments

Comments
 (0)