An alternative to this is adding a dependency like iso3166 or django-countries.

Since ISO 3166 changes infrequently, I erred on the side of fewer dependencies.

I don't mind adding a dependency for this, especially since we should add gettext calls to all these humanized names, and some dependency might have full translation for all the country names already

Added django-countries. I'm unfamiliar with drf-spectacular and needed a bit of dark magic to make it work. If there's an easier way, let me know.

The use of an async function here for a static list of 2 is probably the worst part of this PR. I'll look to improve this.

Yeah, search select is specifically for complex objects that have subtitles and complex interactions. Consider using a Switch for a boolean choice, or if the ergonomics are bad (it's sometimes hard to word), a Radio.

Defaults should be part of the back-end. Also some botched types here.

Yeah, I'm looking at this and thinking that I'm seeing a lot of mixed messages compared to the types generated from the schema. You shouldn't have to be splitting anything; the API should be giving those to you, and the TypeScript definitions in GeoIPPolicy.ts say getting strings where arrays are expected shouldn't be possible). This function is probably the wrong place to be doing this.

If you're crafting a blob to send, start with a default and overwrite it with what you get from the form instead.

Yeah, I'm looking at this and thinking that I'm seeing a lot of mixed messages compared to the types generated from the schema. You shouldn't have to be splitting anything; the API should be giving those to you, and the TypeScript definitions in GeoIPPolicy.ts say getting strings where arrays are expected shouldn't be possible). This function is probably the wrong place to be doing this.

If you're crafting a blob to send, start with a default and overwrite it with what you get from the form instead.

?required=${true} isn't needed here (and I really need to break @BeryJu of the habit). required is all that's needed. You only need the ? syntax for boolean attributes if the attribute can be changed by client code, for example:
?hidden=${!this.open}

Yeah we should probably just rip off the bandaid and change all of those wrong attributes to the correct ones

first is a leftover from earlier iterations of TypeScript. Please use the idiom:
${this.instance?.executionLogging ?? false}

Although in this case I suspect you could just use the boolean forcing expression: ${!!this.instance?.executionLogging}

Yeah, search select is specifically for complex objects that have subtitles and complex interactions. Consider using a Switch for a boolean choice, or if the ergonomics are bad (it's sometimes hard to word), a Radio.

... and given that you've done it twice, break it out into a variable or something so you'll only have to change it once if the rules change. :-)

Since you've provided a not-undefined value for falsity, ifDefined(this.instance?.countryList || "") will always be defined. You can replace that whole expression with this.instance?.countryList ?? ""

both of these cases can also happen if ASN/GeoIP is not configured (pretty rare since we include the DB by default, but it is possible to disable)

(We've had a bit of an offline discussion, but no resolution to this.) I kept the behavior for now, this way we don't leak information on whether a GeoIP database is available.

Let me know if you don't agree.

Usually we don't have this as a setting directly on the policy but instead use the negate flag on the policy binding to inverse the effects. For this case I'm a bit on the fence as you can do two different things in the same policy with it

Oh, I didn't notice that flag! That makes everything so much simpler. I'm not on the fence about it at all 😄

Then there's no need for *_mode and this model will just have 2 fields: asn_list and country_list. Same as Event Matcher Policy for least astonishment:

If any of the configured values match, the policy passes.

This is somewhat of a downside of the generated API client, but this value here should never be undefined so this if shouldn't be required

Combined with the check below (which is still needed because "".split(",") is non-empty)

I don't mind adding a dependency for this, especially since we should add gettext calls to all these humanized names, and some dependency might have full translation for all the country names already

This is an exact copy of authentik/policies/geoip/views:11-14. Weirdly, importing it from there results in gen-build silently failing to generate this part of the schema. I'm out of my depth here, so I just duplicated these 4 lines.

That is very odd but also doesn't surprise me

There may or may not be a better place for this.

@BeryJu?

I'd usually prefer to do it as policies/geoip/iso3166/, but obviously that could potentially cause errors due to that path also having the primary key...as a fallback I'd go with policies/geoip_iso3166/ I guess?

The actual type of data slightly differs from GeoIPPolicy.

I haven't found an easy way to reconcile them (the "proper" way is probably building a component on top of <input> to run a transforming function when its value is read), so I opted for the type assertion.

You could just write:

data.asns = (data.asns?.toString() ?? "") === "" ? [] : (data.asns as unknown as string).split(",").map(Number);

... but it feels like the toString() thing is a crutch you're using, and I'm not sure why.

This currently hits the API for no reason every time there's a change in search. I'll add an optimization.

Yeah this is due to the assumption that the search filtering is done in the backend and not that the endpoint will return everything

You can write a provider that caches the result, and then provides the (locally reduces) results locally. Or you could look at ak-dual-select, which doesn't use the API; you could fetch the results once and then use the Web Component directly without having to go through the Element's API layer.

Yeah this is due to the assumption that the search filtering is done in the backend and not that the endpoint will return everything

Yeah we should probably just rip off the bandaid and change all of those wrong attributes to the correct ones

That is very odd but also doesn't surprise me

I'd usually prefer to do it as policies/geoip/iso3166/, but obviously that could potentially cause errors due to that path also having the primary key...as a fallback I'd go with policies/geoip_iso3166/ I guess?

Is there an example where we can use both "country" and "continent"? Will most people know the parms? (I wouldn't have know that continent was an option.)

It's possible, e.g. countries in Eurovision famously include Australia 🙂

    return context["geoip"]["continent"] == "EU" or context["geoip"]["country"] == "AU"

but I believe this might be more confusing than just keeping it simple. The params are listed right above, so I don't think "new confusion" arises from changing this.

(I changed this just to suggest something other that what could be done with a GeoIP policy.)

Hmmm I see that all of the other headers/titles for policies also capitalize "Policy". Our style is to use sentence style capitalization for headings... do you mind also changing all of the other headings to on this age to use a lower-case "p" on "policies"? :-)