Repository: getmoto/moto
Pull request #7639 (2024-04-29 08:09:15)

EC2: Fix unhandled error for nonexistent security groups
This PR fixes an unhandled case where a non-existent security group when passed to `RevokeSecurityGroupIngress` and `RevokeSecurityGroupEgress` would raise an `AttributeError`.

```
  File "/home/viren/repo/moto-ext/moto/core/responses.py", line 270, in dispatch
    return cls()._dispatch(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/viren/repo/moto-ext/moto/core/responses.py", line 478, in _dispatch
    return self.call_action()
           ^^^^^^^^^^^^^^^^^^
  File "/home/viren/repo/moto-ext/moto/core/responses.py", line 575, in call_action
    response = method()
               ^^^^^^^^
  File "/home/viren/repo/moto-ext/moto/ec2/responses/security_groups.py", line 236, in revoke_security_group_ingress
    self.ec2_backend.revoke_security_group_ingress(**args)
  File "/home/viren/repo/moto-ext/moto/ec2/models/security_groups.py", line 824, in revoke_security_group_ingress
    group.group_id,
    ^^^^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'group_id'
```

The expected behaviour is as follows (tested against AWS)

```
$ aws ec2 revoke-security-group-ingress --group-name fasdf

An error occurred (InvalidGroup.NotFound) when calling the RevokeSecurityGroupIngress operation: The security group 'fasdf' does not exist in default VPC 'vpc-026b0bxxxxxx'
```

Supersedes https://github.com/getmoto/moto/pull/7428

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/getmoto/moto/pull/7639?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getmoto) Report
Attention: Patch coverage is `50.00000%` with `2 lines` in your changes are missing coverage. Please review.
> Project coverage is 94.29%. Comparing base [(`9aef694`)](https://app.codecov.io/gh/getmoto/moto/commit/9aef6944da6a6f1b84be99cefb34f1724e6ca58f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getmoto) to head [(`db3e016`)](https://app.codecov.io/gh/getmoto/moto/pull/7639?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getmoto).
> Report is 216 commits behind head on master.

| [Files](https://app.codecov.io/gh/getmoto/moto/pull/7639?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getmoto) | Patch % | Lines |
|---|---|---|
| [moto/ec2/models/security\_groups.py](https://app.codecov.io/gh/getmoto/moto/pull/7639?src=pr&el=tree&filepath=moto%2Fec2%2Fmodels%2Fsecurity_groups.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getmoto#diff-bW90by9lYzIvbW9kZWxzL3NlY3VyaXR5X2dyb3Vwcy5weQ==) | 50.00% | [2 Missing :warning: ](https://app.codecov.io/gh/getmoto/moto/pull/7639?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getmoto) |

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master    #7639      +/-   ##
==========================================
- Coverage   95.88%   94.29%   -1.60%     
==========================================
  Files         843     1082     +239     
  Lines       82578    92271    +9693     
==========================================
+ Hits        79178    87004    +7826     
- Misses       3400     5267    +1867     
```

| [Flag](https://app.codecov.io/gh/getmoto/moto/pull/7639/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getmoto) | Coverage Δ | |
|---|---|---|
| [servertests](https://app.codecov.io/gh/getmoto/moto/pull/7639/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getmoto) | `29.41% <0.00%> (-3.11%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/getmoto/moto/pull/7639/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getmoto) | `94.26% <50.00%> (-1.59%)` | :arrow_down: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getmoto#carryforward-flags-in-the-pull-request-comment) to find out more.


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/getmoto/moto/pull/7639?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getmoto).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getmoto).


Patch:
diff --git a/moto/ec2/models/security_groups.py b/moto/ec2/models/security_groups.py
index 532313652..81760baf5 100644
--- a/moto/ec2/models/security_groups.py
+++ b/moto/ec2/models/security_groups.py
@@ -812,6 +812,9 @@ class SecurityGroupBackend:
             group_name_or_id, vpc_id
         )  # type: ignore[assignment]
 
+        if group is None:
+            raise InvalidSecurityGroupNotFoundError(group_name_or_id)
+
         if security_rule_ids:
             group.ingress_rules = [
                 rule for rule in group.ingress_rules if rule.id not in security_rule_ids
@@ -983,6 +986,9 @@ class SecurityGroupBackend:
             group_name_or_id, vpc_id
         )  # type: ignore[assignment]
 
+        if group is None:
+            raise InvalidSecurityGroupNotFoundError(group_name_or_id)
+
         if security_rule_ids:
             group.egress_rules = [
                 rule for rule in group.egress_rules if rule.id not in security_rule_ids


Test patch:
diff --git a/tests/test_ec2/test_security_groups.py b/tests/test_ec2/test_security_groups.py
index cbb9cb5ad..ed6127d42 100644
--- a/tests/test_ec2/test_security_groups.py
+++ b/tests/test_ec2/test_security_groups.py
@@ -304,6 +304,13 @@ def test_authorize_ip_range_and_revoke():
         {"CidrIp": "123.123.123.123/32"}
     ]
 
+    # Bad group must produce error
+    with pytest.raises(ClientError) as ex:
+        client.revoke_security_group_egress(GroupId="badcompany")
+    assert ex.value.response["ResponseMetadata"]["HTTPStatusCode"] == 400
+    assert "RequestId" in ex.value.response["ResponseMetadata"]
+    assert ex.value.response["Error"]["Code"] == "InvalidGroup.NotFound"
+
     # Wrong Cidr should throw error
     with pytest.raises(ClientError) as ex:
         wrong_permissions = copy.deepcopy(egress_permissions)
@@ -380,6 +387,13 @@ def test_authorize_other_group_and_revoke():
     assert "RequestId" in ex.value.response["ResponseMetadata"]
     assert ex.value.response["Error"]["Code"] == "InvalidGroup.NotFound"
 
+    # Bad group must produce error
+    with pytest.raises(ClientError) as ex:
+        client.revoke_security_group_ingress(GroupId="badcompany")
+    assert ex.value.response["ResponseMetadata"]["HTTPStatusCode"] == 400
+    assert "RequestId" in ex.value.response["ResponseMetadata"]
+    assert ex.value.response["Error"]["Code"] == "InvalidGroup.NotFound"
+
     # Actually revoke
     security_group.revoke_ingress(IpPermissions=permissions)