Add IP and Country Named Locations

[alexwilcox9]

Hi @manicminer
First attempt at contributing to any GoLang project so if there are obvious seeming mistakes I apologise in advance!
In this PR I've added named location (both country and IP based) and I'm planning to continue on to add full conditional policies if all goes well.

I have a couple of questions that I'm hoping you can help with:

• I've had to comment out the created/modified date in order to stop an error around these values being read only. I see some of your existing types have these same attributes but I can't work out how you got around that issue
• As Graph uses the same endpoint for country and IP based named locations you have to send the ODataType attribute, is there a way to set the default value of that in the model definition as that should theoretically never change?
• Is having two models the right way to go for this resource or should there just be one named location model that contains all the possible attributes of both types?
• Is this the right way to get CA policy support in the AzureAD Terraform provider or should I be contributing directly to that?

Thanks!

[manicminer]

@alexwilcox9 Thanks for this PR, this is off to a great start!

For readonly fields, I believe if you use omitempty in your struct tag, and leave the field with a nil pointer reference, it should be omitted from requests and avoid any API errors related to setting that readonly field - but if you are having issues with this we can test and work this out before merging.

It's a great question whether or not we should implement multiple clients here. Given the API exposes these subtypes via a single endpoint, and supports other methods via that endpoint, I'm leaning towards the latter and having a single NamedLocationsClient, and a set of models for the different types of named locations - largely modelling the API types.

This client could have methods like CreateIPNamedLocation and CreateCountryNamedLocation (or possibly just CreateIP / CreateCountry), which would each accept their respective model. A client is not necessarily expected to always have the same methods and it could simply not have a Create method. This would also allow listing of all named locations via a List method with filtering left to the user, a common Delete method etc.

OData metadata is populated when parsing API responses but there's currently no support for automagically handling these fields in requests, so for the @odata.x fields which accept a single value - such as @odata.type in this case which informs the API which object you are sending - I think it's ok to have the client forcibly set these in the incoming model variable for now. We should be able to cleanly refactor this later.

The models could use struct embedding to implement a base NamedLocation model embedded in child types such as IPNamedLocation and CountryNamedLocation, for example:

type NamedLocation struct {
	ID               *string    `json:"id,omitempty"`
	CreatedDateTime  *time.Time `json:"createdDateTime,omitempty"`
	DisplayName      *string    `json:"displayName,omitempty"`
	ModifiedDateTime *time.Time `json:"modifiedDateTime,omitempty"`
}

type CountryNamedLocation struct {
	*NamedLocation
	CountriesAndRegions               *[]string `json:"countriesAndRegions,omitempty"`
	IncludeUnknownCountriesAndRegions *bool     `json:"includeUnknownCountriesAndRegions,omitempty"`
}

In terms of getting this support into the Terraform AzureAD provider, at present it would first need to be added to this SDK, so thanks for submitting this here!

FYI there's another PR #23 in progress for conditional access policies which looks fairly solid, to avoid any duplicate efforts there :)

[alexwilcox9]

HI @manicminer thanks for the review!

I haven't encountered embedding before so this is a useful tip. If I create a list function that returns a slice of NamedLocation's does that mean the type specific attributes like CountriesAndRegions will not be captured? And if so does that matter?

Cheers for the heads up on that other PR, luckily it looks like we've managed to avoid overlap so far!

[manicminer]

The common list function could return an interface which would allow the user to use type assertion it to discover the actual type, e.g.

// models

type NamedLocation interface {}

type BaseNamedLocation struct {
	// common fields
}

type CountryNamedLocation struct {
	*BaseNamedLocation
	// country-specific fields
}

type IPNamedLocation struct {
	*BaseNamedLocation
	// ip-specific fields
}

// clients

type NamedLocationClient struct {}

func (c NamedLocationClient) List() (*[]NamedLocation, error) {
	r := CountryNamedLocation{
		BaseNamedLocation: &BaseNamedLocation{},
	}
	return &[]NamedLocation{r}, nil
}

func main() {
	locs, _ := NamedLocationClient{}.List()
	if locs != nil {
		for _, l := range *locs {
			fmt.Printf("%+v ", l)
			if _, ok := l.(CountryNamedLocation); ok {
				fmt.Println("is a CountryNamedLocation")
			} else if _, ok := l.(IPNamedLocation); ok {
				fmt.Println("is an IPNamedLocation")
			}
		}
	}
}

(I renamed NamedLocation to BaseNamedLocation for clarity)

In this example the locations in the result are being tested to see which type they are.

These are just suggestions, if it feels too clunky when you are implementing it, feel free to try something different! :)

[alexwilcox9]

Thanks again for your tips, I'm definitely getting closer

So I've tried changing the list function to return an interface but I'm not having much luck with the type assertion

In the list function I am unmarshalling the response into a slice of the interface

var data struct {
	NamedLocations []models.NamedLocation `json:"value"`
}

if err := json.Unmarshal(respBody, &data); err != nil {
	return nil, status, err
}

return &data.NamedLocations, status, nil

And in the test I try the type assertion but it never seems to match

