Allow setting session policy of assumed role #21
Conversation
| ("RoleSessionName", self.role_session_name), | ||
| ("Policy", self.policy), | ||
| ] | ||
| if v is not None |
There was a problem hiding this comment.
This line has no test coverage.
I accidentally missed it out when rewriting my PR.
All the tests pass if it is removed, but without it there's a pretty obvious problem. When no session policy is passed, all we get is a ParamValidationError.
I know the tests mock away most of the boto internals. Could this be a side effect of that?
Any ideas for how we might properly test for it?
I think it would be helpful during testing to allow boto to validate the parameters to assume_role while skipping the remote API call. Does mocking offer this level of control?
Anyway, I realized what was missing when I ran this in a Python console against my real sandbox management account:
botocove.cove(
lambda s: s.client("iam").list_account_aliases()["AccountAliases"],
assuming_session=boto3.Session(profile_name="sandbox-mgmt")
)()
There was a problem hiding this comment.
Interestingly I'd expect a Python function to allow you to pass it's default arg without complaint so that's a bit of a quirk of boto3?
Interesting problem to try and solve but probably not worth worrying about - all I can think of for testing the third party package behaviour is asserting we get the right kind of exception but I'm not super keen on writing that many tests for every boto3 call!
This solution does break the type system but cleaner than writing some sort of switch statement so we can just squash it for this line :)
README.md
Outdated
|
|
||
| A policy document that will be used as a session policy in each Cove session's `sts.assume_role()` call. Unless the value is None, it is passed through via the Policy parameter. | ||
|
|
||
| `policy_arns`: Optional[List[PolicyArn]] |
There was a problem hiding this comment.
PolicyArn may not make much sense here. Should I clarify in the documentation? Or should I replace PolicyArn everywhere with List[Dict[str, str]]?
I wanted to enforce the arn key in the type system, but maybe it's not worth it if it makes the documentation difficult.
There was a problem hiding this comment.
I revered it back to a str just for clarity, boto3 will validate this at runtime and blow up if it's wrong anyway which I guess is the benefit of our previous challenge :)
| @@ -160,5 +160,40 @@ def simple_func(session: CoveSession) -> Optional[str]: | |||
| return session.session_information.RoleSessionName | |||
|
|
|||
| cove_output = simple_func() | |||
| print(cove_output) | |||
|
|
|||
| assert cove_output["Exceptions"] == [] | |||
There was a problem hiding this comment.
The extra assertion is so that the test doesn't pass when an exception was actually raised and the length of Results is 0.
My tests were reporting success even though they were really failing. The hidden error was this:
'CoveSessionInformation' object is not subscriptable
Because I hadn't rewritten the test code correctly.
It was actually the flakehell linter that caught this indirectly by warning my about a variable name that I had forgotten to update for the new test. I realised that if the linter could detect it, then the test should have failed, but it was gladly passing.
There was a problem hiding this comment.
Good catch, probably a dodgy use of all() on my part here
|
Now the PR contains everything I planned. How does it look? |
|
Hey Iain - sorry, I missed the last comment on this one. Will review when I get chance this week! |
|
Hi, @connelldave , happy new year! Do you think we can merge this? It would really help with being able to recommend botocove as an analysis tool in my security-sensitive projects. |
Allow setting session policy of assumed role
A rewrite of #15 to adapt to the changes in v1.4.0 (#19).
Solves #14.
Still needs support for managed policy ARNs.