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] secure organization deletion (#8838) #8903

Merged
merged 6 commits into from
Nov 8, 2024
Merged

Conversation

SouadHadjiat
Copy link
Member

@SouadHadjiat SouadHadjiat commented Nov 5, 2024

Proposed changes

  • add protections to prevent organization deletion if it's a platform organization, if it has an administrator, if it has members
  • call organizationDelete in retention manager (however it doesn't delete permanently, it will go in trash => I don't think it's an issue, since organization delete can be quite destructive, so it's better we keep it in trash in case)
  • add tests

Related issues

To test : organization can't be deleted if it is used as platform organization, or it has at least one member (user part of the organization), or has an organization admin (org admin is also a member, but the error message is different). Otherwise the deletion should work. Also error message is different for users depending on their capability : if user has set access, the message will explain the reason (members or orga admin reason).

Checklist

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

Copy link

codecov bot commented Nov 5, 2024

Codecov Report

Attention: Patch coverage is 68.18182% with 14 lines in your changes missing coverage. Please review.

Project coverage is 66.65%. Comparing base (ad7bbb2) to head (a50bbab).
Report is 10 commits behind head on master.

Files with missing lines Patch % Lines
...ql/src/modules/organization/organization-domain.ts 65.62% 11 Missing ⚠️
...latform/opencti-graphql/src/manager/taskManager.js 25.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8903      +/-   ##
==========================================
+ Coverage   66.29%   66.65%   +0.36%     
==========================================
  Files         597      597              
  Lines       61033    61087      +54     
  Branches     6276     6604     +328     
==========================================
+ Hits        40460    40716     +256     
+ Misses      20573    20371     -202     

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

@SouadHadjiat SouadHadjiat marked this pull request as ready for review November 5, 2024 10:43
@SouadHadjiat SouadHadjiat added the filigran team use to identify PR from the Filigran team label Nov 5, 2024
@lndrtrbn lndrtrbn self-requested a review November 6, 2024 10:41
@Archidoit
Copy link
Member

Behavior ok, working well 👍

@Archidoit
Copy link
Member

⚠️ We can still delete a platform organization via a background task in Data > Entities

@SouadHadjiat
Copy link
Member Author

SouadHadjiat commented Nov 6, 2024

⚠️ We can still delete a platform organization via a background task in Data > Entities

you're right, task manager calls directly deleteElementById, I need to change it as I did with retention manager => should be fixed now

@@ -237,6 +238,9 @@ const executeDelete = async (context, user, element, scope) => {
}
if (scope === BackgroundTaskScope.Import) {
await deleteFile(context, user, element.id);
} else if (element.entity_type === ENTITY_TYPE_IDENTITY_ORGANIZATION) {
// call organizationDelete which will ensure protections (for platform organization & members)
await organizationDelete(context, user, element.internal_id);
} else {
await deleteElementById(context, user, element.internal_id, element.entity_type);
Copy link
Member

Choose a reason for hiding this comment

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

wouln't it be better to add the organization check in 'deleteElementById' ? to avoid forgetting to add the check when using it?

Copy link
Member

@lndrtrbn lndrtrbn Nov 7, 2024

Choose a reason for hiding this comment

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

That would probably make circle dependencies as organizationDelete uses deleteElementById. Maybe having a higher level function that make those kind of checks and then redirect to the correct delete function could be a thing? But it means some global refactoring at every delete resolver, maybe not relevant inside the scope of this PR idk

@SouadHadjiat SouadHadjiat merged commit b43162b into master Nov 8, 2024
6 checks passed
@SouadHadjiat SouadHadjiat deleted the issue/8838 branch November 8, 2024 09:47
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
Development

Successfully merging this pull request may close these issues.

Prevent deletion of the organization that is used as plateforme organization or attibuted to a user
3 participants