Okta: Add disableAssignDefaultRoles to plugin sync settings

[kopiczko]

Issue https://github.com/gravitational/teleport.e/issues/6533

Requires #54924 (so we don't have a mess with the fields order)

Related E changes https://github.com/gravitational/teleport.e/pull/6537

It also enables Access Requests to Okta-originated resources when only App and Group sync is enabled (no Access List sync) a0910a3

Backports:

• v18 not needed, merged before release (link)
• [v17] Okta: Add disableAssignDefaultRoles to plugin sync settings #55169

[r0mant]

@smallinsky and I discussed this yesterday and instead of having a boolean flag to disable the role we instead decided that having a way to provide default role for the integration (which would default to okta-requester) would be a better UX/design. This way users can override it to their own crafted role if they choose to. It looks like this PR still implements the "disable" flag.

[kopiczko]

@r0mant

Just to make things clear: the primary goal of this work is not to allow replace the default okta-requester role assignment but rather being able to disable the default okta-requester role assignment altogether. This is to allow the situation that was possible using the legacy okta_service with SAML connector role mapping without the User Sync enabled.

I tried to convert this to a string slice (defaultRoles []string), but because of backward-compatibility reasons we can't treat an empty list of roles as no roles. It can mean 2 things: either ["okta-requester"] for exiting plugins or [] for newly created plugins. So there are solutions I can think of:

1. Introducing PluginV2.

This sounds like too much effort for such a simple feature and we wouldn't be able to deliver in a reasonable time.

2. Using a boxed type like spec.okta.syncSettings.defaultRoles.value=[...] (instead of expected spec.okta.syncSettings.defaultRoles). And then rely on certain JSON rendering and checking for nil

I don't like this for 2 reasons:

• we need to have this .value thing and it makes IaC tools UX significantly worse
• and more importantly relying on proper JSON rendering and unmarshaling doesn't seem very robust

3. Requiring defaultRoles []string to always be not empty when set, and defaulting the empty slice to ["okta-requester"].

This seems ok but:

• it is not what the current customer needs, that would provide a workaround of being able to set a dummy default role, but from what I understand, what they need is just to disable it
• this would require a significant extra work on the SAML connector evaluation side where we have fixed preserved roles (#4995) because currently the plugin resource is not cached

4. (my preference) Introducing a bool value disableDefaultRoles bool as a first stage (this PR) and then adding defaultRoles []string if ever needed.

This has several advantages:

• addresses the customer's problem without workarounds
• backward-compatibility is solid and doesn't rely on JSON rendering and unmarshaling
• the IaC tool UX is not terrible if we want to create a TF/K8s resource
• tctl is also not bad and can evolve without any deprecations
• we can come up with some more advanced/tailored design instead of a simple list of static roles which btw is currently possible with both the SAML connector and Access Lists
• there is a flow where we create SAML connector if it doesn't exist during the integration setup - in this case it's easier to always have non-empty defaultRoles because SAML connector requires at least one role mapping (same applies to solution 3.)

Potential tctl evolution as the flags get added:

// for now:

tctl plugins install okta --[no-]assign-default-roles // defaults to true

// potentially in the future (after introducing defaultRoles slice):

tctl plugins install okta -h
      --[no-]assign-default-roles  Enables/disables default roles assignment. Defaults to true.
      --default-roles              Default roles roles to be assigned to all synchronized users. Defaults to ["okta-requester"].

// typical usage:

tctl plugins install okta --no-assign-default-roles          // no roles assigned
tctl plugins install okta --default-roles=my_role1,my_role2  // overwrite for okta-requester

// error for empty default roles:

$ tctl plugins install okta --default-roles=""
error: default roles must not be empty, if you want to disable default roles assignment, use --no-assign-default-roles

---

So for now I went with the first part of 4. and renamed the bool flag to disableAssignDefaultRoles with a potential of seamless extension.

Please LMK what you think.

---

EDIT: Another alternative that came to mind. Personally I don't feel it's much better than 4. if at all because it cements the simplistic design, but it allows to have defaultRoles in gRPC requests and tctl, but will still require a bool in the plugin spec.

message CreateIntegraitonRequest {
  // Default roles to assign to all synced users. Currently be either [] or
  // ["okta-requester"].
  repeated string default_roles = 100;
}

message UpdateIntegraitonRequest {
  // Default roles to assign to all synced users. Currently be either [] or
  // ["okta-requester"].
  repeated string default_roles = 100;
}

message PluginOktaSyncSettings {
  // DisableAssignDefaultRoles allows to disable okta-requster role assigments
  // to all synced users.
  bool disble_assign_default_roles = 100;

  //// ** POSSIBLY IN FUTURE: **
  //// // Default roles to assign to all synced users. It must contain at least
  //// // 1 role. Defaults to ["okta-requester"]. Default roles assignment can 
  //// // be disabled with DisableAssignDefaultRoles.
  //// repeated string default_roles = 101;
}

$ tctl plugins install okta --default-roles=""                // disables
$ tctl plugins install okta --default-roles="okta-requester"  // default
$ tctl plugins install okta --default-roles="role1,role2"     // error, until `PluginOktaSyncSettings.default_roles` is introduced

[kopiczko]

Updated the comment above.

[backport-bot-workflows]

@kopiczko See the table below for backport results.

Branch	Result
branch/v17	Failed

[backport-bot-workflows]

@kopiczko See the table below for backport results.

Branch	Result
branch/v17	Failed