✨ Provide CWE Selector #523

C-Valen · 2025-01-13T11:11:19Z

OSIDB-3743 CWE Selector UI

Checklist:

Commits consolidated
Changelog updated
Test cases added/updated
Jira ticket updated

Summary:

Update for the OSIM UI to provide a CWE Selector using suggestions to help users.
Corrections in the Mitre service logic

Changes:

Corrects CweService.ts to use the proper data for weaknesses
Adds new CweSelector component to be used in the flaw form
Adds keyboard navigation to the dropdown menu
Adds new test cases and update snapshots

Considerations:

Depends on ✨ OSIDB-3480: CWE api service implementation #521

Closes OSIDB-3743

MrMarble

We need to fix the event handler, other comments are minor changes

MrMarble · 2025-01-27T08:41:23Z

src/components/CweSelector/CweSelector.vue

+
+onMounted(() => {
+  loadCweData();
+  window.addEventListener('keydown', handleKeyDown);


Why add the event to the window instead of the DOM node? just use @keydown on the element like we do with @click.

Also if we add an eventListener manually we need to take care of removing it, since it is added to the window the event is not destroyed even if the component is unmounted, so the event is always listening, this is a memory leak, also, each time the component is mounted a new event is added without removing the old one, so it will trigger multiple times.

See this playground as an example https://play.vuejs.org/

In case this is needed we could remove it like this:

onUnmounted(() => { window.removeEventListener('keydown', handleKeyDown); });

Because the event is on window even if I'm not focus on it, it will trigger the handler

Screencast.From.2025-01-27.10-12-13.mp4

MrMarble · 2025-01-27T08:46:30Z

src/components/CweSelector/CweSelector.vue

+import type { CWEMemberType } from '@/types/mitreCwe';
+
+const props = withDefaults(defineProps<{
+  error?: null | string;


I know we do this everywhere, but it is needed? ZodError won't be null as far as I know, is either defined or undefined

Suggested change

error?: null | string;

error?: string;

MrMarble · 2025-01-27T08:48:14Z

src/components/CweSelector/CweSelector.vue

+const selectedIndex = ref(-1);
+
+const loadCweData = () => {
+  const data = localStorage.getItem('CWE:API-DATA');


We should export the constant in the service and use it instead of this magic string

osim/src/services/CweService.ts

Line 4 in 7fd876d

const DATA_KEY = 'CWE:API-DATA';

MrMarble · 2025-01-27T08:57:32Z

src/components/CweSelector/CweSelector.vue

+          class="item gap-1"
+          :class="{'selected': index === selectedIndex }"
+          style="display: flex; justify-content: space-between;"


We could use bootstrap classes for this

Suggested change

class="item gap-1"

:class="{'selected': index === selectedIndex }"

style="display: flex; justify-content: space-between;"

class="item gap-1 d-flex justify-content-between"

:class="{'selected': index === selectedIndex }"

MrMarble · 2025-01-27T09:10:23Z

src/components/CweSelector/CweSelector.vue

+    >
+      <template v-if="suggestions.length > 0" #suggestions="{ abort }">
+        <div class="dropdown-header">
+          <span style="flex: 1;">ID</span>


I don't know if we should use thestyle attribute when we also have custom css.
Bootstrap provides flex-grow-X and flex-shrink-X classes, but they are a bit verbose, maybe we could use a class flex-1 and flex-3 and use that instead, that way if the style needs to change only the class needs to be updated and not each component individually

MrMarble · 2025-01-27T09:39:50Z

src/components/CweSelector/CweSelector.vue

+  <LabelDiv :label="props.label" class="mb-2">
+    <EditableTextWithSuggestions
+      v-model="modelValue"
+      :error="props.error"


Same here, no need for props.

Suggested change

:error="props.error"

:error

MrMarble · 2025-01-27T09:42:58Z

src/components/__tests__/CweSelector.spec.ts

All the tests make use of the vm attribute to access internal state, but none of them tests actual component behavior, the functions may work but how do we know they are triggered when a user clicks something or enter text in the searchbox?

Agreed, I have been trying to pay closer attention to the tests I write testing the UI and not the business logic of a component. That said, composables make sense to test for business logic though.

MrMarble · 2025-01-27T09:48:19Z

src/components/__tests__/CweSelector.spec.ts

+import { mountWithConfig } from '@/__tests__/helpers';
+
+describe('cweSelector.vue', () => {
+  let wrapper: any;


Make it at least a VueWrapper so the exists, html and vm properties work.

Suggested change

let wrapper: any;

let wrapper: VueWrapper<any>;

MrMarble · 2025-01-27T09:48:44Z

src/components/__tests__/CweSelector.spec.ts

+      global: {
+        components: {
+          EditableTextWithSuggestions,
+          LabelDiv,
+        },
+      },


is this necessary? we are not mocking those components

MrMarble · 2025-01-27T09:54:44Z

src/services/CweService.ts

+  const cweView = await fetchCweView(baseUrl);
+  const cweCategories = await fetchCweCategories(baseUrl, cweView);
+  const cweWeaknesses = await fetchCweWeaknesses(baseUrl, cweCategories);
+  localStorage.setItem(DATA_KEY, JSON.stringify(cweWeaknesses));


If the user already has the CWE:API-DATA localStorage object, all these new properties will not exists until the version of mitre is updated, since we haven't published the cwe selector to prod I don't think we should handle it, just keep in mind to tell the users to clean the localStorage if they are testing this, as some of it could fail

Actually it fails, the getUsageClass from CweSelector fill throw an error because usage is undefined and the call to .toLowerCase() fails

MrMarble · 2025-02-04T14:30:11Z

src/components/CweSelector/CweSelector.vue

+          class="item gap-1 d-flex justify-content-between"
+          :class="{'selected': index === selectedIndex }"
+          style="justify-content: space-between;"


You added the class but forgot to remove the style

Suggested change

class="item gap-1 d-flex justify-content-between"

:class="{'selected': index === selectedIndex }"

style="justify-content: space-between;"

class="item gap-1 d-flex justify-content-between"

:class="{'selected': index === selectedIndex }"

superbuggy

Left some suggestions, and a couple of minor change requests

superbuggy · 2025-02-04T16:49:59Z

src/components/CweSelector/CweSelector.vue

+  label?: string;
+}>(), {
+  label: '',
+  error: undefined,


We're sort of split between usage of null and undefined in the codebase, but maybe undefined is better as a props default since the null values should come from the errors in useFlawModel

I think I like what you have here better than the default nulls we have elsewhere

superbuggy · 2025-02-04T16:50:49Z

src/components/CweSelector/CweSelector.vue

+const loadCweData = () => {
+  const data = localStorage.getItem(DATA_KEY);
+  if (data) {
+    cweData.value = JSON.parse(data);


Safest to wrap this in a try/catch block

src/components/CweSelector/CweSelector.vue

superbuggy · 2025-02-04T16:53:19Z

src/components/CweSelector/CweSelector.vue

+  modelValue.value = queryParts.join('');
+  queryRef.value = modelValue.value;
+  suggestions.value = [];
+  nextTick(fn);


Is this to defer/delay the click event until the query has been parsed into a suggestion?

Not exactly. That is to ensure the suggestions and query have been updated in the DOM before the optional callback function (fn) is processed.
For example in the scenario of triggering the abort action (pressing ESC) just after clicking a suggestion.

That makes sense, would you mind adding a comment to that effect (waiting for 'abort'/'commit' events)?

superbuggy · 2025-02-04T16:54:09Z

src/components/CweSelector/CweSelector.vue

+onMounted(() => {
+  loadCweData();
+});


Suggested change

onMounted(() => {

loadCweData();

});

onMounted(loadCweData);

superbuggy · 2025-02-04T16:55:49Z

src/components/CweSelector/CweSelector.vue

+          <span class="flex-3">{{ `${cwe.name}. ` }}</span>
+          <span class="badge flex-1" :class="getUsageClass(cwe.usage)">{{ `${cwe.usage}` }}</span>
+          <div class="flex-1">
+            <i v-show="cwe.summary != ''" class="icon bi-info-circle" :title="cwe.summary" />


Suggested change

<i v-show="cwe.summary != ''" class="icon bi-info-circle" :title="cwe.summary" />

<i v-show="cwe.summary !== ''" class="icon bi-info-circle" :title="cwe.summary" />

unless this is what you want, then I think this would be equivalent:

Suggested change

<i v-show="cwe.summary != ''" class="icon bi-info-circle" :title="cwe.summary" />

<i v-show="cwe.summary" class="icon bi-info-circle" :title="cwe.summary" />

superbuggy · 2025-02-04T18:31:38Z

src/components/__tests__/CweSelector.spec.ts

+  it('loads data from localStorage', () => {
+    const data = JSON.stringify([{ id: '123', name: 'Test CWE', status: 'Draft', summary: '', usage: '' }]);
+    localStorage.setItem(DATA_KEY, data);
+    wrapper.vm.loadCweData();


I wonder if it would be better to mock loadCweData here or use a localstorage mock, then test something in the UI showing up as a result, otherwise we're effectively testing Vue internals and JavaScript LocalStorage

C-Valen self-assigned this Jan 13, 2025

C-Valen force-pushed the feature/OSIDB-3743-cwe-selector-ui branch 4 times, most recently from 50bf572 to 2c14d8c Compare January 15, 2025 13:13

C-Valen added the enhancement New feature or request label Jan 15, 2025

C-Valen requested a review from a team January 15, 2025 13:24

C-Valen marked this pull request as ready for review January 15, 2025 13:25

C-Valen marked this pull request as draft January 15, 2025 13:26

C-Valen force-pushed the feature/OSIDB-3743-cwe-selector-ui branch 3 times, most recently from 800da99 to 4340a62 Compare January 15, 2025 15:50

C-Valen marked this pull request as ready for review January 16, 2025 09:45

C-Valen force-pushed the feature/OSIDB-3743-cwe-selector-ui branch 3 times, most recently from 49c5269 to 7fd876d Compare January 24, 2025 15:40

MrMarble requested changes Jan 27, 2025

View reviewed changes

🏷️ Add Zod CWE Mitre types

7452362

C-Valen force-pushed the feature/OSIDB-3743-cwe-selector-ui branch from 7fd876d to fd43cd4 Compare January 30, 2025 11:55

🐛 Correct CWE Service logic

26e6826

C-Valen force-pushed the feature/OSIDB-3743-cwe-selector-ui branch from fd43cd4 to a200533 Compare January 30, 2025 11:56

C-Valen requested review from MrMarble and a team and removed request for MrMarble January 30, 2025 13:24

MrMarble reviewed Feb 4, 2025

View reviewed changes

superbuggy requested changes Feb 4, 2025

View reviewed changes

superbuggy reviewed Feb 4, 2025

View reviewed changes

C-Valen force-pushed the feature/OSIDB-3743-cwe-selector-ui branch from a200533 to 8099f61 Compare February 5, 2025 11:01

C-Valen requested a review from superbuggy February 5, 2025 11:01

C-Valen requested a review from MrMarble February 5, 2025 11:01

C-Valen force-pushed the feature/OSIDB-3743-cwe-selector-ui branch from 8099f61 to a94b0e9 Compare February 6, 2025 11:15

C-Valen added 3 commits February 6, 2025 15:34

✨ Provide CWE Selector

a71bb84

✅ Update tests

8861856

📝 Changelog OSIDB-3743 update

d51bea1

C-Valen force-pushed the feature/OSIDB-3743-cwe-selector-ui branch from a94b0e9 to d51bea1 Compare February 6, 2025 14:36

MrMarble approved these changes Feb 6, 2025

View reviewed changes

superbuggy approved these changes Feb 6, 2025

View reviewed changes

C-Valen merged commit 424ea9a into main Feb 6, 2025
5 checks passed

C-Valen deleted the feature/OSIDB-3743-cwe-selector-ui branch February 6, 2025 14:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Provide CWE Selector #523

✨ Provide CWE Selector #523

C-Valen commented Jan 13, 2025 •

edited

Loading

MrMarble left a comment

MrMarble Jan 27, 2025

MrMarble Jan 27, 2025 •

edited

Loading

MrMarble Jan 27, 2025

MrMarble Jan 27, 2025

MrMarble Jan 27, 2025

MrMarble Jan 27, 2025

MrMarble Jan 27, 2025

MrMarble Jan 27, 2025

superbuggy Feb 4, 2025

MrMarble Jan 27, 2025

MrMarble Jan 27, 2025

MrMarble Jan 27, 2025

MrMarble Feb 4, 2025

superbuggy left a comment

superbuggy Feb 4, 2025

superbuggy Feb 4, 2025

superbuggy Feb 4, 2025

superbuggy Feb 4, 2025

C-Valen Feb 5, 2025

superbuggy Feb 6, 2025

superbuggy Feb 4, 2025

superbuggy Feb 4, 2025

superbuggy Feb 4, 2025 •

edited

Loading

	<i v-show="cwe.summary != ''" class="icon bi-info-circle" :title="cwe.summary" />
	<i v-show="cwe.summary !== ''" class="icon bi-info-circle" :title="cwe.summary" />

	<i v-show="cwe.summary != ''" class="icon bi-info-circle" :title="cwe.summary" />
	<i v-show="cwe.summary" class="icon bi-info-circle" :title="cwe.summary" />

✨ Provide CWE Selector #523

✨ Provide CWE Selector #523

Conversation

C-Valen commented Jan 13, 2025 • edited Loading

OSIDB-3743 CWE Selector UI

Checklist:

Summary:

Changes:

Considerations:

MrMarble left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrMarble Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superbuggy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superbuggy Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

C-Valen commented Jan 13, 2025 •

edited

Loading

MrMarble Jan 27, 2025 •

edited

Loading

superbuggy Feb 4, 2025 •

edited

Loading