fixed undefined email in access groups bug

[J-avery32]

Description

PR fixes security issue where when a user's profile email is undefined it will be concatenated to the groups array. This prevents an undefined value from being put in the array of groups.

Motivation

When an instrumentGroup is undefined on any dataset this will give that user access to that dataset, regardless of whether that user and dataset have matching groups.

Tests included/Docs Updated?

• [ ] Included for each change/fix?
• [ ] Passing? (Merge will not be approved unless this is checked)
• [ ] Docs updated?
• [ ] New packages used/requires npm install?
• [ ] Toggle added for new features?

[J-avery32]

Perhaps it would be useful to check if instrumentGroup is undefined here as well so that we have double protection against this bug? https://github.com/SciCatProject/backend/blob/master/common/models/dataset.js#L130

[dylanmcreynolds]

To expand, @J-avery32 found a place where we if an OIDC user profile didn't have an email and ended up getting access to datasets with an undefined instrumentGroup. That check is here https://github.com/SciCatProject/backend/blob/37c0bc4d8ec1ac8c92fb8c33fe92642d0cb64375/common/models/dataset.js#L130

[dylanmcreynolds]

I assume that the failing gihub action is due to the submitter not being an owner and docker build not giving access?

[stephan271]

I will ask Carlo to take care of this PR. He is back from vacation soon. I agree, the test for instrumentGroup should be added as well.

[nitrosx]

@stephan271 thank you so much for taking the lead on this PR. I will be able to check the reason why the tests fail early next week

[J-avery32]

There are some other issues that I've found which should be fixed in this same PR. So I will need a little more time to complete it.

[minottic]

about the CI failing, I've noticed that the push to ghcr rule has changed a little recently.

This was the rule that triggered the push in the past. What do we want to apply here? Stick to what we used to do, or push to ghcr on every PR to master? If the latter, I am afraid we need to solve the login to ghcr by stopping using the {{ github.actor }} env var (as users not in the scicat group will encounter 403)

[dylanmcreynolds]

So with the old rule, we only try and build/push docker when a PR is merged. It would be nice to see if a PR breaks the docker build before merging, but that should be very rare and if it's a whole lot easier to not put the old rule back in, I vote for that.

[J-avery32]

So, it turns out that my fix actually introduces another larger bug. This was caught by the test I created in accessGroups.js. When any user's currentGroups in ctx.options is undefined or empty they automatically get access to all datasets in the database no matter what. https://github.com/SciCatProject/backend/blob/37c0bc4d8ec1ac8c92fb8c33fe92642d0cb64375/common/models/ownable.js#L16

Their array will only be empty when they have an undefined email because my code prevents the undefined value from being added to the array.

Now when I tried to fix this so that empty user arrays or undefined arrays simply gave the user access to no special groups I got a lot of failing tests. It seems that this is out of my scope to fix so I would appreciate it if someone else took a look at that bug.

Also please note that there are two areas in the code where we want to add an undefined check for intrumentGroup.

One here: https://github.com/SciCatProject/backend/blob/37c0bc4d8ec1ac8c92fb8c33fe92642d0cb64375/common/models/mongo-queryable.js#L614

And one here: https://github.com/SciCatProject/backend/blob/37c0bc4d8ec1ac8c92fb8c33fe92642d0cb64375/common/models/dataset.js#L130

I can add those in or leave them for whoever looks into the empty array bug.

[nitrosx]

@J-avery32 : please do go ahead and add your code. When we work on the fixing the larger bug and all the failing tests, we will review your addition and decide what to do. Thank you so much

[minottic]

@J-avery32 does this

When any user's currentGroups in ctx.options is undefined or empty they automatically get access to all datasets in the database no matter what.

happen also when the u.profile.accessGroups is undefined?

[J-avery32]

@minottic yes. In fact I believe that when the profile.accessGroups is undefined even without my fix that user gets access to everything in the database. Because of the else if, an undefined accessGroups means that the email isn't added to the array. We just get an empty array.

[minottic]

ok, I see, thanks. Maybe doing a step backwards, wouldn't it be a fast fix not to allow an empty email in the u.profile by creating a default fallback whenever it is empty at user creation (and then later concat it to the currentGroups in all cases?). Something like:

u.profile.email? u.profile.email: u.email (and I guess in this case it would come from the auth method generated email, which usually is - for ldap and oidc - method.username@loopback.io)

In this case, we also preserve the logic that the logged-in user has access to the ds she created.

[J-avery32]

Can we be sure that u.email will always be defined?

[J-avery32]

Also my original thought about what caused this bug is wrong. I'm looking further into it and it doesn't seem like the instrumentGroup code is hit for the specific behavior I'm observing. Though the fact that there is no check for instrumentGroup being undefined is probably still a bug, and should be added in to the code. However, I think it affects datablocks and attachments. I'm not super familiar with the internals of scicat so would be nice if someone could verify that.

I think the real issue is if accessGroups, sharedWith, or ownerGroup is undefined on a dataset. The problem is still caused by the undefined email value being added to groups.

Notice this part of the code: https://github.com/SciCatProject/backend/blob/37c0bc4d8ec1ac8c92fb8c33fe92642d0cb64375/common/models/ownable.js#L16-L35

[J-avery32]

This is a comprehensive list of things I think need to get done:

• [ ] Ensure that an undefined accessGroups property on the user does not allow them to access all data/write tests for this (in ownable.js)
• [ ] Ensure that an empty accessGroups array on the user does not allow them to access all data/write tests for this (in ownable.js)
• [ ] Filter out undefined values from the groups array before passing it to mongo/write tests for this (in ownable.js)
• [ ] Ensure that we don't concatenate an undefined email into the accessGroups array and if accessGroups is undefined we don't just get an empty array without concatenating the email/write tests for this (0-script.js)
• [ ] Ensure that we check if instrumentGroup is undefined before comparing it against user roles/write tests for this (dataset.js and mongo-queryable.js)

[minottic]

I think there are cases indeed where the email from ldap can be empty. So, if I understand correctly, the problem is that no filter on groups is added when the user's groups are empty or null. I'll try to fix this condition and see how many tests fail and how hard they are to fix.

If that's a lot of work, I would say a fast fix might be falling back to a hardcoded email when no profile.email is defined and concatenating it to the currentGroups. This might not be the best solution, but anyway at some near point in the future we'll move to the new be