ome / omero-gallery

https://pypi.org/project/omero-gallery/
GNU Affero General Public License v3.0
5 stars 14 forks source link

gallery login required #45

Closed will-moore closed 4 years ago

will-moore commented 5 years ago

See https://github.com/ome/omero-gallery/pull/42#issuecomment-527377318

This adds @login_required() decorator to the gallery home page, meaning that non-public users have to login. NB: The need to login for the new gallery was omitted before since all data was sometimes loaded from a different public server (e.g. IDR) so we didn't need to login locally. But this configuration is edge case and not needed in any of our workflows. More often the lack of @login_required() on the home page would mean the page itself would load but no data would load (permission denied) and no login requested.

To test:

joshmoore commented 4 years ago

Leaving to @pwalczysko to sign off since I failed to make it through yesterday, but I was wondering about the state of:

without PR with PR
old gallery / public
old gallery / non-public
new gallery / public
old gallery / non-public

but I don't think we have anywhere where that is testable.

will-moore commented 4 years ago

Old gallery needed login before this PR, so nothing changes there.

The login needed? table should look like:

without PR with PR
old gallery / public no no
old gallery / non-public yes yes
new gallery / public no no
new gallery / non-public no-fail yes
pwalczysko commented 4 years ago

Screen Shot 2020-01-08 at 16 04 03 Why does gallery fail to load the thumbs of Project 3107 (the very right one in the screenshot) whereas it loads the thumb from the Project 3108 (which looks exactly the same, same images like 3107) ?

login as user-3

Otherwise, works as described.

will-moore commented 4 years ago

@pwalczysko Just looking at the code... The logic goes like this:

We just never before had 2 studies showing the same thumbnail and that situation was never envisioned for IDR.

joshmoore commented 4 years ago

@will-moore : leaving you to follow up on @pwalczysko's question, but merging.