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

[RAM] Add the feature for slack api to have allowed list on channels #159534

Merged
merged 16 commits into from Jun 21, 2023

Conversation

XavierM
Copy link
Contributor

@XavierM XavierM commented Jun 13, 2023

Summary

This will enable our user to create a slack api connector with the ability to only allow some channels as an allowed list. Our user will only be able to edit this input if the secrets is enter like the test button work.

image

Checklist

Delete any items that are not applicable to this PR.

@XavierM XavierM added Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Rule Management Security Solution Detection Rule Management v8.9.0 labels Jun 13, 2023
@lcawl lcawl added the ui-copy Review of UI copy with docs team is recommended label Jun 13, 2023
@XavierM XavierM marked this pull request as ready for review June 15, 2023 21:42
@XavierM XavierM requested a review from a team as a code owner June 15, 2023 21:42
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

Comment on lines +132 to +138
const debounceSetToken = debounce(setAuthToken, INPUT_TIMEOUT);
useEffect(() => {
if (formData.secrets && formData.secrets.token !== authToken) {
debounceSetToken(formData.secrets.token);
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [formData.secrets]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need the useEffect here. You can do:

const authToken = formData.secrets && formData.secrets.token ? formData.secrets.token : ''
const debounceSetToken = debounce(setAuthToken, INPUT_TIMEOUT);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do believe that you need the useEffect, if the user decide to empty the token or not and also the way the form lib is getting initialized

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I miss something but if the user decides to empty the token it will still run the code, right? I don't see any difference between having the useEffect and not having it in this code. Probably I miss something.


return (
<SimpleConnectorForm
isEdit={isEdit}
readOnly={readOnly}
configFormSchema={[]}
secretsFormSchema={getSecretsFormSchema(docLinks)}
configFormSchemaAfterSecrets={configFormSchemaAfterSecrets}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get the reasoning behind it (given also time constraints) but I think the slack connector should have its own customized component and not use the SimpleConnectorForm. The reason is not the position of the configs after the secrets but mostly that the secrets are connected with these configs. We should disable the channels combo box until the user has filled out the secrets and provided a helpful message why they are disabled and what the user should do to be able to config the allowed channels. In Swimlane (more complex), we use steps where in step 1 you put the URL and the password and in step 2 you configure the rest of the options (I am not proposing this complex UI for the slack connector).

Copy link
Member

@cnasikas cnasikas Jun 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I just see what you did with configFormSchemaAfterSecrets. You connected them with getConfigFormSchemaAfterSecrets. Makes sense.

id: 'allowedChannels',
isRequired: false,
label: i18n.ALLOWED_CHANNELS,
helpText: (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can add a message explaining why the combo box is disabled if the user has not filled out the secrets.

}

const configFormSchemaAfterSecrets = useMemo(
() => getConfigFormSchemaAfterSecrets(channels, isLoading, channels.length === 0),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should disable it formData.secrets.token is undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that will be taking care when the channels is empty

@@ -32,35 +39,45 @@ interface SimpleConnectorFormProps {
readOnly: boolean;
configFormSchema: ConfigFieldSchema[];
secretsFormSchema: SecretsFieldSchema[];
configFormSchemaAfterSecrets?: ConfigFieldSchema[];
Copy link
Member

@cnasikas cnasikas Jun 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another solution would be to extend ConfigFieldSchema with a afterSecrets?: boolean attribute and do the partition in the SimpleConnectorFormComponent. This way consumers will not have to create two configurations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might need both, I do not know!

Copy link
Contributor

@JiaweiWu JiaweiWu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Great to see react query being used.

Just have 1 small nitpick regarding naming

import { INTERNAL_BASE_STACK_CONNECTORS_API_PATH } from '../../../common';
import * as i18n from './translations';

interface UseLoadTagsQueryProps {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: this should be UseFetchChannelsProps

@XavierM XavierM enabled auto-merge (squash) June 21, 2023 01:42
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
stackConnectors 203 204 +1

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
actions 261 262 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
stackConnectors 440.0KB 440.7KB +630.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
stackConnectors 34.7KB 35.2KB +535.0B
triggersActionsUi 86.0KB 86.5KB +565.0B
total +1.1KB
Unknown metric groups

API count

id before after diff
actions 266 267 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 13 15 +2
securitySolution 411 415 +4
stackConnectors 90 92 +2
total +8

Total ESLint disabled count

id before after diff
enterpriseSearch 14 16 +2
securitySolution 492 496 +4
stackConnectors 94 96 +2
total +8

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@XavierM XavierM merged commit 1149fe4 into elastic:main Jun 21, 2023
21 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Rule Management Security Solution Detection Rule Management release_note:enhancement Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) ui-copy Review of UI copy with docs team is recommended v8.9.0 v8.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants