Bugfix | Catch error for not found/forbidden role #288
Conversation
It happens when attempting to assume a role that does not exists or the user doesn't have permission to assume it
WalkthroughThe changes in this pull request enhance error handling and logging in the AWS SSO credential management process within the Changes
Assessment against linked issues
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Pull Request Test Coverage Report for Build 11391671478Details
💛 - Coveralls |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- leverage/modules/auth.py (4 hunks)
- tests/test_modules/test_auth.py (2 hunks)
🧰 Additional context used
🪛 Ruff
leverage/modules/auth.py
129-134: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
🔇 Additional comments (1)
tests/test_modules/test_auth.py (1)
277-284: Test correctly validates access denial behaviorThe new test function
test_refresh_layer_credentials_no_accessappropriately checks that anExitErroris raised when access is denied during credential refresh. This ensures that the application handles permission issues gracefully and improves test coverage for error scenarios.
| raise ExitError( | ||
| 40, | ||
| f"User does not have permission to assume role [bold]{sso_role}[/bold]" | ||
| " in this account.\nPlease check with your administrator or try" | ||
| " running [bold]leverage aws configure sso[/bold].", | ||
| ) |
There was a problem hiding this comment.
Chain exceptions when re-raising in an except block
When raising an exception within an except clause, it's recommended to chain it using from error to preserve the original traceback. This helps in debugging by indicating the original cause of the exception.
Apply this diff to chain the exception:
except ClientError as error:
if error.response["Error"]["Code"] == "ForbiddenException":
raise ExitError(
40,
f"User does not have permission to assume role [bold]{sso_role}[/bold]"
" in this account.\nPlease check with your administrator or try"
" running [bold]leverage aws configure sso[/bold].",
+ ) from error
- )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| raise ExitError( | |
| 40, | |
| f"User does not have permission to assume role [bold]{sso_role}[/bold]" | |
| " in this account.\nPlease check with your administrator or try" | |
| " running [bold]leverage aws configure sso[/bold].", | |
| ) | |
| raise ExitError( | |
| 40, | |
| f"User does not have permission to assume role [bold]{sso_role}[/bold]" | |
| " in this account.\nPlease check with your administrator or try" | |
| " running [bold]leverage aws configure sso[/bold].", | |
| ) from error |
🧰 Tools
🪛 Ruff
129-134: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
There was a problem hiding this comment.
@coderabbitai In this case I don't care if the traceback is lost.
There was a problem hiding this comment.
@angelofenoglio Understood, thank you for letting me know.
(_/)
( ;_;)
(/ )
✏️ Learnings added
Learnt from: angelofenoglio
PR: binbashar/leverage#288
File: leverage/modules/auth.py:129-134
Timestamp: 2024-10-17T16:40:11.055Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
tests/test_modules/test_auth.py
Outdated
| b3_client_error.get_role_credentials.side_effect = ClientError( | ||
| {"Error": {"Code": "ForbiddenException", "Message": "No access"}}, "GetRoleCredentials" | ||
| ) |
There was a problem hiding this comment.
Use the correct exception code in ClientError
The error code 'ForbiddenException' used in the ClientError may not be accurate. According to AWS SDK for Python (Boto3) documentation, when access is denied, the error code is typically 'AccessDeniedException'. Using the correct error code ensures that the test accurately simulates the real-world scenario and that exception handling behaves as expected.
Apply this diff to correct the exception code:
- {"Error": {"Code": "ForbiddenException", "Message": "No access"}}, "GetRoleCredentials"
+ {"Error": {"Code": "AccessDeniedException", "Message": "No access"}}, "GetRoleCredentials"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| b3_client_error.get_role_credentials.side_effect = ClientError( | |
| {"Error": {"Code": "ForbiddenException", "Message": "No access"}}, "GetRoleCredentials" | |
| ) | |
| b3_client_error.get_role_credentials.side_effect = ClientError( | |
| {"Error": {"Code": "AccessDeniedException", "Message": "No access"}}, "GetRoleCredentials" | |
| ) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
tests/test_modules/test_auth.py (1)
288-289: Enhance Exception Assertion by Verifying the Error MessageWhen asserting that an
ExitErroris raised, consider using thematchparameter inpytest.raisesto verify the exception message. This ensures that the test not only checks for the exception type but also validates that the error message is as expected, improving test robustness.Apply this diff to include the
matchparameter:- with pytest.raises(ExitError): + with pytest.raises(ExitError, match="Expected error message"):Replace
"Expected error message"with the actual message you expect from theExitError.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- leverage/modules/auth.py (4 hunks)
- tests/test_modules/test_auth.py (3 hunks)
🧰 Additional context used
🪛 Ruff
leverage/modules/auth.py
129-134: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
🔇 Additional comments (6)
leverage/modules/auth.py (5)
3-3: LGTM: Import statements are well-organized and necessary.The added imports for
NoSectionError,NoOptionError,boto3, andClientErrorare logically organized and essential for the new error handling logic implemented in this file.Also applies to: 6-6, 8-8
44-44: LGTM: Minor comment improvement.The change from "built" to "build" in the comment is a small but welcome improvement in grammar and clarity.
108-108: LGTM: Improved debug log message.The updated debug log message "No cached credentials found." is more specific and informative, enhancing the clarity of the logging output.
121-134: LGTM: Improved error handling for credential retrieval.The new try-except block effectively catches
ClientErrorexceptions and provides a clear, actionable error message to the user when they lack permission to assume a role. This aligns well with the PR objectives.Note: The decision not to chain exceptions (i.e., not using
raise ... from error) has been acknowledged in a previous review. While this means the original traceback is lost, it has been deemed acceptable for this specific use case.🧰 Tools
🪛 Ruff
129-134: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
Line range hint
1-164: Summary: Effective implementation of error handling and logging improvements.The changes in this file successfully address the PR objectives by:
- Implementing proper error handling for cases where roles are not found or forbidden.
- Providing clear, user-friendly error messages with actionable instructions.
- Enhancing logging for improved debugging capabilities.
These modifications improve the robustness of the AWS SSO credential management process without altering the core functionality. The changes are well-implemented and should significantly enhance the user experience when dealing with role-related issues.
🧰 Tools
🪛 Ruff
129-134: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
tests/test_modules/test_auth.py (1)
6-6: ImportingClientErroris AppropriateThe addition of
ClientErrorimport is necessary for handling AWS exceptions in your tests.
tests/test_modules/test_auth.py
Outdated
| @mock.patch("leverage.modules.auth.update_config_section") | ||
| @mock.patch("builtins.open", side_effect=open_side_effect) | ||
| @mock.patch("time.time", new=Mock(return_value=1705859000)) | ||
| @mock.patch("pathlib.Path.touch", new=Mock()) | ||
| @pytest.mark.parametrize( | ||
| "error", | ||
| [ | ||
| ClientError({"Error": {"Code": "AccessDeniedException", "Message": "No access"}}, "GetRoleCredentials"), | ||
| ClientError({"Error": {"Code": "ForbiddenException", "Message": "No access"}}, "GetRoleCredentials"), | ||
| ] | ||
| ) | ||
| def test_refresh_layer_credentials_no_access(mock_open, mock_update_conf, sso_container, error): |
There was a problem hiding this comment.
Correct the Order of Mock Parameters in the Test Function
The order of parameters in your test function test_refresh_layer_credentials_no_access does not match the order of your @mock.patch decorators.
The @mock.patch decorators are applied from the bottom up, and the mocks are passed into your test function in the reverse order of how the decorators are stacked.
Given your decorators:
@mock.patch("leverage.modules.auth.update_config_section")
@mock.patch("builtins.open", side_effect=open_side_effect)
@mock.patch("time.time", new=Mock(return_value=1705859000))
@mock.patch("pathlib.Path.touch", new=Mock())
@pytest.mark.parametrize("error", [...])
def test_refresh_layer_credentials_no_access(mock_update_conf, mock_open, sso_container, error):Note that since pathlib.Path.touch and time.time are being patched with new=Mock(), they do not contribute additional parameters to the test function. Therefore, the mock parameters should be:
mock_update_conf(from@mock.patch("leverage.modules.auth.update_config_section"))mock_open(from@mock.patch("builtins.open", ...))sso_container(fixture)error(from@pytest.mark.parametrize)
Currently, your function definition has the parameters in the wrong order:
def test_refresh_layer_credentials_no_access(mock_open, mock_update_conf, sso_container, error):This may lead to incorrect application of mocks.
Apply this diff to correct the parameter order:
- def test_refresh_layer_credentials_no_access(mock_open, mock_update_conf, sso_container, error):
+ def test_refresh_layer_credentials_no_access(mock_update_conf, mock_open, sso_container, error):There was a problem hiding this comment.
@coderabbitai fair enough, I'll apply this patch!
There was a problem hiding this comment.
@angelofenoglio Thank you for making the change!
(\ (\
( ^^ )
((")(")
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_modules/test_auth.py (1)
271-289: Consider enhancing error scenario coverageThe test effectively covers access denial scenarios, but could be strengthened by:
- Verifying the error message content matches the expected user-friendly message
- Asserting that no credential updates occur when access is denied
Consider extending the test with these assertions:
def test_refresh_layer_credentials_no_access(mock_update_conf, mock_open, sso_container, error): with mock.patch("boto3.client") as mocked_client: mocked_client_obj = MagicMock() mocked_client_obj.get_role_credentials.side_effect = error mocked_client.return_value = mocked_client_obj - with pytest.raises(ExitError): + with pytest.raises(ExitError, match=f"Access denied.*{error.response['Error']['Code']}"): refresh_layer_credentials(sso_container) + + # Verify no credentials were saved + mock_update_conf.assert_not_called()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_modules/test_auth.py(3 hunks)
🔇 Additional comments (2)
tests/test_modules/test_auth.py (2)
6-6: LGTM: Import added for AWS error handling
The addition of ClientError import is appropriate for testing AWS SSO error scenarios.
Line range hint 254-269: LGTM: Mock parameter order corrected
The changes properly implement the previously suggested fix for mock parameter ordering, ensuring correct mock application during test execution.
What?
References
Summary by CodeRabbit
New Features
Bug Fixes
Tests