config add web middleware #280

will-moore · 2021-03-03T23:06:09Z

With an update to the default omero.web.middleware as in the PR above,
if the user has previously updated their config then the values will be stored in the etc/grid/config.xml and
the new default will be ignored.
This PR aims to update the omero.web.middleware config in the same way as the above PR.
This will only happen if the above PR is included in omero-web (the new middleware is importable)

NB: last commit of the PR above changes the default omero.web.middleware setting so that it is simpler
for this PR to update in the same way (simply append a single class, instead of update multiple classes in the list).
This allows us to simplify the update and reduce the amount of settings that are hard-coded in config.py.

I'm assuming no-one is upgrading from "4.2.1" (6 years ago).

To test:

with current web (without Custom response headers omero-web#260) do typical CORs config to update middleware:

$ omero config append omero.web.middleware '{"index": 0.5, "class": "corsheaders.middleware.CorsMiddleware"}'
$ omero config append omero.web.middleware '{"index": 10, "class": "corsheaders.middleware.CorsPostCsrfMiddleware"}'

Update omero-py to include this PR
The config should not be changed yet (this won't show CustomHeadersMiddleware

$ omero config get omero.web.middleware

Update omero-web to include Custom response headers omero-web#260
The config should now show CustomHeadersMiddleware

$ omero config get omero.web.middleware

NB: If you ONLY stop web, upgrade web and start web (without any $ omero config... command), the change will not take effect.

joshmoore

A few suggestions for modification. Additionally, this solves the specific issue, but doesn't solve the overall design bug. That's certainly a trade-off we can make, but just to be clear.

joshmoore · 2021-03-04T10:51:20Z

src/omero/config.py

+                    from omeroweb.middleware import CustomHeadersMiddleware
+                except:
+                    self.logger.info(
+                        "Failed to import"


Does this mean the user should upgrade omero-web? Worth saying so?

fixed in d4e16be

This only gets printed if the user sets DEBUG=1 ... which seems pretty unlikely.

joshmoore · 2021-03-04T10:52:31Z

src/omero/config.py

+    def middleware_check(self):
+        for k, v in self.properties(None, True):
+            version = self.version(k)
+            if version == "5.1.0" and v is not None:


This block is only really needed if x == omero.web.middleware, right?

We don't have access to x at this point. v is a list of properties and we need to update the value of omero.web.middleware and the omero.config.version.

Sure, but the for loop over v could be moved up to this point, no?

I'm sorry, I'm not seeing it. The loop over v starts right after I check if v is not None.

No worries. It's a super minor optimization and this isn't critical code.

joshmoore · 2021-03-04T10:54:28Z

src/omero/config.py

+                    for x in list(v):
+                        # User has configured their middleware list.
+                        if x.get("name") == "omero.web.middleware":
+                            val = x.get("value", "")


Should the default be [] or {}? Doesn't json.loads("") throw?

fixed in d4e16be

Immediate modifications look good.

will-moore · 2021-03-04T11:39:45Z

@joshmoore I could maybe do a better fix by creating a INSTALLED_MIDDLEWARE and ADDITIONAL_MIDDLEWARE in ome/omero-web#260 (just as we have for omero.web.apps with INSTALLED_APPS and ADDITIONAL_APPS). Then use this config.py update to only leave user-configured middleware in the omero.web.middleware (ADDITIONAL_MIDDLEWARE) list.
I might give it a try and see how it looks...
Don't know if/when we might want to apply this to other configs. There's not many that we are likely to update.

joshmoore · 2021-03-04T11:52:00Z

Don't know if/when we might want to apply this to other configs.

Are there others where we are copying a default?

will-moore · 2021-03-04T12:19:33Z

I could list them. Looking for where we use json.loads and the default isn't empty:

"omero.web.caches": '{"default": {"BACKEND":' ' "django.core.cache.backends.dummy.DummyCache"}}'
"omero.web.server_list": '[["%s", 4064, "omero"]]' % CUSTOM_HOST
"omero.web.open_with": '[["Image viewer", "webgateway", {"supported_objects": ["image"],"script_url": "webclient/javascript/ome.openwith_viewer.js"}]]'
"omero.web.ui.top_links": (Data, History, Help)
"omero.web.ui.metadata_panes": (tag, map, table, file, comment, rating, other)
"omero.web.ui.right_plugins":  (Aquisition, Preview)

I don't expect many of those defaults to change, or for users to have them configured. Except maybe top_links and open_with. BUT, if we only allow users to ADD to the defaults, then they can't remove items from defaults. E.g. remove metadata_panes.
The same applies to omero.web.middleware. If someone has already removed an item from the default settings, then putting all the defaults into a non-editable list would add it back. So we can't support remove of defaults AND auto update of defaults.
I think I'll try the separation of omero.web.middleware into 'installed' and 'additional' lists, and we can see what that looks like. Then we can decide if we want to do the same with open_with and/or top_links.

will-moore · 2021-03-04T13:07:03Z

With ome/omero-web@70d2229 and the last commits above, the user settings are now distinct from the default middleware.

joshmoore

Testing locally:

In a new environment, ran append and the full list was cached.
Updated omero-py and ran get, the old version was shown.
Ran with DEBUG=1 and could see the message telling me to upgrade.
Updated omero-web and was only shown the properties I had set.

Questions inline.

joshmoore · 2021-03-05T12:00:49Z

src/omero/config.py

+                    from omeroweb.middleware import CustomHeadersMiddleware
+                except:
+                    self.logger.info(
+                        "Failed to import"


This only gets printed if the user sets DEBUG=1 ... which seems pretty unlikely.

joshmoore · 2021-03-05T12:03:39Z

src/omero/config.py

@@ -222,6 +223,41 @@ def toplinks_check(self):
                    if x.get("name") == self.KEY:
                        x.set("value", self.VERSION)

+    def middleware_check(self):
+        mw_classes = [
+            "django.middleware.common.BrokenLinkEmailsMiddleware",


So there's no way to disable this middleware, right? Are there any cases you can think of where that would be necessary?

No, I can't think of any.

joshmoore · 2021-03-05T15:27:41Z

@sbesson was kind enough to scour the history to find ome/openmicroscopy#2030 (comment) -- pasting it here for reference.

will-moore · 2021-03-08T07:25:42Z

I don't think we're going to be able to apply the same approach to the other settings lists above.
For the UI settings (and open_with), it's possible a user will have configured them to remove one or more default items.
For web.caches, this isn't a list, so if it's been configured it will likely be set rather than append to default.
I guess for server_list, we might want to change the one default option at some point. But I think we could leave any upgrade path in config.py till it's needed.

will-moore · 2021-06-16T10:57:49Z

With ome/omero-web#260 closed for now, don't need this currently. Closing...

will-moore added 2 commits March 3, 2021 22:33

config.py adds CustomHeadersMiddleware to web.middleware

eacd393

config.py only adds CustomHeadersMiddleware if it can be imported

b510dcd

joshmoore requested changes Mar 4, 2021

View reviewed changes

Fix issues from PR 280

d4e16be

joshmoore previously approved these changes Mar 4, 2021

View reviewed changes

will-moore added 2 commits March 4, 2021 13:03

config.py removes default omero.web.middleware from config.xml

38384c0

Update logger.info statement

43b0404

Fix test_config

ec751bd

will-moore force-pushed the config_add_web_middleware branch from 36e766d to ec751bd Compare March 4, 2021 14:18

joshmoore reviewed Mar 5, 2021

View reviewed changes

Log Error if user hasn't upgraded omero-web

5f0457e

will-moore closed this Jun 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config add web middleware #280

config add web middleware #280

will-moore commented Mar 3, 2021 •

edited

joshmoore left a comment

joshmoore Mar 4, 2021

will-moore Mar 4, 2021

joshmoore Mar 5, 2021

joshmoore Mar 4, 2021

will-moore Mar 4, 2021

joshmoore Mar 4, 2021

will-moore Mar 8, 2021

joshmoore Mar 8, 2021

joshmoore Mar 4, 2021

will-moore Mar 4, 2021

will-moore commented Mar 4, 2021

joshmoore commented Mar 4, 2021

will-moore commented Mar 4, 2021

will-moore commented Mar 4, 2021

joshmoore left a comment

joshmoore Mar 5, 2021

joshmoore Mar 5, 2021

will-moore Mar 8, 2021

joshmoore commented Mar 5, 2021

will-moore commented Mar 8, 2021

will-moore commented Jun 16, 2021

config add web middleware #280

config add web middleware #280

Conversation

will-moore commented Mar 3, 2021 • edited

joshmoore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

will-moore commented Mar 4, 2021

joshmoore commented Mar 4, 2021

will-moore commented Mar 4, 2021

will-moore commented Mar 4, 2021

joshmoore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshmoore commented Mar 5, 2021

will-moore commented Mar 8, 2021

will-moore commented Jun 16, 2021

will-moore commented Mar 3, 2021 •

edited