[Actions] Expose config value for preconfigured connectors for GET all api/actions/connectors

[YulNaumenko]

Before we didn't have a requirement for returning config values for getAll actions API ofr preconfigured connectors, because we didn't use it in the UI.
Currently we have a problem with identifying deprecated ServiceNow connectors if it is preconfigured, because it is possible only if we have config values: usesTableApi.
This functionality could be a blocker for this PR #115287

config: {
  apiUrl: "https://testservicenow"
  hasAuth: true
  usesTableApi: true
}
connector_type_id: ".servicenow"
id: "my-servicenow"
is_preconfigured: true
name: "preconfigured-servicenow-itsm-connector-type"
referenced_by_count: 0

[mikecote]

I believe we wanted to keep the config values internal in case system administrators did not want to expose them to the users. Unfortunately, it was an assumption from the beginning and we should ask input from @arisonl on whether or not we should continue avoiding exposing config for preconfigured connectors.

[pmuellr]

PeterS was in favor of hiding config for pre-configured connectors. Personally, I always felt like we overloaded pre-configured with an aspect of "hidden" - feels like "pre-configured" and "config is hidden" should be separate things. But I guess "config is hidden" wouldn't be useful outside of pre-configured connectors ...

What is the case for needing this info though ...

we have a problem with identifying deprecated ServiceNow connectors

Where do we do this? Is it when determining if we need to display the "warning" icon in the connector list? I'm wondering if we could add an API somewhere, that could have access to the fields, without exposing them in the GET calls ...

[jonathan-buttner]

@pmuellr

Where do we do this? Is it when determining if we need to display the "warning" icon in the connector list?

Yep, we access the connector's config property to determine if we should display the warning icon and deprecated text. For now, we're just going to not display anything for pre-configured connectors.

Yuliia also found a bug within Cases that was causing the UI to crash because we thought the config field would always be defined: #120686

To guard against that I'm forcing the type within Cases to have config be optional: https://github.com/elastic/kibana/pull/120686/files#diff-066952992e28b242a1457d5286ecf65182dc14ad9508686095f5fb4a549e58f9R26

I'm happy to do something different though. Would it be better to mark ActionResult to have config as optional?

cc: @cnasikas

[pmuellr]

Yep, we access the connector's config property to determine if we should display the warning icon and deprecated text. For now, we're just going to not display anything for pre-configured connectors.

If we really want to keep pre-configured config hidden, then I think we'd need to provide an API that can take an actionId and return whether it should have the "warning" icon displayed. We'd probably want to make it more general, getRuleExtraInfo(id: string): Promise<RuleExtraInfo> kinda thing.

Yuliia also found a bug within Cases that was causing the UI to crash because we thought the config field would always be defined: #120686

To guard against that I'm forcing the type within Cases to have config be optional: https://github.com/elastic/kibana/pull/120686/files#diff-066952992e28b242a1457d5286ecf65182dc14ad9508686095f5fb4a549e58f9R26

I'm happy to do something different though. Would it be better to mark ActionResult to have config as optional?

Clearly that's a bug on our part. Thanks for the PR! Would it be simpler to return an empty object instead of null/undefined in that case? Sometimes optional properties are a PITA to deal with. OTOH, it could be kind nice to leave null/undefined to mean "you're not allowed to see it", because otherwise you'd need to check the isPreconfigured. Six one, half dozen the other?

[jonathan-buttner]

Thanks for the PR!

Yeah no worries!

Would it be simpler to return an empty object instead of null/undefined in that case? Sometimes optional properties are a PITA to deal with. OTOH, it could be kind nice to leave null/undefined to mean "you're not allowed to see it", because otherwise you'd need to check the isPreconfigured. Six one, half dozen the other?

Either sounds good to me, the change to check for an undefined config wasn't too bad in Cases, I'm not sure if that'd cause a lot of cascading changes in the actions plugin if we changed it in ActionResult. I think adding a comment in the interface explaining why config could be optional or empty when it's a pre-configured connector would be helpful too.

What do you think @cnasikas ?

[cnasikas]

Same for me. Whatever is the easiest to change. Returning an empty object seems simpler to me.

Btw, I really like the idea of getRuleExtraInfo(id: string): Promise<RuleExtraInfo>. I think we should do it on cases also.

[gmmorris]

I've taken this on to unblock a customer ask.
We shouldn't expose the config on the API if we can avoid that (this is a bigger risk with preconfigured connectors due to the lack of space awareness and limited RBAC.

Instead I am moving the deprecation check into the server side.

[cnasikas]

Should we return an is_deprecated field at the same level as the is_preconfigured attribute? Connectors on registration can provide a isDeprecated: (config, secrets) => boolean function that the framework will call to determine the is_deprecated field.

[gmmorris]

Should we return an is_deprecated field at the same level as the is_preconfigured attribute?

Yup, that's exactly what I ended up doing: #130541

Connectors on registration can provide a isDeprecated: (config, secrets) => boolean function that the framework will call to determine the is_deprecated field.

That can be a follow up if you want. For now I've just moved the existing logic to the server.

[gmmorris]

@cnasikas - I'm going to merge the PR above into 8.3, so feel free to build on it.
Please note this question by @ymao1 as we might want to add that in a follow up PR?

[gmmorris]

Given the fix above I'm going to close this issue as not going to do, as we solved this from a different direction.