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

[backend/frontend] Add user and group confidence level (#4304) #5436

Merged
merged 12 commits into from
Jan 29, 2024

Conversation

labo-flg
Copy link
Member

@labo-flg labo-flg commented Jan 9, 2024

Proposed changes

This PR addresses the following issues:

Have a look at each issue description for details.

Code review highlights

The key points to review are backend additions and changes, the model definition.
A lot of stuff in the frontend besides the core job (refactor in TS, solving react warnings) make the review difficult, I'm sorry.

How can you test this PR?

Here are some scenarios to test:

  • start the platform on this branch -> you shall see the critical alert dialog, inviting you to edit all your groups

  • Create a user without groups -> they should be created with default groups

  • Create a user with groups -> they should be created with the specified groups, and not the default group

  • Create a user without a confidence level and no group

    • it should be displayed as in error in the various view (list, details, list of users in a group details view) -> icon + tooltip
    • edit the user : the message above the confidence level field shall be explicit (no group to inherit from)
    • edit the user, tab groups : the confidence of each group shall be displayed, with red icon and tooltip if not defined
    • edit the user : set a group in the groups tab, go back to overview -> the message above the confidence level field shall be explicit (inherited value)
    • edit the user : set the user's confidence level -> -> the message above the confidence level field shall be explicit (user value)
  • Create a user without a confidence level and no group -> it should be displayed as in error in the various view (list, details, list of users in a group details view)

  • Create a user with a confidence level -> it should be displayed in the various view (list, details, list of users in a group details view) -> tooltip shall state it's coming from user

  • Create a user without a confidence level but with a group -> it should be displayed in the various view (list, details, list of users in a group details view) -> tooltip shall state it's coming from a specific group - link clickable in tooltip

  • Create a group, you must select a confidence level -> value is then displayed correctly in list and details

  • Open a group without confidence level -> explicit alert in details -> disappears when edited

  • all groups are set with a confidence level -> refresh page -> no more alert dialog

If you wish to artificially nuke your group confidence level for testing purpose, a GQL query on playground is possible (there is no validation implemented yet - see #5696).

mutation GroupEditionOverviewFieldPatchMutation(
  $id: ID!
  $input: [EditInput]!
) {
  groupEdit(id: $id) {
    fieldPatch(input: $input) {
      id
      group_confidence_level {
        max_confidence
      }
    }
  }
}
{
  "id": "4b762ee9-b2da-49fe-99d7-dfd6b9e1fe33",
  "input": {
    "key": "group_confidence_level",
    "value": [null]
  }
}

Checklist

  • I consider the submitted work as finished
  • I tested the code for its functionality
  • I wrote test cases for the relevant uses case
  • I added/update the relevant documentation (either on github or on notion)
  • Where necessary I refactored code to improve the overall quality

Further comments

Now that the new schema definition is in place, the job done on the subject of user's confidence level has been fully reworked.
Thus this PR invalidates the previous PR on the subject: #5283 #5292 #5323

@labo-flg labo-flg marked this pull request as ready for review January 9, 2024 10:24
Copy link

codecov bot commented Jan 9, 2024

Codecov Report

Attention: 31 lines in your changes are missing coverage. Please review.

Comparison is base (61c0432) 66.12% compared to head (1344932) 66.15%.

Files Patch % Lines
...ti-platform/opencti-graphql/src/domain/settings.js 15.38% 22 Missing ⚠️
...pencti-platform/opencti-graphql/src/domain/user.js 91.42% 6 Missing ⚠️
...encti-platform/opencti-graphql/src/domain/grant.js 75.00% 2 Missing ⚠️
...cti-platform/opencti-graphql/src/resolvers/user.js 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5436      +/-   ##
==========================================
+ Coverage   66.12%   66.15%   +0.03%     
==========================================
  Files         513      513              
  Lines       60641    60793     +152     
  Branches     4434     4448      +14     
==========================================
+ Hits        40097    40218     +121     
- Misses      20544    20575      +31     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@labo-flg labo-flg changed the title Add user confidence level - over new schema definition (#4304) [backend/frontend] Add user confidence level - over new schema definition (#4304) Jan 9, 2024
@labo-flg labo-flg self-assigned this Jan 9, 2024
@labo-flg labo-flg added the filigran team use to identify PR from the Filigran team label Jan 9, 2024
@labo-flg
Copy link
Member Author

I'm able to send through the API a partial update like

{
  "id": "<id>",
  "input": {
  "key": "user_confidence_level",
    "value": {
    "max_confidence": 96
  }
}

Which will update the user_confidence_level with overrides to null, causing GQL error at query time as this field is supposedly never null.
I'll check the model, maybe around bad mandatoryType

@lndrtrbn lndrtrbn self-requested a review January 10, 2024 08:28
Copy link
Member

@lndrtrbn lndrtrbn left a comment

Choose a reason for hiding this comment

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

Seems good to me, but I'll let someone else have a look at migration and attributes registrations 😅

@labo-flg
Copy link
Member Author

Seems good to me, but I'll let someone else have a look at migration and attributes registrations 😅

Thanks !

@labo-flg labo-flg marked this pull request as draft January 12, 2024 08:46
@labo-flg
Copy link
Member Author

back to draft : I need to make sure partial edit payload are not accepted

@labo-flg
Copy link
Member Author

labo-flg commented Jan 16, 2024

We need to add schema validation when patching a field.
This is significant work that deserve a dedicated branch/PR.

Everything in this PR is ok, but we need this validation before merging to master.
I will use this branch as a feature branch, and merge into it the validation.

EDIT: validation will be done in another issue (#5696) , this branch will still serve as feature branch

@labo-flg labo-flg force-pushed the issue/4304-confidence-new-schema-def branch 2 times, most recently from 32b5a01 to 00ac6e9 Compare January 24, 2024 10:23
@labo-flg labo-flg changed the title [backend/frontend] Add user confidence level - over new schema definition (#4304) [backend/frontend] Add user and group confidence level (#4304) Jan 25, 2024
@labo-flg labo-flg force-pushed the issue/4304-confidence-new-schema-def branch from ce1c332 to 76e784f Compare January 25, 2024 16:11
@labo-flg labo-flg marked this pull request as ready for review January 25, 2024 16:52
Comment on lines -60 to +61
<Grid container={true} spacing={3} alignItems="end">
<Grid container={true} spacing={3} >
Copy link
Member Author

Choose a reason for hiding this comment

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

this align the 2 inputs (input number + scale Selector) to the top, keeping alignments when the input number shows an error label below.

<SettingsMessagesBanner />
<PlatformCriticalAlertDialog alerts={settings.platform_critical_alerts}/>
Copy link
Member Author

Choose a reason for hiding this comment

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

New popup dialog for admins when the backend has some "critical alerts" to show.
This is for warning about groups with null confidence levels, but could be extended to other types of alerts

Comment on lines +30 to +39
platform_critical_alerts {
message
type
details {
groups {
id
name
}
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

alert details shall depends on alert type... current implem is a bit rough on the edges but we had to move quickly about this.

Comment on lines 107 to 112
const max_confidence = n?.node.group_confidence_level
? `${t_i18n('Max Confidence Level:')} ${n.node.group_confidence_level.max_confidence}`
: t_i18n('No confidence level');
const newLabel = showConfidence
? `${n?.node.name} (${max_confidence})`
: n?.node.name ?? '';
Copy link
Member Author

Choose a reason for hiding this comment

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

old GroupField Select, refactored to typescript, and added opt prop showConfidence to add the group confidence to the label after the name of the group.

effective_confidence_level: {
label: 'confidence',
width: '10%',
isSortable: false,
Copy link
Member Author

Choose a reason for hiding this comment

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

effective confidence level is a computed value backend side, not an attribute in DB (depends on user's confidence + his groups confidence)

we cannot craft a query to do this computation, so we cannot paginate and sort properly this value.

Comment on lines +112 to +137
if (name === 'group_confidence_level') {
if (group.group_confidence_level) {
commitFieldPatch({
variables: {
id: group.id,
input: {
key: 'group_confidence_level',
object_path: '/group_confidence_level/max_confidence',
value: parseInt(value, 10),
},
},
});
} else {
commitFieldPatch({
variables: {
id: group.id,
input: {
key: 'group_confidence_level',
value: {
max_confidence: parseInt(value, 10),
overrides: [],
},
},
},
});
}
Copy link
Member Author

Choose a reason for hiding this comment

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

the else statement could work in both cases (existing confidence or not) right now, but we will implement soon the overrides definition and edition (6.1). By then we'll need to patch with object_path anyway.

Comment on lines +55 to +63
user_confidence_level_enabled: Yup.boolean(),
user_confidence_level: Yup.number()
.min(0, t('The value must be greater than or equal to 0'))
.max(100, t('The value must be less than or equal to 100'))
.when('user_confidence_level_enabled', {
is: true,
then: (schema) => schema.required(t('This field is required')).nullable(),
otherwise: (schema) => schema.nullable(),
}),
Copy link
Member Author

Choose a reason for hiding this comment

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

a technical field (not to submit) associated with a when. Very useful for validation conditioned on other values!

return computeUserEffectiveConfidenceLevel(user);
};

export const computeUserEffectiveConfidenceLevel = (user) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

function split for easy unit tests

Comment on lines +1072 to +1074
type PlatformCriticalAlertDetails {
groups: [Group!]!
}
Copy link
Member Author

Choose a reason for hiding this comment

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

this is where the critical alert system is a bit limited, not well generalized. details shall depends on type.

I did not have time to overengineer this system, honestly it's a quick bandaid to help admins prepare for 6.0.

Copy link
Member

Choose a reason for hiding this comment

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

Not a problem, thats purely dynamic and so easy to change.

@@ -1704,7 +1704,6 @@
"INGESTION_MANAGER": "Ingestion manager",
"PLAYBOOK_MANAGER": "Playbook manager",
"FILE_INDEX_MANAGER": "File index manager",
"INDICATOR_DECAY_MANAGER": "Indicator decay manager",
Copy link
Member

Choose a reason for hiding this comment

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

why has this translation been removed ?

Copy link
Member Author

Choose a reason for hiding this comment

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

In must have executed an extract script in my branch, pre-rebase.
I'll check and restore the missing keys.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've done a review, all keys not related to confidence level have been restored (there was a couple)

Copy link
Member

@aHenryJard aHenryJard left a comment

Choose a reason for hiding this comment

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

I just did a "quick read" review so I share it, I will take take time for a deeper one later.

const renderSource = () => {
const source = (confidenceLevel as Data_EffectiveConfidenceLevel)?.source;

// FIXME: if watching the current user's detailed view, the source is {}, hence the check if (source.entity_type && ...) below
Copy link
Member

Choose a reason for hiding this comment

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

FIXME later ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did not have time to investigate, but it's probably related to how we poorly handle the graphql cache records

Warning: RelayModernRecord: Invalid record update, expected both versions of record 88ec0c6a-13ce-5e39-b486-354fe4a7084f to have the same __typename but got conflicting types MeUser and User. The GraphQL server likely violated the globally unique id requirement by returning the same id for different objects.

Another whole issue to address, certainly not in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm editing myself, which collides with MeUser query for instance. The record gets "updated" wrongly somehow.

@labo-flg labo-flg force-pushed the issue/4304-confidence-new-schema-def branch from 9f0452f to 1344932 Compare January 29, 2024 08:29
@Kedae Kedae merged commit e3dd23b into master Jan 29, 2024
8 checks passed
@labo-flg labo-flg deleted the issue/4304-confidence-new-schema-def branch January 30, 2024 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
filigran team use to identify PR from the Filigran team
Projects
None yet
7 participants