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.

Sign up for GitHub

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

Jump to bottom

[backend] handle inferred organizations in organization sharing #9558

Merged
merged 5 commits into from
Jan 16, 2025
Merged

[backend] handle inferred organizations in organization sharing #9558

merged 5 commits into from
Jan 16, 2025

Conversation

marieflorescontact
Copy link
Member

@marieflorescontact marieflorescontact commented Jan 10, 2025 •
edited
Loading

Proposed changes

  • replace const shouldOrgs = user.allowed_organizations by const shouldOrgs = user.organizations as infferred organization are not in allowed_organizations

Related issues

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

Further comments

@marieflorescontact marieflorescontact self-assigned this Jan 10, 2025
@github-actions github-actions bot added the filigran team use to identify PR from the Filigran team label Jan 10, 2025
@marieflorescontact marieflorescontact marked this pull request as ready for review January 10, 2025 14:52
@codecov Codecov
Copy link

codecov bot commented Jan 10, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.

Project coverage is 65.36%. Comparing base (dde5551) to head (21d40ee).
Report is 9 commits behind head on master.

Files with missing lines Patch % Lines
...ti-platform/opencti-graphql/src/database/engine.js 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #9558      +/-   ##
==========================================
+ Coverage   65.22%   65.36%   +0.13%     
==========================================
  Files         630      630              
  Lines       60199    60258      +59     
  Branches     6741     6817      +76     
==========================================
+ Hits        39267    39388     +121     
+ Misses      20932    20870      -62

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

@SouadHadjiat
Copy link
Member

we might need to change other parts where we use allowed_organizations (in access.ts)

@richard-julien richard-julien force-pushed the master branch 4 times, most recently from f3d2300 to e2ea841 Compare January 12, 2025 14:50
@JeremyCloarec
Copy link
Contributor

I think you're right @SouadHadjiat, everything using allowed_organizations in access.ts should use organizations now.
Or maybe an other fix would be to change the way organizations and allowed_organizations are built in user.js buildCompleteUser: we could include inferences in allowed_organizations and not organizations. That's actually what was done before this change #5721, maybe that was an unintended change?

SouadHadjiat
SouadHadjiat reviewed Jan 16, 2025
View reviewed changes
Copy link
Member

@SouadHadjiat SouadHadjiat left a comment

Choose a reason for hiding this comment

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

I think we want to remove completely allowed_organizations, right ? @marieflorescontact

SouadHadjiat
SouadHadjiat reviewed Jan 16, 2025
View reviewed changes
opencti-platform/opencti-graphql/tests/01-unit/utils/logger-test.ts Outdated
@@ -129,6 +129,12 @@ describe('Logger test suite', () => {
expect(cleanObject.cause.attributes.cause.message).toBe('embedded error');
});

it('Log object with error correctly formatted', () => {
Copy link
Member

Choose a reason for hiding this comment

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

did you change this test ?

Copy link
Member Author

Choose a reason for hiding this comment

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

no

@SouadHadjiat
Copy link
Member

you have a commit from Julien that comes from master (due to force push on master done by Julien a few days ago), you might need to drop this commit on your branch, then rebase master.

@marieflorescontact
Copy link
Member Author

I think we want to remove completely allowed_organizations, right ? @marieflorescontact

I thought we wanted to use user.organizations but keep user.allowed_organizations in the first time. Am i right?

@richard-julien richard-julien merged commit d61f670 into master Jan 16, 2025
6 of 7 checks passed
@richard-julien richard-julien deleted the issue/9078 branch January 16, 2025 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SouadHadjiat SouadHadjiat SouadHadjiat left review comments

@richard-julien richard-julien Awaiting requested review from richard-julien

Assignees

@marieflorescontact marieflorescontact

Labels
filigran team use to identify PR from the Filigran team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Org segregation] Object not visible despite belonging to the correct organization
4 participants
@marieflorescontact @SouadHadjiat @JeremyCloarec @richard-julien