namedLocationSlice := testNamedLocationClient_List(t, c, "")
if namedLocationSlice != nil {
	for _, l := range *namedLocationSlice {
		t.Logf("%+v ", l)
		if _, ok := l.(models.CountryNamedLocation); ok {
			t.Logf("is a CountryNamedLocation")
		} else if _, ok := l.(models.IPNamedLocation); ok {
			t.Logf("is an IPNamedLocation")
		} else {
			t.Logf("Did not match a type")
		}
	}

}

Here's the output from the test

=== RUN   TestNamedLocationClient
    namedlocation_test.go:67: map[@odata.type:#microsoft.graph.ipNamedLocation createdDateTime:2021-04-08T19:47:53.3813587Z displayName:test-updated-ipnl-93TBGeGC id:0c359e8f-57e6-4da4-a424-294e67f214dd ipRanges:[map[@odata.type:#microsoft.graph.iPv4CidrRange cidrAddress:8.8.8.8/32] map[@odata.type:#microsoft.graph.iPv6CidrRange cidrAddress:2001:4860:4860::8888/128]] isTrusted:true modifiedDateTime:2021-04-08T19:48:07.0612108Z] 
    namedlocation_test.go:73: Did not match a type
    namedlocation_test.go:67: map[@odata.type:#microsoft.graph.countryNamedLocation countriesAndRegions:[US GB] createdDateTime:2021-04-08T19:47:54.6160586Z displayName:test-updated-cnl-93TBGeGC id:1074a8f5-b603-4837-9457-11ef532153b6 includeUnknownCountriesAndRegions:false modifiedDateTime:2021-04-08T19:48:08.0334762Z] 
    namedlocation_test.go:73: Did not match a type
--- PASS: TestNamedLocationClient (19.48s)
PASS
ok      github.com/manicminer/hamilton/clients  19.484s

Any input on this would be greatly appreciated, scratching my head on this one!

[manicminer]

@alexwilcox9 In the client List() function, you are unmarshaling the response into a slice of interface{} and the model structs aren't used. I'm assuming the response json has @odata.type set for each element of the array to indicate which type of object it is? If so, you could use the odata struct to pull out any useful odata, then determine the type for each one and attempt to unmarshal it using the appropriate model struct, before appending to a slice of NamedLocation and returning that.

Something like (no error checking, totally untested, may not even compile, pretty much pseudocode):

var result struct {
	NamedLocations *[]json.RawMessage `json:"value"`
}
json.Unmarshal(respBody, &data)

if result.NamedLocations == nil {
	return
}
ret := make([]NamedLocation, 0, len(result.NamedLocations))
for _, namedLocation := range *result.NamedLocations {
	var o odata.OData
	json.Unmarshal(namedLocation, &o)

	switch o.Type {
	case "#microsoft.graph.countryNamedLocation":
		var loc CountryNamedLocation
		json.Unmarshal(namedLocation, &loc)
		ret = append(ret, loc)
	case "#microsoft.graph.ipNamedLocation":
		var loc IPNamedLocation
		json.Unmarshal(namedLocation, &loc)
		ret = append(ret, loc)
	}
}

return ret

This essentially starts off by unmarshaling the containing array into a slice of RawMessage (which is just a byteslice alias), so you can loop through the items and unmarshal them individually. Then for each one it unmarshals using the OData struct (in this project) to inspect the @odata.type property, and then decides which struct to use to unmarshal the useful data before appending to a slice which gets returned at the end.

I think this will work, but shout out if it doesn't make sense or I've overlooked something :)

[alexwilcox9]

@manicminer I think I've got it! Thanks for the example code, I messed around with the pointers until it stopped erroring and it seems to work 😂

$ go test -v -timeout 30s -run ^TestNamedLocationClient$ github.com/manicminer/hamilton/clients
=== RUN   TestNamedLocationClient
    namedlocation_test.go:61: {BaseNamedLocation:0xc000682450 IPRanges:0xc0003fa360 IsTrusted:0xc000400238} 
    namedlocation_test.go:65: is an IPNamedLocation
    namedlocation_test.go:61: {BaseNamedLocation:0xc0006824b0 CountriesAndRegions:0xc0003fa4b0 IncludeUnknownCountriesAndRegions:0xc000400324} 
    namedlocation_test.go:63: is a CountryNamedLocation
--- PASS: TestNamedLocationClient (14.60s)
PASS
ok      github.com/manicminer/hamilton/clients  14.604s

Not sure why the linting is failing as that passes on my machine...
Let me know what you think :)

[manicminer]

Hi @alexwilcox9, thanks for the latest changes, this is looking great!

Can you rename NamedLocationClient to its plural form NamedLocationsClient? I also have a few comments on the client and its tests. If we can get these looked at then I think this will be ready to merge :)

[manicminer]

@alexwilcox9 Thanks for the changes! I resolved the merge conflict and pluralized the client name - this LGTM, let's get it merged :D

[alexwilcox9]

@manicminer Thanks for that! I got so focused on all the other points I totally missed the first one on pluralising!
Cheers for all the help on this

[manicminer]

No worries, thanks for all your work! :)