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

Fix rest api access when using script/develop_and_serve #23144

Conversation

martetassyns
Copy link
Contributor

Breaking change

Proposed change

The script/develop_and_server only took into account UI components that connected through the websocket.
Now support has also been added for those that use the rest api.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Tested through the System / Backup functionality

Additional information

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

script/serve.js Fixed Show fixed Hide fixed
@bramkragten
Copy link
Member

bramkragten commented Dec 6, 2024

You create a proxy, but it is never used? All the API calls are still done directly to the external HA instance and not to the proxy.

https://github.com/martetassyns/home-assistant-frontend/blob/b6ca770f38e5e4a1563a02505b86a6c3dbe31811/src/util/hass-call-api.ts#L58

You can test this easily with the config flows, as they use a REST API

@martetassyns martetassyns marked this pull request as draft December 6, 2024 13:51
script/serve.js Fixed Show fixed Hide fixed
@martetassyns
Copy link
Contributor Author

martetassyns commented Dec 6, 2024

@ bramkragten ok, I have confirmed that to use /config/integrations/integration/hacs you also need to configure cors. I will eventually look into seeing if we can improve this, but for now I just documented that it needs to be done. Can you agree that this is sufficient for this change? (as before this change it was also a requirement)

Note that the proxy stuff is still needed, as for example /hassio/backups uses a different way to access the api (through relative url's) and completely ignores the $hassUrl.

@martetassyns martetassyns marked this pull request as ready for review December 6, 2024 14:16
@martetassyns
Copy link
Contributor Author

@bramkragten ok, ignore my earlier comment. I thought it would a lot of work to get the login flow to work properly when everything would be proxied through the frontend dev instance. But it appears it was rather easy. There is no longer a requirement to configure cors now.

script/serve.js Dismissed Show dismissed Hide dismissed
script/serve.js Dismissed Show dismissed Hide dismissed
script/serve.js Dismissed Show dismissed Hide dismissed
@bramkragten
Copy link
Member

You create a proxy, but it is never used? All the API calls are still done directly to the external HA instance and not to the proxy.

martetassyns/home-assistant-frontend@b6ca770/src/util/hass-call-api.ts#L58

You can test this easily with the config flows, as they use a REST API

I still don't see this addressed, the API calls are still done directly to the external host and not through the proxy.

Check config flows (add integration, integration options), automation editor, script editor, scene editor, etc

@martetassyns
Copy link
Contributor Author

@bramkragten the change the fixes it is that we no longer override the HASS_URL when calling script/develop in script/develop_and_serve, all calls now go through the development frontend server.

Comment on lines +34 to +52
const coreProxy = createProxyMiddleware({
target: coreUrl,
changeOrigin: true,
ws: true,
});
const app = express();
app.use("/", express.static(path.join(repoDir, "hass_frontend")));
app.use("/api/hassio/app", express.static(path.join(repoDir, "hassio/build")));
app.use("/api", coreProxy);
app.get("/auth/authorize", (req, res) => {
res.sendFile(path.join(repoDir, "hass_frontend/authorize.html"));
});
app.use("/auth", coreProxy);
app.get("/onboarding", (req, res) => {
res.sendFile(path.join(repoDir, "hass_frontend/onboarding.html"));
});
app.get("*", (req, res) => {
res.sendFile(path.join(repoDir, "hass_frontend/index.html"));
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const coreProxy = createProxyMiddleware({
target: coreUrl,
changeOrigin: true,
ws: true,
});
const app = express();
app.use("/", express.static(path.join(repoDir, "hass_frontend")));
app.use("/api/hassio/app", express.static(path.join(repoDir, "hassio/build")));
app.use("/api", coreProxy);
app.get("/auth/authorize", (req, res) => {
res.sendFile(path.join(repoDir, "hass_frontend/authorize.html"));
});
app.use("/auth", coreProxy);
app.get("/onboarding", (req, res) => {
res.sendFile(path.join(repoDir, "hass_frontend/onboarding.html"));
});
app.get("*", (req, res) => {
res.sendFile(path.join(repoDir, "hass_frontend/index.html"));
});
const indexHtml = fs.readFileSync(path.join(repoDir, "hass_frontend", "index.html"), "utf8")
.replace(/<\/body>/g, `<script>
const interval = setInterval(() => {
window.hassConnection.then(conn => {
conn.auth.data.hassUrl = "${coreUrl}";
clearInterval(interval);
});
}, 100);
</script>
</body>`);
const coreProxy = createProxyMiddleware({
target: coreUrl,
changeOrigin: true,
ws: true,
});
const app = express();
app.get("/index.html", (req, res) => {
res.send(indexHtml);
});
app.use(express.static(path.join(repoDir, "hass_frontend")));
app.use(express.static(path.join(repoDir, "hass_frontend", "frontend_latest")));
app.use("/api/hassio/app", express.static(path.join(repoDir, "hassio/build")));
app.use("/api", coreProxy);
app.get("/auth/authorize", (req, res) => {
res.sendFile(path.join(repoDir, "hass_frontend/authorize.html"));
});
app.use("/auth", coreProxy);
app.get("/onboarding", (req, res) => {
res.sendFile(path.join(repoDir, "hass_frontend/onboarding.html"));
});
app.get("*", (req, res) => {
res.send(indexHtml);
});

This works but is very dirty. The problem is that the API url comes from the hassConnection.auth.data.hassUrl

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have to change the index, we should do it inside the template with a flag, also, why does it use an interval?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interval was the quickest dirty solution to test & show what needs to happen as hassConnection is not initialized when this script tag runs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MindFreeze why was this extension needed? it worked just fine with proxying everything?

@bramkragten
Copy link
Member

We have done some more thinking and found a different way easier solution, setting allowed cors origins on the core instance 🙈

https://www.home-assistant.io/integrations/http/#cors_allowed_origins

So this PR is no longer needed, we just need to document to set the above for http://localhost:8124

@martetassyns
Copy link
Contributor Author

@bramkragten I disagree that having a proxy for the connections is not a benefit., it is still required because not all functionality uses the value from the login session to determine which /api/ to contact. For example the System/ Backup functionality (which connects directly to /api/... relative paths.

@MindFreeze
Copy link
Contributor

Setting CORS just works. A proxy is very complicated and very hacky. It's essentially a man in the middle attack. Also requires new dependancies

@martetassyns
Copy link
Contributor Author

martetassyns commented Dec 12, 2024

@MindFreeze @bramkragten Does that mean that every part of the code that uses /api/... as a relative path is wrong and should be reworked to use hass.auth.data.hassUrl instead? I thought there was only one, but a quick search on /api in the code base shows quite some places where it's used relative instead.

Some non obvious examples, loading custom panels and translations is done from relative path. Meaning that nothing of the hassio functionality works without fixing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants