Skip to content

fix(ssh_import_id): honor ssh_import_id on the default user - #7050

Open
Mohith1612 wants to merge 1 commit into
canonical:mainfrom
Mohith1612:fix-ssh-import-id-default-user
Open

fix(ssh_import_id): honor ssh_import_id on the default user#7050
Mohith1612 wants to merge 1 commit into
canonical:mainfrom
Mohith1612:fix-ssh-import-id-default-user

Conversation

@Mohith1612

Copy link
Copy Markdown
Contributor

Proposed Commit Message

fix(ssh_import_id): honor ssh_import_id on the default user

handle() read ssh_import_id from the top-level config when the user
was the default one, and from the user definition for everyone else.
An ssh_import_id set inside a default user definition was therefore
parsed, matched by the module's own activation check, and then
silently dropped.

Merge both sources for the default user. A top-level ssh_import_id
keeps working on its own, and non-default users are unaffected.

Fixes GH-4306

Additional Context

Fixes GH-4306.

handle() picks the source of ssh_import_id based on whether the user is
the default one:

if user_cfg["default"]:
    import_ids = util.get_cfg_option_list(cfg, "ssh_import_id", [])
else:
    import_ids = user_cfg["ssh_import_id"]

So ssh_import_id inside a default user definition is read by
normalize_users_groups, counted by the module's own
is_key_in_nested_dict activation check, and then dropped without a log
line. That is the surprise reported in the issue.

This uses util.uniq_merge rather than a plain concatenation because the key
also accepts a comma separated string, and merging keeps the existing
de-duplication.

Driving the real handle() with import_ssh_ids mocked, before and after:

config before after
user: {name: dave, ssh_import_id: [lp:user]} [] ['lp:user']
top-level and user-level both set ['lp:top'] ['lp:top', 'lp:user']
top-level only ['lp:top'] ['lp:top']
non-default user ['lp:bob'] ['lp:bob']

Only the first two rows change. A top-level ssh_import_id on its own is
untouched, and non-default users are untouched.

This does change behaviour on upgrade: an instance whose config sets
ssh_import_id inside the default user block will start importing keys that
were previously ignored. I think that is the point of the fix, since the
config was asking for exactly that, but it is worth calling out explicitly
given the module installs SSH keys.

Doc-only alternative. @TheRealFalcon's analysis on the issue said "At the
very least we need a doc change for this, but I think supporting the user
config makes more sense", so I went with the behaviour fix. If you would
rather keep the current behaviour, the alternative is to document that the
default user takes ssh_import_id only from the top level, and I am happy to
swap this PR for that instead. Nothing in the module docs or the schema
mentions the distinction today.

Test Steps

handle()'s user loop had no unit coverage, so this adds four parametrized
cases. Two of them fail without the code change and pass with it, and two are
controls that pass either way to show nothing else moved:

$ python -m pytest tests/unittests/config/test_cc_ssh_import_id.py -q -k ssh_import_id_sources
# without the fix
FAILED ...::test_ssh_import_id_sources[default_user_config_is_honored]
FAILED ...::test_ssh_import_id_sources[both_are_merged]
2 failed, 2 passed

# with the fix
4 passed

Full runs:

$ tox -e py3
5748 passed, 5 skipped, 13 xfailed, 10 warnings in 161.88s

$ tox -e check_format
ruff: All checks passed!
pylint: Your code has been rated at 10.00/10
black: 595 files would be left unchanged.
isort: Skipped 8 files
mypy: Success: no issues found in 591 source files
congratulations :)

Merge type

  • Squash merge using "Proposed Commit Message"
  • Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

handle() read ssh_import_id from the top-level config when the user
was the default one, and from the user definition for everyone else.
An ssh_import_id set inside a default user definition was therefore
parsed, matched by the module's own activation check, and then
silently dropped.

Merge both sources for the default user. A top-level ssh_import_id
keeps working on its own, and non-default users are unaffected.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ssh_import_id does not work on default user

1 participant