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

check project owner is not removed #566

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

rsiminel
Copy link
Contributor

@rsiminel rsiminel commented Dec 9, 2024

closes #498
@mboudet

Yup, all that was needed was a simple check that the user being deleted isn't the project owner (no need to check if it's the last member of the project because in that case they would be the project owner anyway).

@mboudet
Copy link
Member

mboudet commented Dec 10, 2024

(no need to check if it's the last member of the project because in that case they would be the project owner anyway).

Not so sure about that. Currently, there is no check on the user deletion route regarding projects, and the project 'owner' is stored on the project itself. -> If we delete a user who is a project owner, it's still going to be the owner. So if there is another member of the project, it's going to be the last member of the project, but not the owner.

So, what is needed is a check on the 'user delete route': if the user we are trying to delete is the owner of the project, OR the last user, we abort and send an error message.

There is already a check for websites and DBs here: https://github.com/genouest/genouestaccountmanager/blob/master/routes/users.js#L529 , might as well add a check for project ownership.

@rsiminel
Copy link
Contributor Author

rsiminel commented Dec 12, 2024

If we delete a user who is a project owner, it's still going to be the owner. So if there is another member of the project, it's going to be the last member of the project, but not the owner.

So you're describing a case where the project owner's account was deleted before this PR?
Projet membership is stored on the user, so we'd have to look at every single user to see if one of them is in the same project.

Might I suggest 3 options:

  • We load every single User from the db every time we delete an account. I'm assuming this is sub-optimal.
  • We add a member_count attribute to the Project class.
  • We add a check that all project owners correspond to existing user accounts. I prefer this solution in a 'two birds, one stone' kind of way.

@mboudet
Copy link
Member

mboudet commented Dec 13, 2024

Might I suggest 3 options:

* We load every single User from the db every time we delete an account. I'm assuming this is sub-optimal.

Not THAT suboptimal, honestly. We don't delete users often (and we already do it for the groups when deleting an user).
Plus we do this query anytime we go to the admin page for a project anyway. A proper mongo query filtered on project membership is fairly fast.

* We add a member_count attribute to the Project class.

Not really fond of this one

* We add a check that all project owners correspond to existing user accounts. I prefer this solution in a 'two birds, one stone' kind of way.

Where would this check be, and how would it run? (And how would it communicate the issue to the admin?)

mboudet
mboudet previously approved these changes Jan 14, 2025
routes/users.js Outdated
}
const allprojects = user.projects ? user.projects : [];
for (let project_name of allprojects) {
const users_in_project = await dbsrv.mongo_users().find({ 'projects': project_name }).toArray();
Copy link
Member

@mboudet mboudet Jan 14, 2025

Choose a reason for hiding this comment

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

Might be better to use .count() instead of toArray, and just check the count value after

dbsrv.mongo_users().find({ 'projects': project_name }).count()

Copy link
Member

Choose a reason for hiding this comment

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

(Need to check if it works. It works in mongo directly, but well..)

routes/users.js Outdated
for (let project_name of allprojects) {
const users_in_project = await dbsrv.mongo_users().find({ 'projects': project_name }).toArray();
if (users_in_project.length <= 1) {
res.status(403).send({ message: 'User is the last member of project ${project_name}' });
Copy link
Member

Choose a reason for hiding this comment

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

Might be better to store project names somewhere, and send the response after will all projects.
(Otherwise, you'll try to delete -> change a project -> try to delete -> etc)

@mboudet mboudet self-requested a review January 14, 2025 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Removing a user who is a project owner should raise a warning / error
2 participants