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

Correct group inline policy rendering #3069

Conversation

limitusus
Copy link
Contributor

GetAccountAuthorizationDetails returns an invalid response in group inline policies.
PolicyDocument should be an IAM policy document JSON, however the current implementation returns a Python dict expression.

Similar implementation can be found just below the code, in role inline policies, which seems correct.
https://github.com/spulec/moto/blob/cb600377b48ca676fc6a29a6690aeb51e702da43/moto/iam/responses.py#L2123
This PR fixes group inline policy rendering just like role inline policy.

How to ensure issue

The small code below is a PoC with a group and a policy.
Backend data are correct: you can see a normal JSON.

#!/usr/bin/env python

from moto import mock_iam
from moto.iam.models import iam_backend
import boto3

@mock_iam
def t():
    policy_document = """
{
  "Version": "2012-10-17",
  "Statement":
    {
      "Effect": "Allow",
      "Action": "s3:ListBucket",
      "Resource": "arn:aws:s3:::example_bucket"
    }
}
"""

    iam = boto3.client("iam")
    iam.create_group(GroupName="test-role")
    iam.put_group_policy(
        GroupName="test-role",
        PolicyName="s3",
        PolicyDocument=policy_document,
    )
    ret = iam.get_group_policy(GroupName="test-role", PolicyName="s3")
    d = iam_backend.get_account_authorization_details(["Group"])
    for group in d["groups"]:
        # like in template
        print(f"Group: {group.name}")
        for policy in group.policies:
            print(f"  p: {policy}")
            print(f"  d: {group.get_policy(policy)}")
        # backend data
        print(group.policies["s3"])

t()

Output

Group: test-role
  p: s3
  d: {'policy_name': 's3', 'policy_document': '\n{\n  "Version": "2012-10-17",\n  "Statement":\n    {\n      "Effect": "Allow",\n      "Action": "s3:ListBucket",\n      "Resource": "arn:aws:s3:::example_bucket"\n    }\n}\n', 'group_name': 'test-role'}

{
  "Version": "2012-10-17",
  "Statement":
    {
      "Effect": "Allow",
      "Action": "s3:ListBucket",
      "Resource": "arn:aws:s3:::example_bucket"
    }
}

Copy link
Collaborator

@bblommers bblommers left a comment

Choose a reason for hiding this comment

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

Hi @limitusus, thanks for the PR!

Can you add the test case to Moto as well? I'm a little surprised that the the build seems to be passing - I guess we don't have test coverage for this specified field yet

@coveralls
Copy link

coveralls commented Jun 12, 2020

Coverage Status

Coverage increased (+0.003%) to 94.389% when pulling 5b8c592 on limitusus:bugfix/iam-get-account-authorization-details into 475f022 on spulec:master.

@limitusus
Copy link
Contributor Author

@bblommers Sure I'll try that

@limitusus
Copy link
Contributor Author

@bblommers
I added tests for user/role/group inline policies.
While running tests, I found user inline policies are not included in the response, so I fixed that issue as well.

NOTE: UserPolicyList element is omitted if empty, while RolePolicyList and GroupPolicyList is not.

tests/test_iam/test_iam.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@bblommers bblommers left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks again, @limitusus!

@bblommers bblommers merged commit 849f16f into getmoto:master Jun 14, 2020
@limitusus limitusus deleted the bugfix/iam-get-account-authorization-details branch August 5, 2020 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants