Make access_type required and support application roles and SharePoint URLs#1782
Make access_type required and support application roles and SharePoint URLs#1782jsfr wants to merge 2 commits intohashicorp:mainfrom
access_type required and support application roles and SharePoint URLs#1782Conversation
|
Hi, would it make sense to include a fix for #1776 in this pr as well since it's closely related to this? (Also really hope it gets merged this time) |
jackofallops
left a comment
jackofallops
left a comment
There was a problem hiding this comment.
Hi @jsfr - Thanks for this PR. Unfortunately marking an Optional property as Required is a breaking change for configs, so we can't accept this in the current implementation. If you can take a look at validating the appropriate values in a CustomizeDiff on the resource instead, we can remove the breaking change flag, and still provide users with plan-time validation. There's a few other comments to take a look at too.
Thanks!
| Description: "The role of access type to the specified resource - for `AadGroup` valid values are `Member` and `Owner`, for `AadApplication` it must be a UUID and for `SharePointOnline` it must be a URL", | ||
| Type: pluginsdk.TypeString, | ||
| Optional: true, | ||
| Required: true, |
There was a problem hiding this comment.
Unfortunately, changing from Optional to Required is a breaking change (users not already specifying this value will encounter errors in plan/apply).
Instead, this will need to be implemented using a CustomizeDiff to guard against the incorrect values being specified.
| case "AadGroup": | ||
| originId = fmt.Sprintf("%s_%s", accessType, catalogResourceAssociationId.OriginId) | ||
| displayName = accessType |
There was a problem hiding this comment.
To provide correct backward compatibility, this should be the default: - This is due to the resource currently not handling validation on the value of resource. OriginSystem and passing it straight through. If this is actually manifesting as an error/issue, we can revisit. Specifically, since we don't validate/restrict input on the azuread_access_package_resource_catalog_association.resource_origin_system value, this is likely to cause some users issues.
internal/services/identitygovernance/access_package_resource_package_association_resource.go
Show resolved
Hide resolved
| func TestAccAccessPackageResourcePackageAssociation_invalidAccessType(t *testing.T) { | ||
| data := acceptance.BuildTestData(t, "azuread_access_package_resource_package_association", "test") | ||
| r := AccessPackageResourcePackageAssociationResource{} | ||
|
|
||
| data.ResourceTest(t, r, []acceptance.TestStep{ | ||
| { | ||
| Config: r.invalidAccessType(data), | ||
| ExpectError: regexp.MustCompile(`expected access_type to be one of \[Member Owner\]`), | ||
| }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
As noted above, raising this error is likely to cause issues and/or breaking changes for some users.
| func TestAccAccessPackageResourcePackageAssociation_invalidAccessType(t *testing.T) { | |
| data := acceptance.BuildTestData(t, "azuread_access_package_resource_package_association", "test") | |
| r := AccessPackageResourcePackageAssociationResource{} | |
| data.ResourceTest(t, r, []acceptance.TestStep{ | |
| { | |
| Config: r.invalidAccessType(data), | |
| ExpectError: regexp.MustCompile(`expected access_type to be one of \[Member Owner\]`), | |
| }, | |
| }) | |
| } |
| } | ||
|
|
||
| func (AccessPackageResourcePackageAssociationResource) complete(data acceptance.TestData) string { | ||
| func (AccessPackageResourcePackageAssociationResource) completeWithGroup(data acceptance.TestData) string { |
There was a problem hiding this comment.
Can we leave this test name intact, and name the others withAccessTypeOwner etc?
- Revert access_type from Required to Optional with Default "Member" - Add CustomizeDiff for access_type validation - Make SharePointOnline the default case for backward compatibility - Restore createMsg template variable for error messages - Remove invalidAccessType test - Rename test functions per reviewer request
jsfr
force-pushed
the
jens/access-package-app-role-support
branch
from
f7d91dd to
dae0ed8
Compare
|
@jackofallops I've tried to address your comments now; please let me know if this suffices. Sorry about the long wait here, but this ended up on the back burner for me. |
3 participants
Community Note
Description
This PR makes the
access_typeargument required for theazuread_access_package_resource_package_associationresource and extends its validation to support application role UUIDs and SharePoint URLs in addition to the existingMemberandOwnervalues for Azure AD groups.The change enables users to grant access to application roles (by specifying a role UUID) and SharePoint Online sites (by specifying a URL) through access packages, not just group memberships.
This is the second try at this PR. The previous attempt was in #1627.
Changes to existing Resource / Data Source
Testing
Added these new test cases:
TestAccAccessPackageResourcePackageAssociation_completeWithGroup- validates group membership assignmentTestAccAccessPackageResourcePackageAssociation_completeWithGroupOwner- validates group owner assignmentTestAccAccessPackageResourcePackageAssociation_completeWithApplication- validates application role assignment with UUIDTestAccAccessPackageResourcePackageAssociation_invalidAccessType- validates error handling for invalid access typesI've run all unit tests, but not acceptance tests, as those require creating real resources. I have, however, used the provider locally in a regular setup to create access packages as expected.
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
azuread_access_package_resource_package_association- theaccess_typeproperty is now required and supports application role UUIDs and SharePoint URLs in addition toMemberandOwnervaluesThis is a (please select all that apply):
Related Issue(s)
Fixes #1066
Rollback Plan
If a change needs to be reverted, we will publish an updated version of the provider.
Changes to Security Controls
This change extends the access control capabilities by enabling application role assignments and SharePoint site access through access packages. No changes to encryption or logging.
Note
If this PR changes meaningfully during the course of review please update the title and description as required.