Add automatic credential refresh for AWS SSO #315
Conversation
WalkthroughAdds a bulk AWS SSO credential refresh feature with helpers for expiration and updater management, CLI hooks (login flag and new sso refresh subcommand), refined subcommand invocation handling, and comprehensive tests covering multi-account refresh and error scenarios. 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: sso login --refresh-all / sso refresh --force
CLI->>Auth: refresh_all_accounts_credentials(force_refresh)
Auth->>Auth: discover profiles matching "<project>-sso-*"
loop per matched profile
Auth->>Auth: _check_credentials_expiration(layer_profile, force_refresh)
alt needs refresh
Auth->>SSO: obtain SSO access token (if needed)
Auth->>SSO: get role credentials using access token
alt success
Auth->>FS: update config with new expiration
Auth->>FS: write AWS credentials to credentials file
Auth->>CLI: log success for profile
else permission/token error
Auth->>CLI: log error (raise or skip based on raise_on_permission_error)
end
else skip
Auth->>CLI: log skipped (not expired)
end
end
Auth->>CLI: return summary (successes/failures)
CLI->>User: display results / exit code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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
2 times, most recently
from
14ea1ec to
9e578b1
Compare
Add refresh_all_accounts_credentials() function to refresh credentials for all configured AWS accounts automatically. This feature provides both automatic refresh on SSO login and manual refresh capabilities. New Features: - Add --refresh-all flag to 'leverage aws sso login' command to automatically refresh all account credentials after login - Add 'leverage aws sso refresh [--force]' standalone command for manual credential refresh - Implement smart credential renewal that skips accounts with valid credentials unless --force is specified - Add comprehensive progress logging for each account operation - Handle permission errors gracefully and continue with other accounts Infrastructure Improvements: - Fix _handle_subcommand to properly handle Click options in nested command groups using make_context() for proper option parsing - Add explicit help handling for both --help and help syntaxes - Fix args propagation to nested command groups using inspect.signature - Fix ValueError when caller_name not in args tuple - Improve error handling with proper ExitError usage Testing: - Add 8 comprehensive unit tests covering success, force refresh, no profiles, permission errors, and token errors scenarios - Add integration test with real file operations using tmp_path - Enhance all test docstrings with clear intent, scenarios, and technical details - Minimize mocking to only external dependencies (boto3, time) - All 146 tests passing with zero regressions Production tested with 5 AWS accounts - all scenarios working correctly. Maintains full backward compatibility with existing functionality.
borland667
force-pushed
the
feature/sso-auto-refresh-credentials
branch
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.
| # Invoke the subcommand normally | ||
| # For groups and commands with 'args' parameter, pass remaining args | ||
| # Otherwise, create a new context to properly parse Click options | ||
| 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: | ||
| subcommand_obj.invoke(sub_ctx) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's look at the file structure and find the specific file
fd "utils.py" --type f | grep -E "(leverage|modules)"
# Also check if the file exists at the expected location
if [ -f "leverage/modules/utils.py" ]; then
echo "File found. Checking line count and relevant section..."
wc -l leverage/modules/utils.py
fiRepository: binbashar/leverage
Length of output: 180
🏁 Script executed:
#!/bin/bash
# Read the relevant section of the file with more context
if [ -f "leverage/modules/utils.py" ]; then
# Show lines 35-70 to get full context of the function
sed -n '35,70p' leverage/modules/utils.py | cat -n
fiRepository: binbashar/leverage
Length of output: 180
🏁 Script executed:
#!/bin/bash
# Search for Click command/group definitions to understand callback patterns
rg -n "click\.(command|group|Command|Group)" leverage/modules/utils.py -A 3 -B 1 | head -50Repository: binbashar/leverage
Length of output: 180
🏁 Script executed:
#!/bin/bash
# Check for any None callback assignments or patterns in the codebase
rg -n "callback\s*=\s*None" --type py -C 2 | head -40Repository: binbashar/leverage
Length of output: 180
🌐 Web query:
Click Python library callback None behavior groups commands
💡 Result:
Short answer:
- A Command’s callback may be None — that just means the command does nothing beyond Click’s built-in handling (help, parameter parsing, etc.). [1][2]
- For Group/MultiCommand, the group’s callback is invoked when a subcommand runs (unless you change that behavior with options like invoke_without_command). The group’s return value is normally the subcommand’s return value; if invoke_without_command is set and the group is called without a subcommand, the group callback’s return value is used. Chained groups return a list of subcommand results and you can process results with a result_callback. [1][2]
References:
[1] Click — Commands and Groups (callback invocation, return-value behavior).
[2] Click API (Command.callback may be None).
Handle the case where subcommand_obj.callback is None to prevent TypeError.
Click explicitly allows commands and groups to have None callbacks—this is normal behavior for commands that only perform Click's built-in handling (help, parameter parsing). Calling inspect.signature(subcommand_obj.callback) without a None check will raise TypeError when a subcommand has no callback.
else:
# Invoke the subcommand normally
# For groups and commands with 'args' parameter, pass remaining args
# Otherwise, create a new context to properly parse Click options
- sig = inspect.signature(subcommand_obj.callback)
- if "args" in sig.parameters:
+ callback = subcommand_obj.callback
+ if callback is not None and "args" in inspect.signature(callback).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:
subcommand_obj.invoke(sub_ctx)🤖 Prompt for AI Agents
In leverage/modules/utils.py around lines 52 to 62, the code calls
inspect.signature(subcommand_obj.callback) without handling the case where
subcommand_obj.callback is None, which raises TypeError for commands/groups with
no callback; fix by first checking if subcommand_obj.callback is None and, if
so, skip using inspect.signature and directly create a context with
remaining_args and invoke the subcommand (using make_context + invoke) so Click
can handle parsing/dispatching; otherwise continue to inspect the signature and
follow the existing args-vs-new-context branch.
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
✏️ Tip: You can customize this high-level summary in your review settings.