[backend/frontend] Fix enable references behavior when adding entities to containers #6175

JeremyCloarec · 2024-02-29T11:09:52Z

This PR is only to fix enable references behavior in the scope of the #5884 issue. Other PRs will aim to fully fix the enable references behavior for all entities, and the full list of problems can be seen in the #6074

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

codecov · 2024-02-29T13:58:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.75%. Comparing base (0067e4d) to head (949e85d).
Report is 1 commits behind head on master.

❗ Current head 949e85d differs from pull request most recent head cba58bc. Consider uploading reports for the commit cba58bc to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6175      +/-   ##
==========================================
- Coverage   67.76%   66.75%   -1.02%     
==========================================
  Files         532      541       +9     
  Lines       65065    64556     -509     
  Branches     5462     5307     -155     
==========================================
- Hits        44093    43094     -999     
- Misses      20972    21462     +490

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

SouadHadjiat · 2024-03-12T11:02:06Z

opencti-platform/opencti-graphql/config/schema/opencti.graphql

@@ -12126,8 +12126,8 @@ type ReportEditMutations {
  fieldPatch(input: [EditInput]!, commitMessage: String, references: [String]): Report @auth(for: [KNOWLEDGE_KNUPDATE])
  contextPatch(input: EditContext): Report @auth(for: [KNOWLEDGE_KNUPDATE])
  contextClean: Report @auth(for: [KNOWLEDGE_KNUPDATE])
-  relationAdd(input: StixRefRelationshipAddInput!): StixRefRelationship @auth(for: [KNOWLEDGE_KNUPDATE])
-  relationDelete(toId: StixRef!, relationship_type: String!): Report @auth(for: [KNOWLEDGE_KNUPDATE])
+  relationAdd(input: StixRefRelationshipAddInput!, commitMessage: String, references: [String]): StixRefRelationship @auth(for: [KNOWLEDGE_KNUPDATE])


I think all these API changes will need a client python update (here see opencti_report.py add_stix_object_or_stix_relationship)

Good point thank you, I'll update the client as well

reading python client code, I think it's better to add commitMessage and references data to StixRefRelationshipAddInput instead of adding new parameters here.

I think it's to be consistent with our existing APIs, which all declare (if I'm not mistaken) commitMessage and references as separate parameters. Ex:

relationsAdd(input: StixRefRelationshipsAddInput!, commitMessage: String, references: [String]): StixCoreObject relationDelete(toId: StixRef!, relationship_type: String!, commitMessage: String, references: [String]): StixCoreObject

SouadHadjiat · 2024-03-19T11:04:11Z

When trying to update a grouping with external reference enforced (as an admin), I don't see the list of external references in the dialog :

I can see the query in the network inspector (which returns 0 results), maybe the filter is not right :

JeremyCloarec · 2024-03-19T11:19:43Z

When trying to update a grouping with external reference enforced (as an admin), I don't see the list of external references in the dialog :

That's a bug that we also noticed, that is described here #6074 with the description "Edit an entity while having external reference". It will be worked on in the next step.
Right now the external reference dialog only has the external references already added to the report, and you can not see other external references from the platform in it.

aHenryJard

Only PR reading comment for now.

aHenryJard · 2024-03-19T12:20:02Z

opencti-platform/opencti-front/tests_e2e/enableReferences/addReportObservable.spec.ts

