8000 Allow homeassistant in MQTT configuration_url schema by jbouwh · Pull Request #96107 · home-assistant/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Allow home 8000 assistant in MQTT configuration_url schema #96107

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jul 22, 2023
Merged

Allow homeassistant in MQTT configuration_url schema #96107

merged 5 commits into from
Jul 22, 2023

Conversation

jbouwh
Copy link
Contributor
@jbouwh jbouwh commented Jul 7, 2023 •
edited
Loading

Proposed change

Allow homeassistant in URL schema for configuration URL's. This can be used by add on's that use hassio_ingress and expose configuration URL's that share the Home Assistant base URL.

The frontend will automatically rewite URL's with the homeassistant schema.

Example: homeassistant::/45df7312_zigbee2mqtt/ingress?next=/#/device/0x70ac08fffe87636c/info will be rewritten in the frontend to: http://ha.local:8123/45df7312_zigbee2mqtt/ingress?next=/#/device/0x70ac08fffe87636c/info. Assuming http://ha.local:8123 is your Home Assistant base URL.

Currently there is a frontend bug with hassio_ingress that will fail ingress authentication. Other URL's should work fine.

Type of change

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

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

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

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

@jbouwh jbouwh requested a review from a team as a code owner July 7, 2023 16:50
@home-assistant home-assistant bot added cla-signed core new-feature small-pr PRs with less than 30 lines. labels Jul 7, 2023
@jbouwh jbouwh mentioned this pull request Jul 7, 2023
Closed
14 tasks
frenck
frenck requested changes Jul 7, 2023
View reviewed changes
@home-assistant
Copy link
home-assistant bot commented Jul 7, 2023

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@home-assistant home-assistant bot marked this pull request as draft July 7, 2023 16:53
@jbouwh jbouwh marked this pull request as ready for review July 7, 2023 17:23
@jbouwh jbouwh requested a review from emontnemery as a code owner July 7, 2023 17:23
@home-assistant home-assistant bot requested a review from frenck July 7, 2023 17:23
@fthiery fthiery mentioned this pull request Jul 7, 2023
Closed
@jbouwh jbouwh mentioned this pull request Jul 7, 2023
Merged
8 tasks
@fthiery
Copy link
Contributor
fthiery commented Jul 10, 2023 •
edited
Loading

Hello, i tested this feature by patching the mqtt component (with vol.Optional(CONF_CONFIGURATION_URL): cv.string,) and deploying it as custom_component, then patching the zigbee2mqtt container to announce the configuration_url with the homeassistant:// prefix.

The Visit button gets the proper URL (e.g. http://ha.local:8123/api/hassio_ingress/pqvlFsbQ9EgVr5dr7SyXhXfDePmGOxjQ6UTZWoPRy5g/#/device/0xa4c138f1f4925cce/info) but the link does not open a new window and it doesn't work (it seems to redirect to the default dashboard http://ha.local:8123/lovelace/default). However, opening the link in a new tab (e.g. middle mouse click) does work (i.e. the Z2M UI appears). [EDIT: only after using the ingress UI once, it turns out]

I re-tested with relative URLs (e.g. "configuration_url":"/api/hassio_ingress/pqvlFsbQ9EgVr5dr7SyXhXfDePmGOxjQ6UTZWoPRy5g/#/device/0x70ac08fffe87636c/info") and there the link opens in a new tab and it works as expected [EDIT: only after using the ingress UI once, it turns out].

The frontend seems to be specifically designed to do this: https://github.com/home-assistant/frontend/blob/b1a909d30299486532f7ef683566295d3a282676/src/panels/config/devices/ha-config-device-page.ts#L1074

@frenck how can we have the UI open homeassistant:// links in a new tab (e.g. for a specific sub-path, like /api/hassio_ingress/) ?

Also, after testing if i do not open the ingress URL once before cliking "Visit URL", i get a 401 error. I guess that setIngressCookie is not called when opening a /api/hassio_ingress URL, so that's probably not going to work at all until the user opens the addon's ingress page...

So that probably means that lovelace must be modified to detect that the user is clicking some ingress-related homeassistant:// link, set the cookie and open the link in a new page, or even better embed the ingress subpage.

Is there a way / API to pass a path to the ingress iframe UI URL like http://ha.local:8123/45df7312_zigbee2mqtt/ingress ? E.g. http://ha.local:8123/45df7312_zigbee2mqtt/ingress?next=/#/device/0x70ac08fffe87636c/info" ?

@frenck
Copy link
Member
frenck commented Jul 11, 2023 •
edited
Loading

@frenck how can we have the UI open homeassistant:// links in a new tab (e.g. for a specific sub-path, like /api/hassio_ingress/) ?

We don't. They are internal URLs not external ones.

Also, after testing if i do not open the ingress URL once before cliking "Visit URL", i get a 401 error. I guess that setIngressCookie is not called when opening a /api/hassio_ingress URL, so that's probably not going to work at all until the user opens the addon's ingress page...

This is an known frontend issue, Grafana also has issues like this. See the issue tracker of the frontend for more information.

Is there a way / API to pass a path to the ingress iframe UI URL like

no.

../Frenck

emontnemery
emontnemery previously requested changes Jul 12, 2023
View reviewed changes
@home-assistant home-assistant bot marked this pull request as draft July 12, 2023 06:50
@jbouwh jbouwh marked this pull request as ready for review July 12, 2023 07:29
@home-assistant home-assistant bot requested a review from emontnemery July 12, 2023 07:29
@fthiery
Copy link
Contributor
fthiery commented Jul 12, 2023 •
edited
Loading

Also, after testing if i do not open the ingress URL once before cliking "Visit URL", i get a 401 error. I guess that setIngressCookie is not called when opening a /api/hassio_ingress URL, so that's probably not going to work at all until the user opens the addon's ingress page...
This is an known frontend issue, Grafana also has issues like this. See the issue tracker of the frontend for more information.

Which issue are you referring to ? I tried https://github.com/home-assistant/frontend/issues?q=is%3Aissue+is%3Aopen+grafana but found nothing

@jbouwh if the frontend needs heavy changes for this feature to be useful (the initial need was to target an ingress URL), should this PR still be pursued ? Or should i open a related issue on the Frontend project (related to initializing the ingress session when clicking the Visit button, and possibly displaying the iframe like when accessing the addons UI) ?

@fthiery
Copy link
Contributor
fthiery commented Jul 12, 2023

Also, should this MR not be named "Allow homeassistant in mqtt configuration_url schema" ?

@jbouwh jbouwh changed the title Allow homeassistant in URL schema Allow homeassistant in MQTT configuration_url schema Jul 12, 2023
@jbouwh
Copy link
Contributor Author
jbouwh commented Jul 12, 2023

@jbouwh if the frontend needs heavy changes for this feature to be useful (the initial need was to target an ingress URL), should this PR still be pursued ? Or should i open a related issue on the Frontend project (related to initializing the ingress session when clicking the Visit button, and possibly displaying the iframe like when accessing the addons UI) ?

Just tested this in dev and it seems there is no frontend change needed. The frontend translates the schema correctly.

@fthiery
Copy link
Contributor
fthiery commented Jul 12, 2023

@jbouwh if the frontend needs heavy changes for this feature to be useful (the initial 8000 need was to target an ingress URL), should this PR still be pursued ? Or should i open a related issue on the Frontend project (related to initializing the ingress session when clicking the Visit button, and possibly displaying the iframe like when accessing the addons UI) ?

Just tested this in dev and it seems there is no frontend change needed. The frontend translates the schema correctly.

I confirm it works, but my initial usecase which was to access an ingress URL does not work without frontend modifications. The only way it works currently is to 1) access the ingress UI at least once 2) open the "Visit" link in a new tab.

