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

Support creation of Azure AD Application with "Native" application type #13

Closed
shopskyi opened this issue Jan 10, 2019 · 15 comments
Closed

Comments

@shopskyi
Copy link

shopskyi commented Jan 10, 2019

Please add ability to create azuread_application resource with application type "Native". By default, azuread_application resource is being created with application type is "Web app / API"

For example:

resource "azuread_application" "app" {
name = "app"
type = "native"
}

@sshkel
Copy link
Contributor

sshkel commented Jan 20, 2019

Was looking for the same feature so I started mocking it out. I am fairly new to terraform/azure world, so was looking to bounce off some ideas.
I am looking at how cli has implemented this and boy it ain't pretty. Graph api doesn't have support for native apps so recommended hack is to create a webapp and convert it to native.
https://github.com/Azure/azure-cli/blob/d7a4b887fc2a0d907191be846228b9b528003ee7/src/command_modules/azure-cli-role/azure/cli/command_modules/role/custom.py#L701
I followed the same principal, but one thing I can't figure out is how to unset homepage while doing so.
Setting it to nil doesn't seem to work and an empty string doesn't pass validation. Would appreciate some advice here. Here is modified app function

func resourceApplicationCreate(d *schema.ResourceData, meta interface{}) error {
	client := meta.(*ArmClient).applicationsClient
	ctx := meta.(*ArmClient).StopContext

	name := d.Get("name").(string)

	properties := graphrbac.ApplicationCreateParameters{
		DisplayName:             &name,
		Homepage:                expandADApplicationHomepage(d, name),
		IdentifierUris:          expandADApplicationIdentifierUris(d),
		ReplyUrls:               expandADApplicationReplyUrls(d),
		AvailableToOtherTenants: p.Bool(d.Get("available_to_other_tenants").(bool)),
	}

	if v, ok := d.GetOk("oauth2_allow_implicit_flow"); ok {
		properties.Oauth2AllowImplicitFlow = p.Bool(v.(bool))
	}

	app, err := client.Create(ctx, properties)
	if err != nil {
		return err
	}

	// support for native apps
	if v, ok := d.GetOk("type"); ok && v == "native" {
		properties := graphrbac.ApplicationUpdateParameters{
			Homepage:       nil,
			IdentifierUris: &[]string{},
			AdditionalProperties: map[string]interface{}{
				"publicClient": true,
			},
		}
		res, err := client.Patch(ctx, *app.ObjectID, properties)
		if err != nil {
			log.Print(res)
			return err
		}
	}

	d.SetId(*app.ObjectID)

	return resourceApplicationRead(d, meta)
}

@sshkel
Copy link
Contributor

sshkel commented Jan 21, 2019

@tombuildsstuff if you have any thoughts on this one would appreciate it. Tagged you because you were the last one to commit to this repo :)

@katbyte
Copy link
Collaborator

katbyte commented Jan 22, 2019

Hi @sshkel,

This looks like a good start to me. Do we need to clear the homepage? How does the CLI handle the homepage, does it allow an empty one to be set or does it simply ignore the property in the long run?

@sshkel
Copy link
Contributor

sshkel commented Jan 22, 2019

Hi @katbyte, thanks for volunteering to help out ^_^
When I run it through cli with minimal args, it sets homepage to null. E.g.
az ad app create --display-name yolo-cli --native-app
Though cli allows me to provide homepage for native app.
az ad app create --display-name yolo-cli --native-app --homepage "homepageio"
So I suspect it's not a requirement? That's the part I wanted to confirm and it would be nice to have a consistent behaviour between cli and the terraform module.
Empty value for homepage seems like not a valid thing in general
Doing this fails

az ad app create --display-name yolo-cli --native-app --homepage ""
Invalid value specified for property 'homepage' of resource 'Application'.

@katbyte
Copy link
Collaborator

katbyte commented Jan 23, 2019

Hmm, so it sounds like the CLI is passing nil, and your saying if you make homepage optional and try:

	if v, ok := d.GetOk("homepage"); ok {
		properties. Homepage = expandADApplicationHomepage(d, name)
	}

doesn't it work?

@sshkel
Copy link
Contributor

sshkel commented Jan 24, 2019

ahhh, no, you are right we can totally do that. Looks like current application module diverged from what cli does. With cli if you don't provide a homepage it sets it to null, while terraform module uses expandADApplicationHomepage that helpfully replaces it to https:// + name, if homepage not defined.
Thanks @katbyte, appreciate you holding my hand through this 😄

I will cleanup my code a little bit and get something for a PR.

@katbyte
Copy link
Collaborator

katbyte commented Jan 24, 2019

Happy to help @sshkel 🙂 look forward to the PR

@ghost ghost removed the waiting-response label Jan 24, 2019
@dominik-lekse
Copy link

An AAD application of type "native" has the property "publicClient": true whereas an AAD application of type "Web App" shows "publicClient": null. The az cli command az ad app show shows the property publicClient while in the Azure SDK for Go this property is

To keep consistency with the Azure API, I suggest to name the Terraform resource attribute public_client instead of introducing a type attribute.

@sshkel
Copy link
Contributor

sshkel commented Jan 30, 2019

@katbyte was wondering if you can give me a hand again. There are a couple things I am stuck with.

  1. I am not sure what's the best way to deal with import state functionality. I am currently failing with and stuck to find how to address this. I suspect it has something to do with how importer is implemented atm.
--- FAIL: TestAccAzureADApplication_basic (11.86s)
    testing.go:538: Step 1 error: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.

        (map[string]string) {
        }


        (map[string]string) (len=1) {
         (string) (len=4) "type": (string) (len=10) "webapp/api"
        }
  1. Since we are doing the native app creating in 2 steps at this point do we need to do something with partial state?
    Any help is much appreciated

@sshkel
Copy link
Contributor

sshkel commented Jan 30, 2019

@dominik-lekse makes sense, happy to refactor things if anyone else doesn't have strong objections.

@jjgrayston
Copy link

how's this coming along?

@sshkel
Copy link
Contributor

sshkel commented Mar 25, 2019

@jjgrayston there is a PR currently in review

@praneetloke
Copy link

@sshkel just wondering if you think the linked PR would be merged soon? Thank you so much for doing the work on this! This is an essential feature for being able to manage AAD apps.

@sshkel
Copy link
Contributor

sshkel commented Apr 30, 2019

@praneetloke I am waiting for @katbyte to do a final review, any further tweaks should be minor and quick.
You are welcome, friend :)

@ghost
Copy link

ghost commented Jun 23, 2019

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Jun 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants