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] Workspace should have at least one valid admin (#3922) #4014

Merged
merged 2 commits into from
Aug 10, 2023

Conversation

Archidoit
Copy link
Member

@Archidoit Archidoit commented Aug 9, 2023

Github issue: #3922

demo: see link in Teams

@Archidoit Archidoit self-assigned this Aug 9, 2023
@Archidoit Archidoit marked this pull request as draft August 9, 2023 07:39
@Archidoit Archidoit force-pushed the issue/3922 branch 2 times, most recently from f420a0f to 36bbc26 Compare August 9, 2023 10:03
@Archidoit Archidoit marked this pull request as ready for review August 9, 2023 10:05
@Kedae Kedae added the filigran team use to identify PR from the Filigran team label Aug 9, 2023
@Archidoit Archidoit marked this pull request as ready for review August 9, 2023 13:46
@Archidoit
Copy link
Member Author

Comment: If a user is deleted and he is the only admin of a workspace, the workspace will be unadministrable. This is a more global problem, that will be resolved in a future issue: #4026

@yassine-ouaamou
Copy link
Member

Behaviour tested, code checked, PR approved!

@Archidoit Archidoit merged commit 5a50637 into master Aug 10, 2023
6 checks passed
@Archidoit Archidoit deleted the issue/3922 branch August 10, 2023 08:40
.map((e) => e.id);
if (adminIds.includes(MEMBER_ACCESS_ALL)) { // everyone is admin
return true;
}
Copy link
Member

Choose a reason for hiding this comment

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

maybe add a condition here that checks that there is at least one member with admin right, before trying to resolve the admin ids, otherwise return false
like this :

if (adminIds.length == 0) { 
   return false 
} 

It's the condition we had before.

const userIds = adminIds
.filter((id) => !groups.map((o) => o.id).includes(id)
&& !organizations.map((o) => o.id).includes(id))
.concat(groupsMembersIds, organizationsMembersIds);
Copy link
Member

Choose a reason for hiding this comment

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

groups.map((o) => o.id) and organizations.map((o) => o.id) are done inside the filter, maybe we could extract it before ? have organizationIds and groupIds, then use the include ?

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.

None yet

4 participants