Fix #343

[dhovart]

Cf https://github.com/neuroanatomy/BrainBox/issues/343#issuecomment-1151029187

[codecov-commenter]

Codecov Report

Merging #344 (f641e97) into master (3d80737) will increase coverage by 0.12%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #344      +/-   ##
==========================================
+ Coverage   63.27%   63.39%   +0.12%     
==========================================
  Files          16       16              
  Lines        2309     2306       -3     
==========================================
+ Hits         1461     1462       +1     
+ Misses        848      844       -4     

Flag	Coverage Δ
integration	53.90% <100.00%> (+0.28%)	:arrow_up:
unittests	45.75% <100.00%> (+0.05%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
controller/mri/mri.controller.js	80.50% <ø> (+0.50%)	:arrow_up:
controller/dataSlices/dataSlices.js	89.05% <100.00%> (ø)
controller/atlasmakerServer/atlasmakerServer.js	42.91% <0.00%> (+0.20%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 3d80737...f641e97. Read the comment docs.

[ntraut]

This looks like the bug is fixed but should we write a test to reproduce the bug or is that ok if we simply fix it?

Also, I wonder if the description of the function hasAccesstoFileIfAllowedBySomeProjects is still relevant: https://github.com/neuroanatomy/BrainBox/blob/3d807378f53c03d45a974dcb5002a29847cc1232/services/BrainboxAccessControlService.js#L21-L34

Not sure also if the privilegedAccessInfo warning is still relevant: https://github.com/neuroanatomy/BrainBox/blob/10ac79df1c387bd5dc0406042edf5dcf10db22fa/templates/mri.mustache#L53

There is also still the problem that even if the mri controller should no longer fail because of trying to get the property shortname of null, the errors are still not properly handled because it is an async function and express is not designed to automatically catch errors coming from promises.

[dhovart]

You're right about the test, I should introduce one to make sure we're not going to have regressions. Will do this first thing tomorrow.

Answered you earlier about the access control check in #343

I think it's a good thing to still have the warning in place. Of course you can now directly access the page if you know the URL but hopefully people will not share the address, because of this warning (it will only show if you're a collaborator to a project using it, and that no project allow anyone seeing files)

[r03ert0]

Keep the warning. @katja already had a case of a collaborator who unintentionally leaked the URL of an MRI that wasn't supposed to be shared (the collaborator added it to a public project).

On Jun 12 2022, at 5:05 pm, Denis Hovart @.***> wrote:

You're right about the test, I should introduce one to make sure we're not going to have regressions. Will do this first thing tomorrow. Answered you earlier about the access control check in #343 (https://github.com/neuroanatomy/BrainBox/issues/343) I think it's a good thing to still have the warning in place. Of course you can now directly access the page if you know the URL but hopefully people will not share the address, because of this warning (it will only show if you're a collaborator to a project using it, and that no project allow anyone seeing files) — Reply to this email directly, view it on GitHub (https://github.com/neuroanatomy/BrainBox/pull/344#issuecomment-1153198832), or unsubscribe (https://github.com/notifications/unsubscribe-auth/AARUETFZTPPYJARDZRJJUK3VOX4DTANCNFSM5YJ545KQ). You are receiving this because you are subscribed to this thread.

[ntraut]

Maybe this bug is too specific to need to make a test for it: it happens when we load a test mri which is not part of a project but only from the second time.

It is the sentence "The MRI file is only accessible if some projects allow it" which no longer describes the actual behavior, but maybe it is not very important to have this imprecision in the description.

Also, in which case some files may use a 'source' property to store url? I looked in the brainbox db and did not find any...

What do you think we should do to handle promise rejections in express middlewares like mri controller. Should we add a catch block each time or use wrapper like express-async-handler ?

[ntraut]

Also, in which case some files may use a 'source' property to store url? I looked in the brainbox db and did not find any...

Ah yes I didn't look well, it's in the case the file has a name in addition to its source.

[dhovart]

I think you're mixing up two methods documentation. The behaviour is still the same as it was and it says clearly: if all projects allow it.

It could be a good idea to introduce express-async-handler, yes.

[ntraut]

No with the fix the MRI files are accessible even if no project allows it provided that we have their url, it is just the annotations from the private projects that we don't see.

For the possible source and name properties of files I'm still confused because I tried to add MRI to a project giving them names, and they are still stored as single string in the files list. Does it come from a previous behaviour which changed since? Would not it better to try to uniformize rather than dealing continuously with those special cases?

[dhovart]

But to me the fix is unrelated to this particular method, so I'm not sure what is your point.

I also was confused to see we both had at times a list of object with the properties "source" and "name" and a list of strings. I attempted unifying it but the database may still not be consistent.

[ntraut]

Before MRI had to be listed in a project as viewable to be visible (if we except the bug that included also project with backup=true), now we can see them even if it is not the case. Maybe it is not exactly the case and I am still missing something, am I?

For the "source" and "name" issue maybe that's it and we can fix in the db, but the current behaviour of storing the name in the mri collection rather than in the project collection struggles me: what if several projects use the same url but with different names? should we take an arbitrary one? is that normal that people have the right to change the name set by other projects?

[dhovart]

Oh, I think I may get your point. I didn't understand it was a matter of semantics. What you're saying is that now the method name does no longer look correct to you, since we can access the files no matter what.

The method hasAccessToFile... does not tell you if you can directly access the MRI URL, it just compares the access level set as a parameter (view/add/remove etc.) and tells you if this specific access is set in settings as granted or not.

If I follow your logic we should remove this setting defining viewing access to files since we can directly access a MRI if we know its url. Is that correct?

Can you point me to where you see that people can change the name of a file defined in other projects? I think names are only stored in a project's settings.

[ntraut]

I was more thinking of the description in the jsdoc, but maybe also the function name doesn't fit very well anymore.

I think the files authorizations are more to tell if the users have the right to view/edit the list of the project files rather than viewing the files themselves, that may be the source of the confusion.

[ntraut]

Can you point me to where you see that people can change the name of a file defined in other projects? I think names are only stored in a project's settings.

Sorry, i forgot to answer to that question. It's simple, now when people give a name for an mri in a project, it will be save in the mri collection. So if the mri is already used by another project, the name will be overwritten. I don't really understand why it's designed like this.

[ntraut]

hello, i think we should not wait to long before merging this pr. for the two remaining issues, i can create a separate pr to introduce express-async-handler, and i can create an issue for the mri names being shared between projects (maybe that's a feature but at least we need to clarify this).

[katjaq]

Ok! Sounds good to me. I just remembered that @dhovart had said on MM to not merge yet when this PR first was made. But I guess now it looks good to merge. Do you agree @dhovart ? Thank you @ntraut for your testing & feedback. Yes, I agree, we can look at the names all together and add clarification. So I go ahead and merge, if you do not oppose @dhovart .

Thank you all!

[ntraut]

Hi @katjaq, he said that for his other pr that he created just to see the result of the ci. i will merge so, thanks @dhovart for those fixes!