Add automatic credential refresh for AWS SSO#315
Add automatic credential refresh for AWS SSO#315borland667 wants to merge 1 commit intobinbashar:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a bulk AWS SSO credential refresh feature with helpers for expiration checks, lazy credential updaters, per-profile refresh orchestration, CLI hooks (login flag + new sso refresh command), refined subcommand invocation, and tests covering multi-account flows and error cases. Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as CLI (leverage/modules/aws.py)
participant Auth as Auth Module (leverage/modules/auth.py)
participant SSO as AWS SSO/API
participant FS as Filesystem
User->>CLI: run `sso login --refresh-all` or `sso refresh --force`
CLI->>Auth: refresh_all_accounts_credentials(force_refresh)
Auth->>Auth: discover SSO profile sections for project
loop per SSO profile
Auth->>Auth: _check_credentials_expiration(layer_profile, force_refresh)
alt needs refresh
Auth->>SSO: _get_sso_token_or_raise -> obtain access token (if needed)
Auth->>SSO: request role credentials for account_id + sso_role
alt credentials returned
Auth->>FS: update config section with new expiration via ConfigUpdater
Auth->>FS: write AWS credentials to credentials file via credentials_updater
Auth->>CLI: log "refreshed" for profile
else permission or token error
Auth->>CLI: log "error" for profile (raise or warn based on flag)
end
else skip
Auth->>CLI: log "skipped" for profile
end
end
Auth->>CLI: return summary of refreshed/skipped/errors
CLI->>User: display results / exit with code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
leverage/modules/utils.py (1)
31-33: Exit code check is inverted; raises on success.Currently raises Exit when exit_code is 0 (success). Flip the condition.
Apply this diff:
- if not exit_code: - raise Exit(exit_code) + if exit_code: + raise Exit(exit_code)
🧹 Nitpick comments (7)
leverage/modules/utils.py (2)
39-51: Show help with a subcommand context.Use make_context for accurate command path/options; return after printing help.
Apply this diff:
- if is_help_request and (not is_group or len(remaining_args) == 1): - # Show help for the subcommand - click.echo(subcommand_obj.get_help(context)) + if is_help_request and (not is_group or len(remaining_args) == 1): + # Show help for the subcommand with its own context + with subcommand_obj.make_context(subcommand, [], parent=context) as sub_ctx: + click.echo(subcommand_obj.get_help(sub_ctx)) + return
55-63: Guard callback None and avoid TypeError with inspect.signature.Some Click groups may have no callback; handle that path and prefer direct pass-through for groups/args-style callbacks.
Apply this diff:
- sig = inspect.signature(subcommand_obj.callback) - if "args" in sig.parameters: - # Pass the remaining args to the subcommand (for groups) - context.invoke(subcommand_obj, args=remaining_args) - else: - # Create a new context with remaining args so Click can parse options - with subcommand_obj.make_context(subcommand, list(remaining_args), parent=context) as sub_ctx: - with context.scope(cleanup=False): - subcommand_obj.invoke(sub_ctx) + callback = getattr(subcommand_obj, "callback", None) + if callback is None: + # Group without a callback; pass args directly. + context.invoke(subcommand_obj, args=remaining_args) + return + sig = inspect.signature(callback) + if "args" in sig.parameters or is_group: + # Pass the remaining args directly (common for groups). + context.invoke(subcommand_obj, args=remaining_args) + else: + # Create a new context so Click can parse options. + with subcommand_obj.make_context(subcommand, list(remaining_args), parent=context) as sub_ctx: + with context.scope(cleanup=False): + subcommand_obj.invoke(sub_ctx)leverage/modules/auth.py (3)
191-196: Preserve original cause when wrapping token errors.Re-raise with context to aid debugging; optional to narrow exception types.
Apply this diff:
- try: - access_token = cli.get_sso_access_token() - except Exception as e: - raise ExitError(1, f"Failed to get SSO access token: {e}") + try: + access_token = cli.get_sso_access_token() + except Exception as e: + raise ExitError(1, f"Failed to get SSO access token: {e}") from eOptionally narrow to specific exceptions (e.g., FileNotFoundError, KeyError, json.JSONDecodeError).
239-247: Remove unnecessary else after continue.Slight simplification and aligns with lints.
Apply this diff:
- except ClientError as error: - if error.response["Error"]["Code"] in ("AccessDeniedException", "ForbiddenException"): - logger.warning( - f"No permission to assume role [bold]{sso_role}[/bold] in {account_name} account. Skipping." - ) - error_count += 1 - continue - else: - raise + except ClientError as error: + if error.response["Error"]["Code"] in ("AccessDeniedException", "ForbiddenException"): + logger.warning( + f"No permission to assume role [bold]{sso_role}[/bold] in {account_name} account. Skipping." + ) + error_count += 1 + continue + raise
177-186: Defensive handling of ConfigUpdater.sections() return type.If sections() yields Section objects, use .name for matching.
Apply this diff:
- sso_profiles = [] - for section in config_updater.sections(): - if section.startswith(f"profile {cli.project}-sso-") and section != f"profile {cli.project}-sso": - sso_profiles.append(section) + sso_profiles = [] + for section in config_updater.sections(): + section_name = section if isinstance(section, str) else getattr(section, "name", str(section)) + if section_name.startswith(f"profile {cli.project}-sso-") and section_name != f"profile {cli.project}-sso": + sso_profiles.append(section_name)tests/test_modules/test_auth.py (2)
341-347: Remove unused helper or silence ARG001.read_text_side_effect_multi is unused; delete it or prefix with underscore to silence Ruff.
Apply this diff to remove:
-def read_text_side_effect_multi(self: PosixPath, *args, **kwargs): - """ - Side effect for reading multi-account config files. - """ - return data_dict_multi.get(str(self), data_dict_multi.get(self.name, ""))
374-407: Prefix unused patched args to satisfy Ruff (ARG001).Several tests receive mocks they don't use (mock_open, mock_boto, mock_update_conf). Prefix with underscores or add noqa to signatures.
Example:
-def test_refresh_all_accounts_credentials_success(mock_open, mock_boto, mock_update_conf, sso_container, caplog): +def test_refresh_all_accounts_credentials_success(_mock_open, _mock_boto, _mock_update_conf, sso_container, caplog):Repeat similarly for other tests flagged by ARG001.
Also applies to: 412-437, 461-480, 486-535, 539-561, 568-597
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
leverage/modules/auth.py(1 hunks)leverage/modules/aws.py(4 hunks)leverage/modules/utils.py(3 hunks)tests/test_modules/test_auth.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
leverage/modules/auth.py (4)
leverage/logger.py (4)
info(116-118)warning(122-124)debug(110-112)error(128-130)leverage/path.py (2)
host_aws_profiles_file(203-204)host_aws_credentials_file(207-208)leverage/container.py (2)
sso_region_from_main_profile(327-333)get_sso_access_token(322-324)leverage/_utils.py (1)
ExitError(109-116)
tests/test_modules/test_auth.py (4)
leverage/modules/auth.py (4)
refresh_layer_credentials(83-157)get_layer_profile(17-46)SkipProfile(13-14)refresh_all_accounts_credentials(160-275)leverage/container.py (1)
get_sso_access_token(322-324)leverage/_utils.py (1)
ExitError(109-116)leverage/path.py (2)
host_aws_profiles_file(203-204)host_aws_credentials_file(207-208)
leverage/modules/aws.py (3)
leverage/_utils.py (2)
get_or_create_section(181-185)ExitError(109-116)leverage/modules/auth.py (1)
refresh_all_accounts_credentials(160-275)leverage/_internals.py (1)
pass_container(44-53)
leverage/modules/utils.py (1)
tests/conftest.py (1)
context(29-34)
🪛 Pylint (4.0.1)
leverage/modules/auth.py
[refactor] 160-160: Too many local variables (21/15)
(R0914)
[refactor] 239-246: Unnecessary "else" after "continue", remove the "else" and de-indent the code inside it
(R1724)
[refactor] 160-160: Too many statements (54/50)
(R0915)
leverage/modules/aws.py
[error] 225-225: Instance of 'ExitError' has no 'code' member
(E1101)
[error] 255-255: Instance of 'ExitError' has no 'code' member
(E1101)
leverage/modules/utils.py
[error] 1-1: Unrecognized option found: suggestion-mode
(E0015)
[refactor] 1-1: Useless option value for '--disable', 'print-statement' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'parameter-unpacking' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'unpacking-in-except' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'old-raise-syntax' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'backtick' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'import-star-module-level' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'apply-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'basestring-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'buffer-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'cmp-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'coerce-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'execfile-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'file-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'long-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'raw_input-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'reduce-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'standarderror-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'unicode-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'xrange-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'coerce-method' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'delslice-method' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'getslice-method' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'setslice-method' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'no-absolute-import' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'old-division' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'dict-iter-method' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'dict-view-method' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'next-method-called' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'metaclass-assignment' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'indexing-exception' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'raising-string' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'reload-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'oct-method' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'hex-method' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'nonzero-method' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'cmp-method' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'input-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'round-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'intern-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'unichr-builtin' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'map-builtin-not-iterating' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'zip-builtin-not-iterating' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'range-builtin-not-iterating' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'filter-builtin-not-iterating' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'using-cmp-argument' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'div-method' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'idiv-method' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'rdiv-method' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'exception-message-attribute' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'invalid-str-codec' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'sys-max-int' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'bad-python3-import' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'deprecated-string-function' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'deprecated-str-translate-call' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'deprecated-itertools-function' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'deprecated-types-field' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'next-method-defined' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'dict-items-not-iterating' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'dict-keys-not-iterating' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'dict-values-not-iterating' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'deprecated-operator-function' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'deprecated-urllib-function' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'xreadlines-attribute' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'deprecated-sys-function' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'exception-escape' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'comprehension-escape' was removed from pylint, see pylint-dev/pylint#4942.
(R0022)
🪛 Ruff (0.14.1)
leverage/modules/auth.py
194-194: Do not catch blind exception: Exception
(BLE001)
195-195: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
271-271: Do not catch blind exception: Exception
(BLE001)
tests/test_modules/test_auth.py
341-341: Unused function argument: args
(ARG001)
341-341: Unused function argument: kwargs
(ARG001)
348-348: Unused function argument: args
(ARG001)
348-348: Unused function argument: kwargs
(ARG001)
374-374: Unused function argument: mock_open
(ARG001)
374-374: Unused function argument: mock_boto
(ARG001)
374-374: Unused function argument: mock_update_conf
(ARG001)
412-412: Unused function argument: mock_open
(ARG001)
412-412: Unused function argument: mock_boto
(ARG001)
450-450: Unused function argument: args
(ARG001)
450-450: Unused function argument: kwargs
(ARG001)
461-461: Unused function argument: mock_open
(ARG001)
461-461: Unused function argument: mock_boto
(ARG001)
486-486: Unused function argument: mock_open
(ARG001)
486-486: Unused function argument: mock_update_conf
(ARG001)
539-539: Unused function argument: mock_open
(ARG001)
568-568: Unused function argument: mock_open
(ARG001)
568-568: Unused function argument: mock_boto
(ARG001)
leverage/modules/aws.py
225-225: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
255-255: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (1)
leverage/modules/utils.py (1)
19-23: Solid fallback when caller_name is absent.Graceful recovery for --help and similar flows looks good.
borland667
force-pushed
the
feature/sso-auto-refresh-credentials
branch
from
81739de to
4ebbeb5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
leverage/modules/auth.py (1)
219-226: Reconsider counting skipped credentials as successes.Currently, skipped credentials (still valid) increment
success_count. This is semantically odd since no action was taken. Consider either:
- Introducing a separate
skipped_countfor clarity in the final summary- Documenting in the summary message that "successful" includes skipped credentials
This would provide clearer feedback to users about what actually happened.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
leverage/modules/auth.py(1 hunks)leverage/modules/aws.py(4 hunks)leverage/modules/utils.py(3 hunks)tests/test_modules/test_auth.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- leverage/modules/aws.py
🧰 Additional context used
🧬 Code graph analysis (3)
tests/test_modules/test_auth.py (4)
leverage/modules/auth.py (3)
get_layer_profile(17-46)SkipProfile(13-14)refresh_all_accounts_credentials(160-275)leverage/container.py (1)
get_sso_access_token(322-324)leverage/_utils.py (1)
ExitError(109-116)leverage/path.py (2)
host_aws_profiles_file(203-204)host_aws_credentials_file(207-208)
leverage/modules/utils.py (1)
tests/conftest.py (1)
context(29-34)
leverage/modules/auth.py (3)
leverage/path.py (2)
host_aws_profiles_file(203-204)host_aws_credentials_file(207-208)leverage/container.py (2)
sso_region_from_main_profile(327-333)get_sso_access_token(322-324)leverage/_utils.py (1)
ExitError(109-116)
🪛 Ruff (0.14.6)
tests/test_modules/test_auth.py
341-341: Unused function argument: args
(ARG001)
341-341: Unused function argument: kwargs
(ARG001)
348-348: Unused function argument: args
(ARG001)
348-348: Unused function argument: kwargs
(ARG001)
374-374: Unused function argument: mock_open
(ARG001)
374-374: Unused function argument: mock_boto
(ARG001)
374-374: Unused function argument: mock_update_conf
(ARG001)
412-412: Unused function argument: mock_open
(ARG001)
412-412: Unused function argument: mock_boto
(ARG001)
450-450: Unused function argument: args
(ARG001)
450-450: Unused function argument: kwargs
(ARG001)
461-461: Unused function argument: mock_open
(ARG001)
461-461: Unused function argument: mock_boto
(ARG001)
486-486: Unused function argument: mock_open
(ARG001)
486-486: Unused function argument: mock_update_conf
(ARG001)
539-539: Unused function argument: mock_open
(ARG001)
568-568: Unused function argument: mock_open
(ARG001)
568-568: Unused function argument: mock_boto
(ARG001)
leverage/modules/auth.py
194-194: Do not catch blind exception: Exception
(BLE001)
195-195: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
271-271: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (14)
leverage/modules/utils.py (3)
1-1: LGTM: Import added for signature inspection.The
inspectmodule import is necessary for the signature inspection logic used later in the file to determine if subcommand callbacks accept anargsparameter.
19-23: LGTM: Robust error handling for missing caller_name.The try-except wrapper appropriately handles cases where
caller_nameis not inargs(e.g., when using--help), defaulting to start from the beginning. This preventsValueErrorexceptions from breaking the command dispatch.
37-63: LGTM: Enhanced subcommand dispatch with proper Click option parsing.The expanded subcommand handling correctly:
- Resolves subcommand objects and calculates remaining args
- Distinguishes help requests for groups vs. leaf commands
- Uses signature inspection to determine the appropriate invocation path
- Creates new contexts when needed for proper Click option parsing
This enables the new CLI features while maintaining backward compatibility.
leverage/modules/auth.py (3)
160-186: LGTM: Well-structured bulk credential refresh function.The function correctly:
- Identifies SSO profiles matching the pattern
profile {project}-sso-*- Excludes the main SSO profile with the inequality check
- Provides early exit with helpful guidance when no profiles are found
- Logs clear progress information
The docstring and initial setup logic are clear and appropriate.
271-273: Generic exception handler is acceptable here for resilience.While static analysis flags the broad
Exceptioncatch, it's appropriate in this bulk operation context to continue processing remaining accounts even if one fails unexpectedly. The error is logged with the account name, providing sufficient debugging information.
238-246: LGTM: Permission error handling enables partial success.The permission error handling correctly:
- Catches specific
ClientErrorcodes (AccessDeniedException,ForbiddenException)- Logs a warning instead of raising an error (appropriate for bulk operations)
- Increments
error_countand continues processing other accountsThis allows the function to succeed partially when permissions vary across accounts.
tests/test_modules/test_auth.py (8)
12-17: LGTM: Import updated to include new function.The import correctly adds
refresh_all_accounts_credentialsto the test module's surface, enabling the comprehensive test suite added below.
310-407: LGTM: Comprehensive test for multi-account credential refresh.The test thoroughly validates:
- SSO profile identification and filtering
- Smart expiration checking (skipping valid credentials)
- Per-account progress logging
- Success/failure summary reporting
The test data setup with
FILE_AWS_CONFIG_MULTI_ACCOUNTSand side effects is well-structured. The docstring provides excellent context.
412-437: LGTM: Force refresh test validates bypass of expiration checks.The test correctly verifies that
force_refresh=Trueprocesses all accounts regardless of credential validity and skips no accounts. The docstring explains the use cases clearly.
461-480: LGTM: No-profiles test ensures graceful degradation.The test validates that the function handles missing SSO profiles appropriately with helpful guidance to run
leverage aws configure sso. This prevents confusing errors for new users.
486-535: LGTM: Permission error test validates partial success handling.The test effectively verifies:
- Continued processing when one account fails with permission errors
- Warning-level logging (not errors) for permission issues
- Accurate success/failure counts in the summary
The side_effect setup simulating mixed success/failure is well-crafted.
539-561: LGTM: Token error test validates fail-fast behavior.The test correctly verifies that SSO access token retrieval failures are wrapped in
ExitErrorwith clear messaging and that processing stops immediately without attempting to process any accounts.
568-597: LGTM: Update calls test validates correct file operations.The test thoroughly verifies:
- Correct number of
update_config_sectioncalls (2 per account: config + credentials)- Proper profile name formatting (with/without "profile " prefix)
- All expected accounts are processed
This ensures the function follows AWS CLI file format conventions correctly.
600-662: LGTM: Integration test validates end-to-end file handling.The integration test provides valuable coverage by:
- Using real file I/O with temporary files
- Verifying actual ConfigUpdater parsing behavior
- Testing with realistic AWS config file content
- Minimizing mocks to only external dependencies (boto3, time)
This complements the unit tests by exercising the actual file handling logic.
borland667
force-pushed
the
feature/sso-auto-refresh-credentials
branch
from
4ebbeb5 to
c87602c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
leverage/modules/auth.py (1)
297-300: Improve exception handling with specific exception types and chaining.As noted in a previous review, the generic
Exceptioncatch should be more specific, and the re-raise should use exception chaining to preserve the original traceback.# Note: get_sso_access_token() can raise various exceptions (FileNotFoundError, # json.JSONDecodeError, KeyError, OSError) so we catch Exception to handle all cases try: access_token = cli.get_sso_access_token() - except Exception as e: - raise ExitError(1, f"Failed to get SSO access token: {e}") + except (FileNotFoundError, KeyError, json.JSONDecodeError, OSError) as e: + raise ExitError(1, f"Failed to get SSO access token: {e}") from eNote: You'll need to add
import jsonat the top of the file.
🧹 Nitpick comments (3)
leverage/modules/utils.py (1)
51-63: Consider simplifying context invocation.The
context.scope(cleanup=False)wrapper on line 62 may be unnecessary. Typically, invoking within the new sub-context is sufficient:else: # Create a new context with remaining args so Click can parse options with subcommand_obj.make_context(subcommand, list(remaining_args), parent=context) as sub_ctx: - with context.scope(cleanup=False): - subcommand_obj.invoke(sub_ctx) + subcommand_obj.invoke(sub_ctx)If there's a specific reason for using
context.scope()(e.g., preserving parent context state), consider adding a brief comment explaining why.tests/test_modules/test_auth.py (1)
613-615: Remove unused imports.The
tempfileandosimports are not used in this test function since file creation uses thetmp_pathpytest fixture.- import tempfile - import os from pathlib import Pathleverage/modules/auth.py (1)
346-348: Add exception chaining in the loop error handler.While the generic
Exceptioncatch is acceptable here for resilience (to continue processing other accounts), add exception chaining for better debugging:except Exception as e: - logger.error(f"Failed to refresh credentials for {account_name}: {e}") + logger.exception(f"Failed to refresh credentials for {account_name}: {e}") error_count += 1Using
logger.exception()automatically includes the full traceback in the log output, which aids debugging without interrupting the loop.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
leverage/modules/auth.py(4 hunks)leverage/modules/aws.py(4 hunks)leverage/modules/utils.py(3 hunks)tests/test_modules/test_auth.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- leverage/modules/aws.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-17T16:40:11.185Z
Learnt from: angelofenoglio
Repo: binbashar/leverage PR: 288
File: leverage/modules/auth.py:129-134
Timestamp: 2024-10-17T16:40:11.185Z
Learning: In `leverage/modules/auth.py`, it's acceptable to raise exceptions within an `except` block without chaining the original exception using `from error`, even if the traceback is lost.
Applied to files:
leverage/modules/auth.py
🧬 Code graph analysis (2)
tests/test_modules/test_auth.py (4)
leverage/modules/auth.py (3)
refresh_layer_credentials(221-260)get_layer_profile(18-47)SkipProfile(14-15)leverage/container.py (1)
get_sso_access_token(322-324)leverage/_utils.py (1)
ExitError(109-116)leverage/path.py (2)
host_aws_profiles_file(203-204)host_aws_credentials_file(207-208)
leverage/modules/utils.py (1)
tests/conftest.py (1)
context(29-34)
🪛 Ruff (0.14.6)
leverage/modules/auth.py
164-164: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
299-299: Do not catch blind exception: Exception
(BLE001)
300-300: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
346-346: Do not catch blind exception: Exception
(BLE001)
tests/test_modules/test_auth.py
341-341: Unused function argument: args
(ARG001)
341-341: Unused function argument: kwargs
(ARG001)
348-348: Unused function argument: args
(ARG001)
348-348: Unused function argument: kwargs
(ARG001)
374-374: Unused function argument: mock_open
(ARG001)
374-374: Unused function argument: mock_boto
(ARG001)
374-374: Unused function argument: mock_update_conf
(ARG001)
412-412: Unused function argument: mock_open
(ARG001)
412-412: Unused function argument: mock_boto
(ARG001)
450-450: Unused function argument: args
(ARG001)
450-450: Unused function argument: kwargs
(ARG001)
461-461: Unused function argument: mock_open
(ARG001)
461-461: Unused function argument: mock_boto
(ARG001)
486-486: Unused function argument: mock_open
(ARG001)
486-486: Unused function argument: mock_update_conf
(ARG001)
539-539: Unused function argument: mock_open
(ARG001)
568-568: Unused function argument: mock_open
(ARG001)
568-568: Unused function argument: mock_boto
(ARG001)
🔇 Additional comments (18)
leverage/modules/utils.py (3)
1-2: LGTM!The
inspectimport is appropriately added to support the signature-based parameter detection used later in the file.
19-23: LGTM!Defensive handling for when
caller_nameis not found in args (e.g.,--helpscenarios). Defaulting to position 0 is a sensible fallback.
37-50: LGTM!Good enhancement to handle help requests properly for both groups and leaf commands. The logic correctly differentiates between showing help when
--helpis the only remaining arg vs. forwarding to nested commands within a group.tests/test_modules/test_auth.py (9)
12-17: LGTM!Import updated correctly to include the new
refresh_all_accounts_credentialsfunction from the public API.
310-366: Well-structured test data.Good test setup with multi-account configuration covering different credential states: one with expired credentials (security), one with valid credentials (shared), and one with no expiration set (network). The mock client is properly configured for credential responses.
369-406: LGTM!Comprehensive test covering the success path with proper verification of:
- Account discovery logging
- Credential refresh for expired accounts
- Skip behavior for valid credentials
- Summary reporting
The unused mock parameters are necessary for the decorator stack to work correctly.
409-436: LGTM!Good test verifying that
force_refresh=Truebypasses the expiration check and processes all accounts.
439-479: LGTM!Good edge case test for the scenario where no SSO account profiles are configured.
482-534: LGTM!Excellent test covering the resilience behavior where permission errors on individual accounts don't halt processing of remaining accounts. The side_effect list pattern effectively simulates mixed success/failure scenarios.
537-560: LGTM!Good test verifying fail-fast behavior when SSO access token retrieval fails.
563-596: LGTM!Good test verifying the correct number of update calls and proper profile naming conventions for both config and credentials files.
599-661: Good integration test.This test provides valuable coverage with real file I/O operations, validating the actual parsing and file handling logic with minimal mocking. Good use of the
tmp_pathfixture for isolated test files.leverage/modules/auth.py (6)
4-4: LGTM!Appropriate addition of
Optionaltype hint import.
61-92: LGTM!Well-documented helper function with clear semantics. The 30-minute buffer for renewal is a sensible default to avoid credential expiration during long-running operations.
95-109: LGTM!Clean helper function for lazy initialization of the credentials updater. Good use of
touch(exist_ok=True)for idempotent file creation.
112-195: Well-structured credential retrieval helper.Good separation of concerns with configurable error handling behavior via
raise_on_permission_error. The conditional token retrieval (line 150) allows the function to work in both single-profile and bulk-refresh contexts.
221-260: LGTM!Good refactoring of
refresh_layer_credentialsto use the new helper functions. The lazy initialization ofcredentials_updater(lines 245-246) avoids unnecessary file operations when all credentials are still valid.
263-350: Good implementation of bulk credential refresh.The function handles the multi-account refresh workflow well with:
- Proper profile discovery using pattern matching
- Smart expiration checking with force override
- Resilient error handling that continues processing other accounts
- Clear progress logging and final summary
One minor observation: the
account_namevariable may be undefined if an exception occurs before line 311, causing the error message on line 347 to fail. Consider adding a fallback:for profile_section in sso_profiles: + account_name = profile_section # Fallback for error logging try: # Extract account name from profile (e.g., "profile project-sso-security" -> "security") account_name = profile_section.replace(f"profile {cli.project}-sso-", "")Likely an incorrect or invalid review comment.
borland667
force-pushed
the
feature/sso-auto-refresh-credentials
branch
3 times, most recently
from
9e578b1 to
9a7a71d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
leverage/modules/auth.py(4 hunks)leverage/modules/aws.py(4 hunks)leverage/modules/utils.py(3 hunks)tests/test_modules/test_auth.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- leverage/modules/aws.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-17T16:40:11.185Z
Learnt from: angelofenoglio
Repo: binbashar/leverage PR: 288
File: leverage/modules/auth.py:129-134
Timestamp: 2024-10-17T16:40:11.185Z
Learning: In `leverage/modules/auth.py`, it's acceptable to raise exceptions within an `except` block without chaining the original exception using `from error`, even if the traceback is lost.
Applied to files:
leverage/modules/auth.py
🧬 Code graph analysis (3)
leverage/modules/utils.py (1)
tests/conftest.py (1)
context(29-34)
leverage/modules/auth.py (3)
leverage/path.py (2)
host_aws_credentials_file(207-208)host_aws_profiles_file(203-204)leverage/container.py (2)
get_sso_access_token(322-324)sso_region_from_main_profile(327-333)leverage/_utils.py (1)
ExitError(109-116)
tests/test_modules/test_auth.py (4)
leverage/modules/auth.py (1)
refresh_all_accounts_credentials(264-369)leverage/container.py (1)
get_sso_access_token(322-324)leverage/_utils.py (1)
ExitError(109-116)leverage/path.py (2)
host_aws_profiles_file(203-204)host_aws_credentials_file(207-208)
🪛 Ruff (0.14.6)
leverage/modules/auth.py
365-365: Do not catch blind exception: Exception
(BLE001)
tests/test_modules/test_auth.py
341-341: Unused function argument: args
(ARG001)
341-341: Unused function argument: kwargs
(ARG001)
348-348: Unused function argument: args
(ARG001)
348-348: Unused function argument: kwargs
(ARG001)
374-374: Unused function argument: mock_open
(ARG001)
374-374: Unused function argument: mock_boto
(ARG001)
374-374: Unused function argument: mock_update_conf
(ARG001)
412-412: Unused function argument: mock_open
(ARG001)
412-412: Unused function argument: mock_boto
(ARG001)
450-450: Unused function argument: args
(ARG001)
450-450: Unused function argument: kwargs
(ARG001)
461-461: Unused function argument: mock_open
(ARG001)
461-461: Unused function argument: mock_boto
(ARG001)
486-486: Unused function argument: mock_open
(ARG001)
486-486: Unused function argument: mock_update_conf
(ARG001)
539-539: Unused function argument: mock_open
(ARG001)
568-568: Unused function argument: mock_open
(ARG001)
568-568: Unused function argument: mock_boto
(ARG001)
🔇 Additional comments (17)
tests/test_modules/test_auth.py (9)
12-17: LGTM!The new import for
refresh_all_accounts_credentialsis correctly added alongside existing imports fromleverage.modules.auth.
310-366: Well-structured test data for multi-account scenarios.The test configuration defines a realistic multi-account setup with varied credential states (security: needs refresh, shared: valid, network: needs refresh). The mock client and file I/O side effects are correctly implemented.
Note: The static analysis warnings about unused
args/kwargsparameters are false positives—these are required for signature compatibility with the functions being mocked.
369-406: LGTM!Comprehensive test for the success scenario with appropriate assertions for logging messages, smart skip behavior for valid credentials, and summary output.
409-436: LGTM!Good test coverage for the
force_refresh=Truepath, verifying that expiration checks are bypassed and all accounts are processed.
458-479: LGTM!Good edge case coverage for the scenario where no SSO account profiles are configured.
482-534: LGTM!Excellent test for partial failure handling. The side_effect list simulating success/error/success properly validates that the function continues processing after permission errors and reports accurate success/failure counts.
537-560: LGTM!Good test coverage for fail-fast behavior when SSO access token retrieval fails.
563-596: LGTM!Good verification that the function correctly updates both config and credentials files with proper profile name formatting.
599-659: Good integration test with real file I/O.This test provides valuable end-to-end verification with minimal mocking. The setup correctly creates a scenario where one account has valid credentials (security with expiration > renewal time) and one needs refresh (shared with no expiration).
Minor: The comment on line 649 could be clearer—"2 calls" corresponds to 1 account being refreshed × 2 files (config + credentials).
leverage/modules/utils.py (2)
19-23: LGTM!Good defensive fix for handling cases where
caller_nameis not inargs(e.g., when using--help). Defaulting tocaller_pos = 0is a safe fallback.
43-50: LGTM on help detection logic.The logic correctly distinguishes between leaf commands (show help anytime requested) and groups (only show help when
--helpis the sole remaining arg, allowing subcommands to be invoked). Supportinghelpas a positional arg enables the documentedleverage aws sso refresh helpsyntax.leverage/modules/auth.py (6)
62-93: LGTM!Well-designed helper with clear return value semantics. The 30-minute renewal window provides a good safety buffer for long-running tasks.
96-110: LGTM!Simple and focused helper for lazy credentials file initialization.
113-196: LGTM!Well-designed helper that consolidates credential retrieval and update logic. The
raise_on_permission_errorflag enables flexible error handling for different use cases (fail-fast for single-layer vs. continue-on-error for bulk refresh). Exception chaining withfrom erroris properly implemented.
222-261: LGTM!Clean refactoring that maintains existing behavior while reusing the new helper functions. Lazy initialization of
credentials_updateris a nice optimization.
295-319: Comprehensive exception handling with good documentation.The exception list is well-documented in the comments. Note that
PermissionErrorandIsADirectoryErrorare subclasses ofOSError, so they're redundant in the tuple, but listing them explicitly improves code readability and clarity of intent.
327-368: PotentialUnboundLocalErrorif exception occurs beforeaccount_nameis assigned.If an exception occurs at line 333 or 334 (e.g.,
NoOptionError),account_namefrom line 330 may not be defined yet when the exception handler on line 366 tries to use it.for profile_section in sso_profiles: + account_name = None # Initialize to avoid UnboundLocalError try: # Extract account name from profile (e.g., "profile project-sso-security" -> "security") account_name = profile_section.replace(f"profile {cli.project}-sso-", "") # ... rest of the try block except Exception as e: - logger.exception(f"Failed to refresh credentials for {account_name}: {e}") + logger.exception(f"Failed to refresh credentials for {account_name or profile_section}: {e}") error_count += 1Note: The bare
Exceptioncatch flagged by Ruff is acceptable here for resilience—logger.exceptioncaptures the full traceback, and the function is designed to continue processing other accounts.Likely an incorrect or invalid review comment.
Feature: bulk refresh AWS SSO credentials for all configured accounts without running Terraform. Optional --refresh-all on 'leverage aws sso login' and new 'leverage aws sso refresh' [--force]. Skips accounts with valid credentials (>30 min left); continues on permission errors and reports refreshed/skipped/failed counts. - auth: refresh_all_accounts_credentials(); helpers _get_sso_profile_ from_section, _get_sso_profile_sections, _get_sso_token_or_raise, _refresh_one_profile; get_layer_profile reuses section parsing. - aws: --refresh-all on login; sso refresh command. - utils: fix exit code (raise on non-zero); help via make_context; guard callback None; invoke with args/is_group. - _utils: ExitError.exit_code for Pylint. - tests: refresh_all success/force/no-profiles/permission/token/update/ integration; ARG001 and summary assertions.
borland667
force-pushed
the
feature/sso-auto-refresh-credentials
branch
from
9a7a71d to
dd80d59
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
leverage/modules/auth.py (1)
338-340: Consider narrowing the exception catch for better error visibility.The broad
except Exceptioncatch ensures one profile failure doesn't halt the entire refresh operation, which is good for user experience. However, it may also mask programming errors (e.g.,AttributeError,NameError).Consider catching only expected exceptions like
ClientError,NoSectionError, andNoOptionError, and letting unexpected exceptions propagate. Alternatively, keep the broad catch but ensure thorough test coverage.♻️ Optional: narrow exception handling
- except Exception as e: - logger.exception(f"Failed to refresh credentials for {profile_section}: {e}") - return "error" + except (ClientError, NoSectionError, NoOptionError) as e: + logger.warning(f"Failed to refresh credentials for {profile_section}: {e}") + return "error"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leverage/modules/auth.py` around lines 338 - 340, Currently the except block catches all Exceptions in the refresh routine (logger.exception call and return "error"), which can hide programming errors; change it to catch only expected errors (e.g., botocore.exceptions.ClientError and configparser.NoSectionError/NoOptionError) around the code that can raise them, log the error with logger.exception(f"Failed to refresh credentials for {profile_section}: {e}") and return "error" for those cases, and let other unexpected exceptions propagate (i.e., remove the broad "except Exception" or re-raise after logging) so that bugs like AttributeError/NameError are not silently swallowed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@leverage/modules/auth.py`:
- Around line 338-340: Currently the except block catches all Exceptions in the
refresh routine (logger.exception call and return "error"), which can hide
programming errors; change it to catch only expected errors (e.g.,
botocore.exceptions.ClientError and configparser.NoSectionError/NoOptionError)
around the code that can raise them, log the error with
logger.exception(f"Failed to refresh credentials for {profile_section}: {e}")
and return "error" for those cases, and let other unexpected exceptions
propagate (i.e., remove the broad "except Exception" or re-raise after logging)
so that bugs like AttributeError/NameError are not silently swallowed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bd41dd54-b1fc-4cfd-b1d3-a65d94451f59
📒 Files selected for processing (5)
leverage/_utils.pyleverage/modules/auth.pyleverage/modules/aws.pyleverage/modules/utils.pytests/test_modules/test_auth.py
🚧 Files skipped from review as they are similar to previous changes (1)
- leverage/modules/aws.py
1 participant
Add Automatic Credential Refresh for AWS SSO
What?
New Features
--refresh-allflag onleverage aws sso loginto refresh all account credentials after SSO loginleverage aws sso refresh [--force]command--forcespecifiedInfrastructure Fixes
_handle_subcommand()for nested commands--helpandhelpsyntaxes for all wrapped Python commandsinspect.signature()Tests
Why?
Currently, after SSO login, you need to manually refresh credentials for each AWS account. Often this means running
terraform planorterraform outputjust to trigger credential refresh - even when you only want to generate a kubeconfig for EKS or run a simple AWS CLI command. This is time-consuming and annoying.This PR streamlines the workflow with automatic or one-command refresh that skips accounts with valid credentials.
Benefits: Time savings, better UX, no unnecessary Terraform operations, graceful error handling, backward compatible (opt-in)
Usage
Testing
Before Release
Review the checklist here
Summary by CodeRabbit
New Features
--refresh-allflag on SSO login and a newsso refreshcommand--forceoption to override expiration checks and force refreshImprovements
Tests