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.

Sign up for GitHub

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

Don't call /directoryObjects for applications or service principals #713

Merged
merged 2 commits into from
Jan 14, 2022
Merged

Don't call /directoryObjects for applications or service principals #713

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7c8268c
Don't retrieve owner principals as this may requires additional permi…
manicminer Jan 10, 2022
fd0534c
Document additional potential required permissions for groups
manicminer Jan 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/resources/group.md
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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