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

Fixes for some object ownership issues with applications, groups and service principals #519

Merged
merged 12 commits into from
Aug 20, 2021

Conversation

manicminer
Copy link
Contributor

@manicminer manicminer commented Aug 13, 2021

This PR is functionally complete but pending manicminer/hamilton#86

For applications and service principals:

Where no owners are specified in configuration, The calling principal is explicitly assigned as the owner at creation time, and then removed immediately afterwards. This provides stable behavior across resources where sometimes the API will assign the caller as the owner and sometimes not, and it's impossible to explicitly specify zero owners at creation time.

Where owners are specified, 19 of them are assigned at creation time, in addition to the calling principal. This is unordered since the owners property is a TypeSet in all cases. The remaining owners (if any) are subsequently assigned immediately after creation. If the calling principal isn't included in the configured owners, it's removed immediately afterwards.

For groups:

API Issues

  • New groups are assigned the calling principal as the sole owner (contrary to documentation)
  • It's impossible to create a group with no owners (contrary to documentation)
    • Empty list: The value of 'odata.bind' property annotation is an empty array. The 'odata.bind' property annotation must have a non-empty array as its value
    • null: Invalid value for [email protected] property
  • Groups cannot have all their owners removed (documented)
  • M365 groups must have at least one owner that is a user (undocumented)
    • This is inconsistently enforced. It's possible to create an M365 group with a single service principal owner at the expense of further validations errors when later updating owners.
  • Security / legacy groups have erroneously inherited some of the validation for M365 group user owners
    • When removing the last user owner from a security group, this error is returned: The group must have at least one owner, hence this owner cannot be removed.

Remediation

If no owners are specified in configuration, the calling principal is assigned ownership and retains that ownership. The owners property is optional + computed.

Where owners are specified, the calling principal is not given ownership. The first (up to) 20 principals specified in configuration are preferentially selected to be the calling principal (if included in the configuration), then any users specified, followed by any other service principals - these are included in the POST request to create the group. Any additional owners are added one-by-one post-creation of the group.

This aims to work around strict validation with Microsoft 365 groups where at least one owner must be a user at all times! This validation has also been observed erroneously with legacy / security groups too. The API also prevents removal of the last remaining user owner, if a user has ever been assigned ownership of a group. A group cannot have all its owners removed (hence MinItems: 1). Ensuring that at least one user is included in the set of owners in configuration is an exercise left to the practitioner.

Unfortunately very little of this can be validated at plan time, since the owner principals may or may not exist at that time. Have opted to leave the API errors around owner removal exposed at apply time, since any validation we add at this point is papering over that and doesn't improve things.

Create and Update timeouts are also increased for groups to 20 mins, and applications & service princpals to 10 mins, to accommodate the time needed to post large numbers of members/owners.

Closes: #478
Closes: #517

Enhancements

  • azuread_service_principal - add support for the owners property

…service principals

Where no owners are specified in configuration, The calling principal
is explicitly assigned as the owner at creation time, and then removed
immediately afterwards. This provides stable behavior across resources
where sometimes the API will assign the caller as the owner and
sometimes not, and it's impossible to explicitly specify zero owners
at creation time.

Where owners are specified, 19 of them are assigned at creation time,
in addition to the calling principal. This is unordered since the
`owners` property is a TypeSet in all cases. The remaining owners (if
any) are subsequently assigned immediately after creation.

However for groups, the first 19 principals are preferentially
selected to be users, followed by service principals. This aims to
work around strict validation with Microsoft 365 groups where at least
one owner must be a user at all times! Ensuring that at least one user
is included in the set of owners in configuration is an exercise left
to the practitioner.

In all cases, if the calling principal is not explicitly included in the
set of owners in the configuration, it will not remain an owner
following an apply operation. This too is a decision left to the
practitioner to explicitly include this principal as an owner if so
required.

Create and Update timeouts are also increased for groups to 20 mins,
and applications & service princpals to 10 mins, to accommodate the time
needed to post large numbers of members/owners.
@manicminer manicminer force-pushed the bugfix/ownership-alignment branch from 3a942a7 to 85401e1 Compare August 17, 2021 14:07
@manicminer manicminer force-pushed the bugfix/ownership-alignment branch from 85401e1 to e2c7f71 Compare August 17, 2021 16:26
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM aside from some comments on var naming - i've only commented in one place but it applies to all the places following that pattern.

internal/services/applications/application_resource.go Outdated Show resolved Hide resolved
internal/services/groups/group_resource.go Outdated Show resolved Hide resolved
internal/services/groups/group_resource.go Outdated Show resolved Hide resolved
@manicminer
Copy link
Contributor Author

Test results

Applications

Screenshot 2021-08-20 at 00 52 00

Groups

One unrelated failure, one failure attributable to as-yet-unhanded replication delay with groups having dozens of owners (users unlikely to observe, to be fixed separately)

Screenshot 2021-08-20 at 00 52 18

Service Principals

One failure attributable to as-yet-unhanded replication delay with service principals having dozens of owners (users unlikely to observe, to be fixed separately)

Screenshot 2021-08-20 at 00 52 34

Users

Screenshot 2021-08-20 at 01 18 02

@manicminer manicminer merged commit d25f966 into main Aug 20, 2021
@manicminer manicminer deleted the bugfix/ownership-alignment branch August 20, 2021 00:18
manicminer added a commit that referenced this pull request Aug 20, 2021
@github-actions
Copy link

This functionality has been released in v2.0.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.