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

make webui work with non-root context path #1305

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

hubsif
Copy link
Contributor

@hubsif hubsif commented Feb 9, 2022

This is based on @altaroca's work in PR #1291 (issue #1290), opening a new PR since I don't have permission to push to his/her.

Summary:
This adds the possibility to run MainUI under a subpath using a reverse proxy. For it to work, the reverse proxy needs to inject a cookie called "X-OPENHAB-BASEURL" with the subpath as its value.

Detailed explanation:
First, to simulate a reverse proxy with the Webpack devserver, apply the following patch:

--- a/bundles/org.openhab.ui/web/build/webpack.config.js
+++ b/bundles/org.openhab.ui/web/build/webpack.config.js
@@ -51 +51,3 @@ module.exports = {
-    historyApiFallback: true,
+    historyApiFallback: { index: '/proxypath/' },
+    publicPath: '/proxypath/',
+    headers: { "Set-Cookie": "X-OPENHAB-BASEURL=/proxypath;" },
@@ -56 +58,2 @@ module.exports = {
-      context: ['/auth', '/rest', '/chart', '/proxy', '/icon', '/static', '/changePassword', '/createApiToken'],
+      context: ['/proxypath/auth', '/proxypath/rest', '/proxypath/chart', '/proxypath/proxy', '/proxypath/icon', '/proxypath/static', '/proxypath/changePassword', '/proxypath/createApiToken', '/proxypath/habpanel', '/proxypath/basicui'],
+      pathRewrite: { '^/proxypath' : '' },

With this, MainUI should be reachable (and functional 😉) at http://localhost:8080/proxypath/.

How it works in detail, is that some js in index.html reads the cookie X-OPENHAB-BASEURL, stores its value in the global variable baseUrl and creates a <base href={baseUrl}> tag. This is to make all the images in index.html work, like favicon. Also, the app's main js gets added by scripting now (and not injected by Webpack at build time anymore). This is because the app.js would be loaded before the base tag is added, i.e. my browser always tried to load /js/app.js first and then {baseUrl}/js/app.js (which seems weird to me because it doesn't happen with the favicons etc., but it's just like that 🤷‍♂️). Sourcing the main js dynamically cleans that up.

When the app is loaded, the first thing it does is to assign the base url to the global variable __webpack_public_path__. This feature from Webpack is basically the main key and makes the application run under the base url, i.e. makes the app load all assets from underneath the base url. This assignment is exported to an extra file, because otherwise it wouldn't work properly with fonts (for some unknown reason. Felt like it to me hours to find this!).
Then for the Framework7 router and pushState to work properly with this, the base url gets assigned to pushStateRoot on load.
To fix communication with the openHAB API, all "static" urls are prepended by the base url. The same goes for the few manual "redirects" that set window.location.
That's basically it.

Limitations:
Due to the following open issues, until fixed, this PR can be considered as POC.

  • linking
    This is the main limitation, I think: If run under a subpath, "links" don't work anymore again. This seems to be a limitation from the design of the Framework7 router, which does make it possible to add the base url to links/routes (actually it might work if baseUrl is prepended to all router paths in the app, but that seems a little "undesirable" to me). Still, I think there might be quite some users that would drop this for being able to run MainUI subpathed.
    (edit) addition: the url in the browser's adress bar shows correctly, so creating links or bookmarks from there works just fine. It's just the tabbed browsing ("right-click-open-in-new-tab") that doesn't work.
  • other UIs
    this PR does not fix any issues with other UIs (yet), which are definitely present, e.g. in HABPanel
  • broken oauth authentication
    Authentication does not yet work with this. Though, to make it work only some slight changes in the core are required IMHO. Actually, the main limitation is within the core limiting redirects to a root url only. I actually don't quite understand why this is present, IMHO it's not required (but then I'm not a security expert). Also, the auth site from the core would need to redirect relatively (basically just change /auth to auth here).

Side notes
In this PR I've touched a few additional things:

  • all images are (IMHO) properly relocated, i.e. moved from res/ to images/
  • the basicui tile image has been moved to the basicui bundle (where it IMHO belongs 😉)
  • I don't know about the presence of cometvisu.png, so I removed it

Like said before, this PR is a POC. It doesn't yet touch the other UIs and it still needs a lot of testing on different clients (browsers, platforms i.e. iOS, ...) (BTW: @ghys is cordova actually being used and tested or can it be dropped?).

But before proceeding fixing open issues, with the above noted, I'd like to get feedback if this is something you'd actually like to go with and if there are maybe other limitations that could be showstoppers and need to be looked into first.

@relativeci
Copy link

relativeci bot commented Feb 9, 2022

Job #378: Bundle Size — 10.64MB (-0.53%).

314cc34 vs d53c94a

Changed metrics (4/10)
Metric Current Baseline
Initial JS 1.67MB(+0.02%) 1.67MB
Cache Invalidation 18.13% 29.81%
Assets 239(-0.42%) 240
Modules 1510(+0.07%) 1509
Changed assets by type (4/7)
            Current     Baseline
HTML 1.5KB (+26.83%) 1.19KB
IMG 85.23KB (-39.44%) 140.74KB
JS 8.64MB (-0.03%) 8.64MB
Other 858B (+1.42%) 846B

View Job #378 report on app.relative-ci.com

@ghys
Copy link
Member

ghys commented Feb 9, 2022

Sourcing the main js dynamically cleans that up.

You mean serving the index.html with a Java servlet that would replace a placeholder (because that could be an option) or what you describe afterwards?

When the app is loaded, the first thing it does is to assign the base url to the global variable webpack_public_path.

Eh, kudos for digging that one up, actually! 😲

(actually it might work if baseUrl is prepended to all router paths in the app, but that seems a little "undesirable" to me)

Why not? If it's done "cleanly" like all path: '/settings/things/:uid') replaced with path: getPath('/settings/things/:uid'), the function adding the prefix, that wouldn't really be too bad.

I actually don't quite understand why this is present, IMHO it's not required (but then I'm not a security expert).

The main UI stores the OAuth refresh_token in localStorage. The refresh_token is used to get access tokens which allow in turn to query protected endpoints in the API. If you were to have rogue JS that would "steal" the refresh_token from the localStorage (very easy on the same domain), then the API would be compromised. The trick is that sessions open by the main UI also set a cookie that are only valid for the root path and HttpOnly:

image
Sessions like this, which are flagged internally, require both that X-OPENHAB-SESSIONID cookie and the access_token for authorization. Since HttpOnly cookies are invisible to JS, stealing the request_token with JS wouldn't be enough because you don't know the session ID.

It's not perfect, but it makes it a little bit harder. It would be possible to set these cookies to the base path set by the X-OPENHAB-BASEPATH cookie value to make it work (there could be minor security implications).

(BTW: @ghys is cordova actually being used and tested or can it be dropped?).

It's still there in case we decide to resuscitate it but it doesn't work even now, so don't worry about it.

@hubsif
Copy link
Contributor Author

hubsif commented Feb 10, 2022

You mean serving the index.html with a Java servlet that would replace a placeholder (because that could be an option) or what you describe afterwards?

No, actually I mean what I described before 😄: Also, the app's main js gets added by scripting now (and not injected by Webpack at build time anymore).
Maybe for us developers code (and here) says more than words 😉.

Why not? If it's done "cleanly" like all path: '/settings/things/:uid') replaced with path: getPath('/settings/things/:uid'), the function adding the prefix, that wouldn't really be too bad.

It's not only about .navigate() calls (and if it was, I think a neat solution could be to just extend that method). It's also href= links in templates like here and maybe more.

The trick is that sessions open by the main UI also set a cookie that are only valid for the root path and HttpOnly.

Hm, not sure I can follow here. Afaik a cookie is valid for all subpaths as well, i.e. the X-OPENHAB-SESSIONID cookie is valid for the whole domain.
And I don't see how what you explained is related to limiting the oauth redirect url to root - the very same could be done with a subpath, couldn't it?
I probably have to think more about this to fully understand it ... 😄.

But before proceeding fixing open issues, with the above noted, I'd like to get feedback if this is something you'd actually like to go with and if there are maybe other limitations that could be showstoppers and need to be looked into first.

You missed to respond to this. From your response I assume there's general interest in finishing this PR?

@ghys
Copy link
Member

ghys commented Feb 10, 2022

Maybe for us developers code (and here) says more than words 😉.

Ok I see, not sure how I feel about this, seems like a hack tbh... normally I'd prefer avoiding these kind of things and stick the most natural way, but if it has no adverse consequences, why not.

It's not only about .navigate() calls (and if it was, I think a neat solution could be to just extend that method). It's also href= links in templates like here and maybe more.

Ah, I see. Count this as another problem with the f7 router... I suppose it was primarily designed for mobile apps.

But before proceeding fixing open issues, with the above noted, I'd like to get feedback if this is something you'd actually like to go with and if there are maybe other limitations that could be showstoppers and need to be looked into first.

You missed to respond to this. From your response I assume there's general interest in finishing this PR?

I have no fundamental issues with pursuing this, as offering the option to serve the web UI from a subpath is often encountered in all kinds of software, though one might argue that openHAB is somewhat "special" in that it comes with an application server and multiple UIs with very different codebases (Basic UI, HABPanel...).
As you remarked those have to be checked/fixed as well or you will encounter problems. Until then - and not only fixing the main UI - I'm not sure we could consider that putting openHAB as a whole behind a subpath is officially supported.

But I don't want to stifle your efforts, just thinking about the implications, which turn out to be quite a lot 🙂

@hubsif
Copy link
Contributor Author

hubsif commented Feb 18, 2022

So, I performed some basic tests for MainUI using Firefox and Chrome, iOS 14, Android 10 and to me it seems all good.

I've had a closer look at authentication and I still don't understand how auth is supposed to work. I thought it's somewhat oauth based, but then I can't see why client_id and redirect_url are bound to each other, like in if (!clientId.equals(baseRedirectUri)). Actually, according to oauth guide, client_id should even not be "guessable".

Cause like said I'm not too deep into security, I didn't dig any deeper and just made it work with this quick hack:

openhab-core auth patch
diff --git a/bundles/org.openhab.core.io.http.auth/src/main/java/org/openhab/core/io/http/auth/internal/AuthorizePageServlet.java b/bundles/org.openhab.core.io.http.auth/src/main/java/org/openhab/core/io/http/auth/internal/AuthorizePageServlet.java
index 763401792..67370da7a 100644
--- a/bundles/org.openhab.core.io.http.auth/src/main/java/org/openhab/core/io/http/auth/internal/AuthorizePageServlet.java
+++ b/bundles/org.openhab.core.io.http.auth/src/main/java/org/openhab/core/io/http/auth/internal/AuthorizePageServlet.java
@@ -136 +136 @@ public class AuthorizePageServlet extends AbstractAuthPageServlet {
-            String clientId = params.get("redirect_uri")[0];
+            String clientId = params.get("client_id")[0];
@@ -143,4 +142,0 @@ public class AuthorizePageServlet extends AbstractAuthPageServlet {
-            if (!clientId.equals(baseRedirectUri)) {
-                throw new IllegalArgumentException("unauthorized_client");
-            }
-
@@ -210 +206 @@ public class AuthorizePageServlet extends AbstractAuthPageServlet {
-        responseBody = responseBody.replace("{formAction}", "/auth");
+        responseBody = responseBody.replace("{formAction}", "auth");
diff --git a/bundles/org.openhab.core.io.rest.auth/src/main/java/org/openhab/core/io/rest/auth/internal/TokenResource.java b/bundles/org.openhab.core.io.rest.auth/src/main/java/org/openhab/core/io/rest/auth/internal/TokenResource.java
index f596e8cbb..30b012b3f 100644
--- a/bundles/org.openhab.core.io.rest.auth/src/main/java/org/openhab/core/io/rest/auth/internal/TokenResource.java
+++ b/bundles/org.openhab.core.io.rest.auth/src/main/java/org/openhab/core/io/rest/auth/internal/TokenResource.java
@@ -247,2 +247,2 @@ public class TokenResource implements RESTResource {
-                NewCookie newCookie = new NewCookie(SESSIONID_COOKIE_NAME, null, "/", domainUri.getHost(), null, 0,
-                        false, true);
+                NewCookie newCookie = new NewCookie(SESSIONID_COOKIE_NAME, null, domainUri.getPath(),
+                        domainUri.getHost(), null, 0, false, true);
@@ -350,6 +350,7 @@ public class TokenResource implements RESTResource {
-                if (!("".equals(domainUri.getPath()) || "/".equals(domainUri.getPath()))) {
-                    throw new IllegalArgumentException(
-                            "Will not honor the request to set a session cookie for this client, because it's only allowed for root redirect URIs");
-                }
-                NewCookie newCookie = new NewCookie(SESSIONID_COOKIE_NAME, sessionId, "/", domainUri.getHost(), null,
-                        2147483647, false, true);
+                // if (!("".equals(domainUri.getPath()) || "/".equals(domainUri.getPath()))) {
+                // throw new IllegalArgumentException(
+                // "Will not honor the request to set a session cookie for this client,
+                // because it's only allowed for root redirect URIs");
+                // }
+                NewCookie newCookie = new NewCookie(SESSIONID_COOKIE_NAME, sessionId, domainUri.getPath(),
+                        domainUri.getHost(), null, 2147483647, false, true);
With this I was able to authenticate during all my tests.

I also had a look at HABPanel and as it seems HABPanel/Angular 1 seems to use only a single, never changing url path (index.html) and hashes for routing. By making all urls to the core relative as in ../ all my basic tests succeeded. Though this might need further testing, as I don't really have a (sophisticated) dashboard.

All in all I'd say this PR is finished for me regarding MainUI and HABPanel. With required auth changes made to the core, I think it could be merged. openHAB might then still not "officially" support reverse proxying, but since these changes don't break anything they would help those many tinkerers out there subpathing openHAB on their own responsibility 😉.

@hubsif hubsif marked this pull request as ready for review February 18, 2022 16:08
@hubsif hubsif requested review from ghys and a team as code owners February 18, 2022 16:08
@hubsif
Copy link
Contributor Author

hubsif commented Feb 22, 2022

Ah, forgot something: While testing I found PWA to not work. Has that actually worked before?

@ghys
Copy link
Member

ghys commented Mar 3, 2022

While testing I found PWA to not work. Has that actually worked before?

Very much so. I'm open to alternatives as long as they don't hinder the ability to "add as app" PWA functionality in the default case.

hubsif added 2 commits April 5, 2022 20:26
Signed-off-by: Hubert Nusser <hubsif@gmx.de>
Signed-off-by: Hubert Nusser <hubsif@gmx.de>
@hubsif
Copy link
Contributor Author

hubsif commented Apr 5, 2022

So, I finally found some time to check PWA.
It actually still works just fine for default installations under root.
Unfortunately, the used WorkboxPlugin does not support the dynamic __webpack_public_path__, which makes PWA break as soon as the UI is run under a subpath.

So, the two limitations left are non-working links and non-working PWA, which I think is acceptable for many users that want to "inofficially" subpath the mainUI.

@ghys
Copy link
Member

ghys commented Apr 5, 2022

Alright, thanks for the investigations, my main priorities for acceptability of this subpath feature are:

  • it should work as before with no drawbacks in the current default scenario, and if you go the subpath route it should be made clear that this is not completely supported yet (e.g. this or that UIs might break) and therefore should clearly be stated in the config description and/or the docs as BETA - until everything is addressed, that is.
  • it should not complexify the code too much to accommodate a scenario that is not destined to be "mainstream" (some other software might offer this option but they probably only have a single UI and not a full application server with multiple UIs like openHAB so the stakes are different).

@ghys
Copy link
Member

ghys commented Apr 5, 2022

To be clear - the PR looks great so far and you'll get more feedback from me when I get some time to test it properly!

@hubsif
Copy link
Contributor Author

hubsif commented Apr 6, 2022

my main priorities for acceptability of this subpath feature are:

I fully agree to both. The latter actually being the reason why I didn't pursue fixing the linking issue...

To be clear - the PR looks great so far and you'll get more feedback from me when I get some time to test it properly!

Sure, take your time.

@AshleighTheCutie
Copy link

Any update on this? I'm having to move away from Home Assistant due to this exact issue, and now I'm seeing OpenHAB doesn't support it either.

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.

3 participants