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

Allows the supervisor to send a session's user to addon with header X-Remote-User #88472

Merged
merged 16 commits into from Aug 22, 2023

Conversation

baflo
Copy link
Contributor

@baflo baflo commented Feb 20, 2023

Related PRs

#88472
home-assistant/frontend#15505
home-assistant/supervisor#4152

Proposed change

This implements a feature that allows the supervisor to send a session's username to an addon. The hassio component will add the username in the body if the command is /ingress/session.

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

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

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
  • 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:

@home-assistant
Copy link

Hey there @home-assistant/supervisor, mind taking a look at this pull request as it has been labeled with an integration (hassio) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of hassio can trigger bot actions by commenting:

  • @home-assistant close Closes the issue.
  • @home-assistant rename Awesome new title Change the title of the issue.
  • @home-assistant reopen Reopen the issue.
  • @home-assistant unassign hassio Removes the current integration label and assignees on the issue, add the integration domain after the command.

@balloob
Copy link
Member

balloob commented Feb 20, 2023

Can you explain how you plan on this being used?

@baflo
Copy link
Contributor Author

baflo commented Feb 21, 2023

Can you explain how you plan on this being used?

As the documentation for addon development already promises, addons get authentication "for free" by the home assistant ingress.

However, the addons currently don't know what user is authenticated. With this feature Home Assistant works like a SSO service and allows addons to maintain different users.

This could e.g. be used by RaspberryMatic for different users with different permissions or by paperless-ngx to manage documents for select users.

This was also requested by multiple threads in the forum:
https://community.home-assistant.io/t/hassio-ingress-header-for-user-name-exist/202291
https://community.home-assistant.io/t/expose-the-active-home-assistant-user-to-the-addon-through-ingress/221144

Copy link
Member

@pvizeli pvizeli left a comment

Choose a reason for hiding this comment

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

I think that make sense for SSO. I had the same idea but not time for it

@balloob
Copy link
Member

balloob commented Feb 22, 2023

The name of the user is not static, and can be changed by the user. I think that we should also include the user ID and recommend that to be used as the identifier for the user.

@baflo
Copy link
Contributor Author

baflo commented Feb 24, 2023

The name of the user is not static, and can be changed by the user. I think that we should also include the user ID and recommend that to be used as the identifier for the user.

I find a user ID and two usernames on the user object. I understand I shouldn't use the displayed username, as that one is obviously not static.

Is the "login" username also considered non-static? I would prefer to use that to send to addons. However, if only the actual ID is static, I'll change it to send that.

EDIT: nvm, just understood the login-username is just one of many possibble auth-possibilities. So it must be the ID.

image

Copy link
Member

@pvizeli pvizeli left a comment

Choose a reason for hiding this comment

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

Needs Supervisor PR merged as well

Copy link
Member

@pvizeli pvizeli left a comment

Choose a reason for hiding this comment

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

NVM, we use _ for API data

@@ -9,6 +9,7 @@
ATTR_COMPRESSED = "compressed"
ATTR_CONFIG = "config"
ATTR_DATA = "data"
ATTR_SESSION_DATA_USER_ID = "user-id"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ATTR_SESSION_DATA_USER_ID = "user-id"
ATTR_SESSION_DATA_USER_ID = "user_id"

@home-assistant home-assistant bot marked this pull request as draft March 1, 2023 09:38
@home-assistant
Copy link

home-assistant bot commented Mar 1, 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.

@balloob
Copy link
Member

balloob commented Mar 1, 2023

I'm actually wondering now, why don't we just add the hass user ID + is-admin header to every ingress request we forward? that way we don't need a new API.

@pvizeli
Copy link
Member

pvizeli commented Mar 1, 2023

I'm actually wondering now, why don't we just add the hass user ID + is-admin header to every ingress request we forward? that way we don't need a new API.

It's about the session, giving the possibility to explicitly give access to add-ons at the user base. We should touch the header of forwarding stuff less as possible. For the API itself, yeah we can forward such information and also protocol what or who is using it. But now you mix 2 things together. The API is body driven for features and access are header driven.

@balloob
Copy link
Member

balloob commented Mar 1, 2023

Had a quick call with Pascal. He is right, we should have this information stored in the session itself, as it unlocks other features like user-based access control for ingress. It also should be set here, and not in the frontend as we do not want to allow for impersonation.

@baflo baflo marked this pull request as ready for review May 17, 2023 12:53
@baflo baflo requested a review from a team as a code owner May 17, 2023 12:53
@home-assistant home-assistant bot requested a review from pvizeli May 17, 2023 12:53
@github-actions
Copy link

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@github-actions github-actions bot added the stale label Aug 15, 2023
@baflo
Copy link
Contributor Author

baflo commented Aug 16, 2023

Not stale :)

@github-actions github-actions bot removed the stale label Aug 16, 2023
@pvizeli pvizeli merged commit 00b75ce into home-assistant:dev Aug 22, 2023
3 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants