diff --git a/tabcmd/commands/user/create_site_users.py b/tabcmd/commands/user/create_site_users.py index c63436b0..3c64e6b8 100644 --- a/tabcmd/commands/user/create_site_users.py +++ b/tabcmd/commands/user/create_site_users.py @@ -41,7 +41,12 @@ def run_command(cls, args): UserCommand.validate_file_for_import(args.filename, logger, detailed=True, strict=args.require_all_valid) logger.info(_("addusers.status").format(args.filename.name, creation_site)) - user_obj_list = UserCommand.get_users_from_file(args.filename, logger) + # A --auth-type override replaces any CSV-supplied auth value, so suppress + # the CSV-time "Local" remap warning when one is given -- it would describe + # a value that's about to be discarded. + user_obj_list = UserCommand.get_users_from_file( + args.filename, logger, warn_on_local_remap=not getattr(args, "auth_type", None) + ) logger.info(_("session.monitorjob.percent_complete").format(0)) error_list = [] for user_obj in user_obj_list: @@ -49,7 +54,7 @@ def run_command(cls, args): if args.role: user_obj.site_role = args.role # tsc is case sensitive if args.auth_type: - user_obj.auth_setting = args.auth_type + user_obj.auth_setting = UserCommand.normalize_auth_setting(args.auth_type, user_obj.name, logger) number_of_users_listed += 1 result = server.users.add(user_obj) logger.info(_("common.output.succeeded").format(user_obj.name)) diff --git a/tabcmd/commands/user/create_users_command.py b/tabcmd/commands/user/create_users_command.py index 9dafcb3f..1bce4528 100644 --- a/tabcmd/commands/user/create_users_command.py +++ b/tabcmd/commands/user/create_users_command.py @@ -48,7 +48,12 @@ def run_command(cls, args): UserCommand.validate_file_for_import(args.filename, logger, detailed=True, strict=args.require_all_valid) logger.info(_("createusers.status").format(args.filename.name)) - user_obj_list = UserCommand.get_users_from_file(args.filename, logger) + # A --auth-type override replaces any CSV-supplied auth value, so suppress + # the CSV-time "Local" remap warning when one is given -- it would describe + # a value that's about to be discarded. + user_obj_list = UserCommand.get_users_from_file( + args.filename, logger, warn_on_local_remap=not getattr(args, "auth_type", None) + ) logger.info(_("session.monitorjob.percent_complete").format(0)) error_list = [] for user_obj in user_obj_list: @@ -57,7 +62,7 @@ def run_command(cls, args): if args.role: user_obj.site_role = args.role if args.auth_type: - user_obj.auth_setting = args.auth_type + user_obj.auth_setting = UserCommand.normalize_auth_setting(args.auth_type, user_obj.name, logger) server.users.add(user_obj) logger.info(_("common.output.succeeded").format(user_obj.name)) number_of_users_added += 1 diff --git a/tabcmd/commands/user/user_data.py b/tabcmd/commands/user/user_data.py index 272a5375..63656921 100644 --- a/tabcmd/commands/user/user_data.py +++ b/tabcmd/commands/user/user_data.py @@ -41,11 +41,12 @@ def populate(self, values: List[str]) -> None: if n_values >= 8: self.auth = values[7] - def to_tsc_user(self) -> TSC.UserItem: + def to_tsc_user(self, logger: Optional[logging.Logger] = None, warn_on_local_remap: bool = True) -> TSC.UserItem: site_role = UserCommand.evaluate_site_role(self.license_level, self.admin_level, self.publisher) if not site_role: raise AttributeError(_("tabcmd.user.error.site_role_required")) - user = TSC.UserItem(self.name, site_role, self.auth) + auth = UserCommand.normalize_auth_setting(self.auth, self.name, logger, warn=warn_on_local_remap) + user = TSC.UserItem(self.name, site_role, auth) user.email = self.email user.fullname = self.fullname return user @@ -104,6 +105,28 @@ class UserCommand(Server): This class acts as a base class for user related group of commands """ + # tabcmd Classic accepts "Local" as an auth type; TSC's UserItem.Auth enum + # has no Local value and rejects it. Map Classic's "Local" to ServerDefault + # (both for CSV values and for the CLI --auth-type override) so inputs + # authored for Classic import without crashing, and log a warning so the + # operator sees the remap happened. + # warn=False suppresses the warning for a remap whose result won't be the + # effective value -- e.g. a CSV row's "Local" that a CLI --auth-type + # override is about to replace. + @staticmethod + def normalize_auth_setting( + auth, username: Optional[str] = None, logger: Optional[logging.Logger] = None, warn: bool = True + ): + if isinstance(auth, str) and auth.lower() == "local": + if warn: + # Use the command's logger when given: it carries the console handlers, + # so the operator sees the warning on stderr, not only in tabcmd.log. + (logger or logging.getLogger(__name__)).warning( + _("tabcmd.user.warning.local_auth_remapped").format(username) + ) + return TSC.UserItem.Auth.ServerDefault + return auth + @staticmethod def set_role_arg(parser): parser.add_argument( @@ -193,7 +216,9 @@ def _validate_item(item: str, possible_values: List[str], column_type) -> None: raise AttributeError(_("commandlineutils.errors.bad_value").format(column_type, item, possible_values)) @staticmethod - def get_users_from_file(csv_file: io.TextIOWrapper, logger=None) -> List[TSC.UserItem]: + def get_users_from_file( + csv_file: io.TextIOWrapper, logger=None, warn_on_local_remap: bool = True + ) -> List[TSC.UserItem]: csv_file.seek(0) # set to start of file in case it has been read earlier if logger: logger.debug("Reading from file {}".format(csv_file.name)) @@ -202,14 +227,16 @@ def get_users_from_file(csv_file: io.TextIOWrapper, logger=None) -> List[TSC.Use if logger: logger.debug("> {}".format(line)) while line: - user: Optional[TSC.UserItem] = UserCommand._parse_line(line) + user: Optional[TSC.UserItem] = UserCommand._parse_line(line, logger, warn_on_local_remap) if user: user_list.append(user) line = csv_file.readline() return user_list @staticmethod - def _parse_line(line: str) -> Optional[TSC.UserItem]: + def _parse_line( + line: str, logger: Optional[logging.Logger] = None, warn_on_local_remap: bool = True + ) -> Optional[TSC.UserItem]: if line is None or line is False or line == "\n" or line == "": return None line = line.strip() @@ -217,7 +244,7 @@ def _parse_line(line: str) -> Optional[TSC.UserItem]: data = Userdata() values: List[str] = list(map(str.strip, line_parts)) data.populate(values) - return data.to_tsc_user() + return data.to_tsc_user(logger, warn_on_local_remap) # https://help.tableau.com/current/server/en-us/csvguidelines.htm#settings_and_site_roles @staticmethod diff --git a/tabcmd/locales/en/LC_MESSAGES/tabcmd.mo b/tabcmd/locales/en/LC_MESSAGES/tabcmd.mo index 85c42d08..64b3ad0a 100644 Binary files a/tabcmd/locales/en/LC_MESSAGES/tabcmd.mo and b/tabcmd/locales/en/LC_MESSAGES/tabcmd.mo differ diff --git a/tabcmd/locales/en/tabcmd_messages_en.properties b/tabcmd/locales/en/tabcmd_messages_en.properties index a1f7272a..3b898d3c 100644 --- a/tabcmd/locales/en/tabcmd_messages_en.properties +++ b/tabcmd/locales/en/tabcmd_messages_en.properties @@ -230,6 +230,7 @@ tabcmd.status.job_completed=Job completed tabcmd.status.waiting_for_refresh_job=Waiting for refresh job to begin tabcmd.user.error.site_role_required=Site role is required tabcmd.user.help.auth_type=Assigns the authentication type for all users in the CSV file. Possible values: +tabcmd.user.warning.local_auth_remapped=User '{0}': auth_setting 'Local' is not a Tableau Server auth type; mapped to ServerDefault. The site's default auth method will be used. tabcmd.user.help.site_role=Specifies a site role for all users in the .csv file. Possible roles: tabcmd.warning.calculations_not_supported=Adding or removing Calculations tasks are not supported tabcmdparser.global.behaviors=Global behaviors: diff --git a/tests/commands/test_user_utils.py b/tests/commands/test_user_utils.py index 67de8d56..7a41e67b 100644 --- a/tests/commands/test_user_utils.py +++ b/tests/commands/test_user_utils.py @@ -1,3 +1,4 @@ +import logging import unittest from unittest.mock import * from tabcmd.commands.user.user_data import UserCommand, Userdata @@ -150,3 +151,66 @@ def test_parse_line_preserves_role(self): user = UserCommand._parse_line("username, pword, fname, creator, none, yes, email") assert user is not None assert user.site_role == "Creator", f"Expected Creator, got {user.site_role}" + + def test_local_auth_maps_to_server_default(self): + # tabcmd Classic accepts "Local" as an auth type; TSC's Auth enum has no + # Local value, so passing it through raises ValueError on server.users.add. + # to_tsc_user should map Classic's "Local" -> ServerDefault for parity + # and log a warning so the operator sees the remap happened. + data = Userdata() + data.populate(["username", "pword", "fname", "creator", "none", "yes", "email", "Local"]) + with self.assertLogs("tabcmd.commands.user.user_data", level="WARNING") as logs: + user = data.to_tsc_user() + assert user.auth_setting == TSC.UserItem.Auth.ServerDefault, user.auth_setting + # Assert the warning names the affected user so the operator can act on + # it. Avoid coupling to the localize key -- once the .mo catalog carries + # this msgid, `_()` returns the translated text instead of the key. + assert any("username" in msg for msg in logs.output), logs.output + + def test_local_auth_case_insensitive(self): + data = Userdata() + data.populate(["username", "pword", "fname", "creator", "none", "yes", "email", "local"]) + with self.assertLogs("tabcmd.commands.user.user_data", level="WARNING"): + user = data.to_tsc_user() + assert user.auth_setting == TSC.UserItem.Auth.ServerDefault, user.auth_setting + + def test_non_local_auth_passes_through(self): + # SAML/OpenID/etc. are not remapped. + data = Userdata() + data.populate(["username", "pword", "fname", "creator", "none", "yes", "email", TSC.UserItem.Auth.SAML]) + user = data.to_tsc_user() + assert user.auth_setting == TSC.UserItem.Auth.SAML, user.auth_setting + + def test_local_auth_warning_uses_supplied_logger(self): + # The command logger carries the console handlers; the warning must go there. + data = Userdata() + data.populate(["username", "pword", "fname", "creator", "none", "yes", "email", "Local"]) + with self.assertLogs("tabcmd.test.command_logger", level="WARNING") as logs: + data.to_tsc_user(logging.getLogger("tabcmd.test.command_logger")) + assert any("username" in msg for msg in logs.output), logs.output + + def test_normalize_auth_setting_cli_value(self): + with self.assertLogs("tabcmd.test.command_logger", level="WARNING"): + result = UserCommand.normalize_auth_setting("Local", "bob", logging.getLogger("tabcmd.test.command_logger")) + assert result == TSC.UserItem.Auth.ServerDefault + + def test_local_auth_csv_warning_suppressed_when_cli_overrides(self): + # Regression: a CSV row of "Local" logged the remap warning during parsing + # even when a --auth-type CLI override was about to replace that value, + # describing a remap that never took effect. When a CLI override is + # coming, the CSV-time warning must be suppressed. + data = Userdata() + data.populate(["username", "pword", "fname", "creator", "none", "yes", "email", "Local"]) + with self.assertNoLogs("tabcmd.commands.user.user_data", level="WARNING"): + user = data.to_tsc_user(warn_on_local_remap=False) + assert user.auth_setting == TSC.UserItem.Auth.ServerDefault, user.auth_setting + + def test_local_auth_csv_warning_still_fires_without_override(self): + # Without a CLI override, the CSV's own "Local" value is effective, so the + # warning should still fire. + data = Userdata() + data.populate(["username", "pword", "fname", "creator", "none", "yes", "email", "Local"]) + with self.assertLogs("tabcmd.commands.user.user_data", level="WARNING") as logs: + user = data.to_tsc_user(warn_on_local_remap=True) + assert user.auth_setting == TSC.UserItem.Auth.ServerDefault, user.auth_setting + assert any("username" in msg for msg in logs.output), logs.output