+  });
+});
+
+test('Add and remove observable from Observables tab of a Report as Admin user', async ({ page }) => {


question: is it intended that this "test" is not inside a test.describe block ?

Kind of! The describe blocks I used before were either to: group up tests that are dependant on each other (for the role/group/user creation), or to run the tests in the describe block with a specific context (the authentication describe block is used to run the authentication in a new browser context, to be able to log in the new user without having to log out the main user).
Since this test is independant from any others, I decided to not include it in a describe.
But the structure of the test file and the usage of describe blocks in general could be up to debate for sure

aHenryJard · 2024-03-19T12:25:41Z

opencti-platform/opencti-front/tests_e2e/enableReferences/addReportObservable.spec.ts

+  test('Create basic user role', async ({ page }) => {
+    const rolesSettingsPage = new RolesSettingsPage(page);
+    const rolePage = new RolePage(page);
+    const roleFormPage = new RoleFormPage(page);
+
+    await page.goto('/dashboard/settings/accesses/roles');
+    await expect(rolesSettingsPage.getSettingsPage()).toBeVisible();
+    await rolesSettingsPage.getAddRoleButton().click();
+    await roleFormPage.fillNameInput(noBypassRoleName);
+    await roleFormPage.getCreateButton().click();
+    await expect(rolesSettingsPage.getRoleInList(noBypassRoleName)).toBeVisible();
+    await rolesSettingsPage.getRoleInList(noBypassRoleName).click();
+    await rolePage.getEditButton().click();
+    await roleFormPage.getCapabilitiesTab().click();
+    await roleFormPage.getAccessKnowledgeCheckbox().click();
+    await expect(roleFormPage.getAccessKnowledgeCheckbox()).toBeChecked();
+    await roleFormPage.getCreateUpdateKnowledgeCheckbox().click();
+    await expect(roleFormPage.getCreateUpdateKnowledgeCheckbox()).toBeChecked();
+    await roleFormPage.getAccessAdministrationCheckbox().click();
+    await expect(roleFormPage.getAccessAdministrationCheckbox()).toBeChecked();
+  });


Random thinking: I guess we could have some way to reuse that piece of test code that creates user/roles and groups I don't mean one groups for all test, but have a methode like createRole(roleName, listOfCapacities) ...etc

That's a good point, we already talked about that with Souad and Jpk actually. We are planning to discuss it more during the e2e workshop

aHenryJard · 2024-03-19T12:27:31Z

opencti-platform/opencti-graphql/src/resolvers/container.js

-      relationAdd: ({ input }) => stixDomainObjectAddRelation(context, context.user, id, input),
-      relationDelete: ({ toId, relationship_type: relationshipType }) => stixDomainObjectDeleteRelation(context, context.user, id, toId, relationshipType),
+      relationAdd: ({ input, commitMessage, references }) => stixDomainObjectAddRelation(context, context.user, id, input, { commitMessage, references }),
+      // eslint-disable-next-line max-len


can you split the line instead ? Or is it really not readable ?

I didn't find a nice way to keep it readable and keep lint happy, so i went with this. Also, in other resolvers like stixCoreRelationship, the relationDelete line was also handled this way

JeremyCloarec marked this pull request as ready for review March 4, 2024 10:52

JeremyCloarec linked an issue Mar 4, 2024 that may be closed by this pull request

Mechanisms inconsistency when "enforce reference" is activated #5884

Closed

SamuelHassine force-pushed the master branch 6 times, most recently from 655453f to d793a45 Compare March 10, 2024 18:17

SouadHadjiat reviewed Mar 12, 2024

View reviewed changes

richard-julien force-pushed the master branch 5 times, most recently from 84a7fec to 5dff342 Compare March 14, 2024 23:42

JeremyCloarec added the filigran team use to identify PR from the Filigran team label Mar 15, 2024

JeremyCloarec force-pushed the issue/5884 branch from 91a6b87 to e6ee9f6 Compare March 18, 2024 16:51

aHenryJard reviewed Mar 19, 2024

View reviewed changes

JeremyCloarec force-pushed the issue/5884 branch from 949e85d to 9c98449 Compare April 3, 2024 07:36

SouadHadjiat approved these changes Apr 3, 2024

View reviewed changes

JeremyCloarec changed the title ~~[backend/frontend] Fix enable references behavior when adding entities to report~~ [backend/frontend] Fix enable references behavior when adding entities to containers Apr 3, 2024

JeremyCloarec added 2 commits April 3, 2024 10:32

[backend/frontend] Partial fix to enforced references in reports

dbaae7a

[client] Updated graphql.ts

c9fe0ea

SamuelHassine force-pushed the issue/5884 branch from 9c98449 to c9fe0ea Compare April 3, 2024 08:32

[front] Improve e2e tests

cba58bc

SamuelHassine merged commit ddbbd7e into master Apr 3, 2024
4 of 6 checks passed

SamuelHassine deleted the issue/5884 branch April 3, 2024 10:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[backend/frontend] Fix enable references behavior when adding entities to containers #6175

[backend/frontend] Fix enable references behavior when adding entities to containers #6175

JeremyCloarec commented Feb 29, 2024 •

edited

codecov bot commented Feb 29, 2024 •

edited

SouadHadjiat Mar 12, 2024

JeremyCloarec Mar 12, 2024

aHenryJard Mar 19, 2024

SouadHadjiat Mar 25, 2024

SouadHadjiat commented Mar 19, 2024

JeremyCloarec commented Mar 19, 2024

aHenryJard left a comment

aHenryJard Mar 19, 2024

JeremyCloarec Mar 19, 2024

aHenryJard Mar 19, 2024

JeremyCloarec Mar 19, 2024

aHenryJard Mar 19, 2024

JeremyCloarec Mar 19, 2024

[backend/frontend] Fix enable references behavior when adding entities to containers #6175

[backend/frontend] Fix enable references behavior when adding entities to containers #6175

Conversation

JeremyCloarec commented Feb 29, 2024 • edited

Checklist

codecov bot commented Feb 29, 2024 • edited

Codecov Report

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

SouadHadjiat commented Mar 19, 2024

JeremyCloarec commented Mar 19, 2024

aHenryJard 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

JeremyCloarec commented Feb 29, 2024 •

edited

codecov bot commented Feb 29, 2024 •

edited