Do you see any alternative usecases for homeassistant://-prefixed configuration_urls ?

@jbouwh
Copy link
Contributor Author
jbouwh commented Jul 12, 2023

@jbouwh if the frontend needs heavy changes for this feature to be useful (the initial need was to target an ingress URL), should this PR still be pursued ? Or should i open a related issue on the Frontend project (related to initializing the ingress session when clicking the Visit button, and possibly displaying the iframe like when accessing the addons UI) ?

Just tested this in dev and it seems there is no frontend change needed. The frontend translates the schema correctly.

I confirm it works, but my initial usecase which was to access an ingress URL does not work without frontend modifications. The only way it works currently is to 1) access the ingress UI at least once 2) open the "Visit" link in a new tab.

Do you see any alternative usecases for homeassistant://-prefixed configuration_urls ?

Not directly, but we could wait merging this one till the ingress frontend issues are solved?

@jbouwh jbouwh marked this pull request as draft July 12, 2023 08:01
@fthiery
Copy link
Contributor
fthiery commented Jul 12, 2023

This is an known frontend issue, Grafana also has issues like this. See the issue tracker of the frontend for more information.

Found it in discussions, i'll chime in: home-assistant/frontend#11273

Not directly, but we could wait merging this one till the ingress frontend issues are solved?

I think so too

@frenck
Copy link
Member
frenck commented Jul 22, 2023

Not directly, but we could wait merging this one till the ingress frontend issues are solved?

I think we could continue on this one. While there is an ingress bug, I don't see a reason to block it. One could (in theory), also link to a config panel or integration page.

@jbouwh jbouwh marked this pull request as ready for review July 22, 2023 15:28
@jbouwh jbouwh marked this pull request as draft July 22, 2023 15:48
@jbouwh jbouwh force-pushed the add-homeassistant-schema-as-a-valid-url branch from 2c3887f to c412d0d Compare July 22, 2023 15:55
@jbouwh jbouwh marked this pull request as ready for review July 22, 2023 15:55
@frenck frenck added the smash Indicator this PR is close to finish for merging or closing label Jul 22, 2023
frenck
frenck approved these changes Jul 22, 2023
View reviewed changes
Copy link
Member
@frenck frenck left a comment

Choose a reason for hiding this comment

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

Thanks, @jbouwh 👍

../Frenck

@frenck frenck dismissed emontnemery’s stale review July 22, 2023 20:50

Comments addressed

@frenck frenck merged commit 9424d11 into home-assistant:dev Jul 22, 2023
@jbouwh jbouwh deleted the add-homeassistant-schema-as-a-valid-url branch July 22, 2023 20:51
@github-actions github-actions bot locked and limited conversation to collaborators Jul 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@fthiery fthiery fthiery left review comments

@frenck frenck frenck approved these changes

@emontnemery emontnemery Awaiting requested review from emontnemery emontnemery is a code owner

Assignees
No one assigned
Labels
by-code-owner cla-signed core new-feature small-pr PRs with less than 30 lines. smash Indicator this PR is close to finish for merging or closing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jbouwh @fthiery @frenck @emontnemery
0