Skip to content

Conversation

@paul-tavares
Copy link
Contributor

@paul-tavares paul-tavares commented Sep 9, 2020

Summary

Adds the ability to create a new Trusted App entry to the Trusted Apps List page. Includes some basic validations, but not a comprehensive set (that will be coming in a subsequent PR).

emt-691-create-trusted-app-pr

Checklist

@paul-tavares paul-tavares added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.1.0 Team:Endpoint Management Feature:Endpoint Elastic Endpoint feature labels Sep 9, 2020
@paul-tavares paul-tavares self-assigned this Sep 9, 2020
| MacosLinuxConditionEntry
| (Omit<MacosLinuxConditionEntry, 'field'> & {
field: 'signer';
field: 'porcess.code_signature';
Copy link
Contributor

Choose a reason for hiding this comment

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

wait is this actually called porcess?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, according to the info. I got :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry - Just got what you were implying 😄
It's a spelling mistake. Will correct it. Thanks for looking @parkiino

}

/** Store State when an API request has been sent to create a new trusted app entry */
export interface TrustedAppCreatePending {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if there's a way to group these states similar to the List resource. I know it's not entirely the same since we're creating a resource as opposed to loading it.


const generateNewEntry = (): NewTrustedApp['entries'][0] => {
return {
field: 'process.hash.*',
Copy link
Contributor

Choose a reason for hiding this comment

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

these are the field defaults?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this ultimately drive the default entry for a condition. I know this is not matching what @bfishel has in the Mocks, but for now it is the easier path to take in order to not over-complicate the types of data (I would have to add '' or null' to just about every field here). I plan to discuss that with Bonnie at some point.

isValid: boolean;
errors: Partial<{ [k in keyof NewTrustedApp]: string }>;
}
const validateFormValues = (values: NewTrustedApp): ValidationResult => {
Copy link
Contributor

Choose a reason for hiding this comment

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

we've talked about it a couple times in meetings, another validation we could do is field name uniqueness, for instance, you can only have one hash field, etc. Maybe this could solved when we call generateNewEntry() - enforce there that fields must be unique, generate the next "available field" and when none left, disable the "AND" button.

Not required for this PR, was just putting ideas out there for more validations.

FYI @charlie-pichette @efreeti

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 opened a separate issue to track UI/UX around validation feedback. This method here is bound to change based on that feedback

Copy link
Contributor

@kevinlog kevinlog left a comment

Choose a reason for hiding this comment

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

LGTM, some smaller comments. Would like @efreeti thoughts as well

| TrustedAppsListPageState['createView']
| Immutable<TrustedAppsListPageState['createView']>;

export const isTrustedAppCreatePendingState = (
Copy link
Contributor

Choose a reason for hiding this comment

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

is this related to what bohdan was talking about before about having selectors return types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes :)

Type guards are a runtime (executable code) function whose purpose is to ensure we are working with the correct type. In TS, it helps tell it that if this function evaluates to true, then the value being evaluated (data) is of a given type (note the : data is TrustedAppCreateSuccess in the return value for this function's signature.

@parkiino
Copy link
Contributor

is the spacing expected when you have multiple fields, and are you even allowed to have more than 1 hash?
image

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
securitySolution 1991 +8 1983

async chunks size

id value diff baseline
securitySolution 10.1MB +72.6KB 10.1MB

page load bundle size

id value diff baseline
securitySolution 793.4KB +171.0B 793.3KB

History

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

@paul-tavares
Copy link
Contributor Author

@parkiino I was able to reproduce that misalignment in the screen capture, but only when I used the browser's zoom and increase it. Seems to be caused by label (Operator) on that first condition row. I think this is mostly the because you will get from Flex-Box, being that we are not using a "true" table to display this UI.

Thanks for the review

@paul-tavares paul-tavares merged commit c178c8a into elastic:master Sep 16, 2020
@paul-tavares paul-tavares deleted the task/EMT-691-trusted-apps-ui-create branch September 16, 2020 20:21
paul-tavares added a commit to paul-tavares/kibana that referenced this pull request Sep 16, 2020
)

* Add UI for creating a new Trusted App entry
paul-tavares added a commit that referenced this pull request Sep 17, 2020
…) (#77675)

* [SECURITY_SOLUTION][ENDPOINT] Create Trusted Apps UI Flow (#77076)

* Add UI for creating a new Trusted App entry

* update invalid snapshot

(cherry picked from commit 333b200)

Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: spalger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature:Endpoint Elastic Endpoint feature release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants