Skip to content

Commit

Permalink
Merge pull request #713 from hashicorp/bugfix/user-permissions-for-ap…
Browse files Browse the repository at this point in the history 
…p-sp-resources

Don't call /directoryObjects for applications or service principals
  • Loading branch information
@manicminer
manicminer authored Jan 14, 2022
2 parents eeb7181 + fd0534c commit 6406b23
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 71 deletions.
2 changes: 2 additions & 0 deletions docs/resources/group.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ When authenticated with a service principal, this resource requires one of the f

If using the `assignable_to_role` property, this resource additionally requires one of the following application roles: `RoleManagement.ReadWrite.Directory` or `Directory.ReadWrite.All`

If specifying owners for a group, which are user principals, this resource additionally requires one of the following application roles: `User.Read.All`, `User.ReadWrite.All`, `Directory.Read.All` or `Directory.ReadWrite.All`

When authenticated with a user principal, this resource requires one of the following directory roles: `Groups Administrator`, `User Administrator` or `Global Administrator`

## Example Usage
Expand Down
55 changes: 19 additions & 36 deletions internal/services/applications/application_resource.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -981,10 +981,8 @@ func applicationResourceCreate(ctx context.Context, d *schema.ResourceData, meta
if callerObject == nil {
return tf.ErrorDiagF(errors.New("returned callerObject was nil"), "Could not retrieve calling principal object %q", callerId)
}
// TODO: remove this workaround for https://github.com/hashicorp/terraform-provider-azuread/issues/588
//if callerObject.ODataId == nil {
// return tf.ErrorDiagF(errors.New("ODataId was nil"), "Could not retrieve calling principal object %q", callerId)
//}

// @odata.id returned by API cannot be relied upon, so construct our own
callerObject.ODataId = (*odata.Id)(utils.String(fmt.Sprintf("%s/v1.0/%s/directoryObjects/%s",
client.BaseClient.Endpoint, client.BaseClient.TenantId, callerId)))

Expand All @@ -997,29 +995,25 @@ func applicationResourceCreate(ctx context.Context, d *schema.ResourceData, meta
// Retrieve and set the initial owners, which can be up to 20 in total when creating the application
if v, ok := d.GetOk("owners"); ok {
ownerCount := 0
for _, ownerId := range v.(*schema.Set).List() {
if strings.EqualFold(ownerId.(string), callerId) {
for _, ownerIdRaw := range v.(*schema.Set).List() {
ownerId := ownerIdRaw.(string)

// If the calling principal was found in the specified owners, we won't remove them later
if strings.EqualFold(ownerId, callerId) {
removeCallerOwner = false
continue
}
ownerObject, _, err := directoryObjectsClient.Get(ctx, ownerId.(string), odata.Query{})
if err != nil {
return tf.ErrorDiagF(err, "Could not retrieve owner principal object %q", ownerId)
}
if ownerObject == nil {
return tf.ErrorDiagF(errors.New("ownerObject was nil"), "Could not retrieve owner principal object %q", ownerId)

ownerObject := msgraph.DirectoryObject{
ODataId: (*odata.Id)(utils.String(fmt.Sprintf("%s/v1.0/%s/directoryObjects/%s",
client.BaseClient.Endpoint, client.BaseClient.TenantId, ownerId))),
ID: &ownerId,
}
// TODO: remove this workaround for https://github.com/hashicorp/terraform-provider-azuread/issues/588
//if ownerObject.ODataId == nil {
// return tf.ErrorDiagF(errors.New("ODataId was nil"), "Could not retrieve owner principal object %q", ownerId)
//}
ownerObject.ODataId = (*odata.Id)(utils.String(fmt.Sprintf("%s/v1.0/%s/directoryObjects/%s",
client.BaseClient.Endpoint, client.BaseClient.TenantId, ownerId)))

if ownerCount < 19 {
ownersFirst20 = append(ownersFirst20, *ownerObject)
ownersFirst20 = append(ownersFirst20, ownerObject)
} else {
ownersExtra = append(ownersExtra, *ownerObject)
ownersExtra = append(ownersExtra, ownerObject)
}
ownerCount++
}
Expand Down Expand Up @@ -1082,7 +1076,6 @@ func applicationResourceCreate(ctx context.Context, d *schema.ResourceData, meta

func applicationResourceUpdate(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics {
client := meta.(*clients.Client).Applications.ApplicationsClient
directoryObjectsClient := meta.(*clients.Client).Applications.DirectoryObjectsClient
applicationId := d.Id()
displayName := d.Get("display_name").(string)

Expand Down Expand Up @@ -1175,21 +1168,11 @@ func applicationResourceUpdate(ctx context.Context, d *schema.ResourceData, meta
if len(ownersToAdd) > 0 {
newOwners := make(msgraph.Owners, 0)
for _, ownerId := range ownersToAdd {
ownerObject, _, err := directoryObjectsClient.Get(ctx, ownerId, odata.Query{})
if err != nil {
return tf.ErrorDiagF(err, "Could not retrieve owner principal object %q", ownerId)
}
if ownerObject == nil {
return tf.ErrorDiagF(errors.New("returned ownerObject was nil"), "Could not retrieve owner principal object %q", ownerId)
}
// TODO: remove this workaround for https://github.com/hashicorp/terraform-provider-azuread/issues/588
//if ownerObject.ODataId == nil {
// return tf.ErrorDiagF(errors.New("ODataId was nil"), "Could not retrieve owner principal object %q", ownerId)
//}
ownerObject.ODataId = (*odata.Id)(utils.String(fmt.Sprintf("%s/v1.0/%s/directoryObjects/%s",
client.BaseClient.Endpoint, client.BaseClient.TenantId, ownerId)))

newOwners = append(newOwners, *ownerObject)
newOwners = append(newOwners, msgraph.DirectoryObject{
ODataId: (*odata.Id)(utils.String(fmt.Sprintf("%s/v1.0/%s/directoryObjects/%s",
client.BaseClient.Endpoint, client.BaseClient.TenantId, ownerId))),
ID: &ownerId,
})
}

properties.Owners = &newOwners
Expand Down
54 changes: 19 additions & 35 deletions internal/services/serviceprincipals/service_principal_resource.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -421,10 +421,8 @@ func servicePrincipalResourceCreate(ctx context.Context, d *schema.ResourceData,
if callerObject == nil {
return tf.ErrorDiagF(errors.New("returned callerObject was nil"), "Could not retrieve calling principal object %q", callerId)
}
// TODO: remove this workaround for https://github.com/hashicorp/terraform-provider-azuread/issues/588
//if callerObject.ODataId == nil {
// return tf.ErrorDiagF(errors.New("ODataId was nil"), "Could not retrieve calling principal object %q", callerId)
//}

// @odata.id returned by API cannot be relied upon, so construct our own
callerObject.ODataId = (*odata.Id)(utils.String(fmt.Sprintf("%s/v1.0/%s/directoryObjects/%s",
client.BaseClient.Endpoint, client.BaseClient.TenantId, callerId)))

Expand All @@ -437,29 +435,25 @@ func servicePrincipalResourceCreate(ctx context.Context, d *schema.ResourceData,
// Retrieve and set the initial owners, which can be up to 20 in total when creating the service principal
if v, ok := d.GetOk("owners"); ok {
ownerCount := 0
for _, ownerId := range v.(*schema.Set).List() {
if strings.EqualFold(ownerId.(string), callerId) {
for _, ownerIdRaw := range v.(*schema.Set).List() {
ownerId := ownerIdRaw.(string)

// If the calling principal was found in the specified owners, we won't remove them later
if strings.EqualFold(ownerId, callerId) {
removeCallerOwner = false
continue
}
ownerObject, _, err := directoryObjectsClient.Get(ctx, ownerId.(string), odata.Query{})
if err != nil {
return tf.ErrorDiagF(err, "Could not retrieve owner principal object %q", ownerId)
}
if ownerObject == nil {
return tf.ErrorDiagF(errors.New("ownerObject was nil"), "Could not retrieve owner principal object %q", ownerId)

ownerObject := msgraph.DirectoryObject{
ODataId: (*odata.Id)(utils.String(fmt.Sprintf("%s/v1.0/%s/directoryObjects/%s",
client.BaseClient.Endpoint, client.BaseClient.TenantId, ownerId))),
ID: &ownerId,
}
// TODO: remove this workaround for https://github.com/hashicorp/terraform-provider-azuread/issues/588
//if ownerObject.ODataId == nil {
// return tf.ErrorDiagF(errors.New("ODataId was nil"), "Could not retrieve owner principal object %q", ownerId)
//}
ownerObject.ODataId = (*odata.Id)(utils.String(fmt.Sprintf("%s/v1.0/%s/directoryObjects/%s",
client.BaseClient.Endpoint, client.BaseClient.TenantId, ownerId)))

if ownerCount < 19 {
ownersFirst20 = append(ownersFirst20, *ownerObject)
ownersFirst20 = append(ownersFirst20, ownerObject)
} else {
ownersExtra = append(ownersExtra, *ownerObject)
ownersExtra = append(ownersExtra, ownerObject)
}
ownerCount++
}
Expand Down Expand Up @@ -513,7 +507,6 @@ func servicePrincipalResourceCreate(ctx context.Context, d *schema.ResourceData,

func servicePrincipalResourceUpdate(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics {
client := meta.(*clients.Client).ServicePrincipals.ServicePrincipalsClient
directoryObjectsClient := meta.(*clients.Client).ServicePrincipals.DirectoryObjectsClient

var tags []string
if v, ok := d.GetOk("feature_tags"); ok && len(v.([]interface{})) > 0 && d.HasChange("feature_tags") {
Expand Down Expand Up @@ -558,20 +551,11 @@ func servicePrincipalResourceUpdate(ctx context.Context, d *schema.ResourceData,
if len(ownersToAdd) > 0 {
newOwners := make(msgraph.Owners, 0)
for _, ownerId := range ownersToAdd {
ownerObject, _, err := directoryObjectsClient.Get(ctx, ownerId, odata.Query{})
if err != nil {
return tf.ErrorDiagF(err, "Could not retrieve owner principal object %q", ownerId)
}
if ownerObject == nil {
return tf.ErrorDiagF(errors.New("returned ownerObject was nil"), "Could not retrieve owner principal object %q", ownerId)
}
// TODO: remove this workaround for https://github.com/hashicorp/terraform-provider-azuread/issues/588
//if ownerObject.ODataId == nil {
// return tf.ErrorDiagF(errors.New("ODataId was nil"), "Could not retrieve owner principal object %q", ownerId)
//}
ownerObject.ODataId = (*odata.Id)(utils.String(fmt.Sprintf("%s/v1.0/%s/directoryObjects/%s",
client.BaseClient.Endpoint, client.BaseClient.TenantId, ownerId)))
newOwners = append(newOwners, *ownerObject)
newOwners = append(newOwners, msgraph.DirectoryObject{
ODataId: (*odata.Id)(utils.String(fmt.Sprintf("%s/v1.0/%s/directoryObjects/%s",
client.BaseClient.Endpoint, client.BaseClient.TenantId, ownerId))),
ID: &ownerId,
})
}

properties.Owners = &newOwners
Expand Down

0 comments on commit 6406b23

Please sign in to comment.