Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix | Catch error for not found/forbidden role #288

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Bugfix | Catch error for not found/forbidden role #288

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9b08ea8
Catch `ForbiddenException`
angelofenoglio Oct 17, 2024
4e37e8f
Add tests
angelofenoglio Oct 17, 2024
0b48d5f
Add `AccessDeniedException` as possible error code
angelofenoglio Oct 17, 2024
134ffa1
Fix test
angelofenoglio Oct 17, 2024
8b6144e
Reorder test parameters to match patches
angelofenoglio Oct 17, 2024
df74769
Format
angelofenoglio Oct 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 19 additions & 9 deletions leverage/modules/auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import time
from configparser import NoSectionError, NoOptionError
from pathlib import Path
from configparser import NoSectionError, NoOptionError

import boto3
import hcl2
import boto3
from configupdater import ConfigUpdater
from botocore.exceptions import ClientError

from leverage import logger
from leverage._utils import key_finder, ExitError, get_or_create_section
Expand Down Expand Up @@ -40,7 +41,7 @@ def get_layer_profile(raw_profile: str, config_updater: ConfigUpdater, tf_profil
except NoSectionError:
raise ExitError(40, f"Missing {sso_profile} permission for account {account_name}.")

# if we are processing a profile from a different layer, we need to built it
# if we are processing a profile from a different layer, we need to build it
layer_profile = layer_profile or f"{project}-{account_name}-{sso_role.lower()}"

return account_id, account_name, sso_role, layer_profile
Expand Down Expand Up @@ -104,7 +105,7 @@ def refresh_layer_credentials(cli):
expiration = int(config_updater.get(f"profile {layer_profile}", "expiration").value) / 1000
except (NoSectionError, NoOptionError):
# first time using this profile, skip into the credential's retrieval step
logger.debug(f"No cached credentials found.")
logger.debug("No cached credentials found.")
else:
# we reduce the validity 30 minutes, to avoid expiration over long-standing tasks
renewal = time.time() + (30 * 60)
Expand All @@ -117,11 +118,20 @@ def refresh_layer_credentials(cli):

# retrieve credentials
logger.debug(f"Retrieving role credentials for {sso_role}...")
credentials = client.get_role_credentials(
roleName=sso_role,
accountId=account_id,
accessToken=cli.get_sso_access_token(),
)["roleCredentials"]
try:
credentials = client.get_role_credentials(
roleName=sso_role,
accountId=account_id,
accessToken=cli.get_sso_access_token(),
)["roleCredentials"]
except ClientError as error:
if error.response["Error"]["Code"] in ("AccessDeniedException", "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].",
)
Comment on lines +129 to +134
Copy link

@coderabbitai coderabbitai bot Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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 except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai In this case I don't care if the traceback is lost.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.


# update expiration on aws/<project>/config
logger.info(f"Writing {layer_profile} profile")
Expand Down
24 changes: 23 additions & 1 deletion tests/test_modules/test_auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
from unittest.mock import Mock, MagicMock, PropertyMock

import pytest
from botocore.exceptions import ClientError
from configupdater import ConfigUpdater

from leverage._utils import ExitError
Expand Down Expand Up @@ -249,8 +250,8 @@ def test_refresh_layer_credentials_still_valid(mock_open, mock_boto, sso_contain

@mock.patch("leverage.modules.auth.update_config_section")
@mock.patch("builtins.open", side_effect=open_side_effect)
@mock.patch("boto3.client", return_value=b3_client)
@mock.patch("time.time", new=Mock(return_value=1705859000))
@mock.patch("boto3.client", return_value=b3_client)
@mock.patch("pathlib.Path.touch", new=Mock())
def test_refresh_layer_credentials(mock_boto, mock_open, mock_update_conf, sso_container, propagate_logs):
refresh_layer_credentials(sso_container)
Expand All @@ -265,3 +266,24 @@ def test_refresh_layer_credentials(mock_boto, mock_open, mock_update_conf, sso_c
"aws_secret_access_key": "secret-key",
"aws_session_token": "session-token",
}


@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_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):
refresh_layer_credentials(sso_container)
Loading