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.

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
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
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

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
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_open, mock_update_conf, sso_container, error):
Copy link

@coderabbitai coderabbitai bot Oct 17, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

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):

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 fair enough, I'll apply this patch!

Copy link

Choose a reason for hiding this comment

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

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

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