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

auth_resp_x_vouch_user is always homeassistant for all users with home assistant provider #565

Open
ivanjx opened this issue Jun 27, 2024 · 5 comments · May be fixed by #566
Open

auth_resp_x_vouch_user is always homeassistant for all users with home assistant provider #565

ivanjx opened this issue Jun 27, 2024 · 5 comments · May be fixed by #566

Comments

@ivanjx
Copy link

ivanjx commented Jun 27, 2024

Describe the problem
i am trying to implement basic authorization per domain with openresty following the examples provided in this repo. however for all the users on my home assistant, the auth_resp_x_vouch_user is always homeassistant.

Expected behavior
auth_resp_x_vouch_user should be the same as the home assistant username.

Desktop (please complete the following information):

  • OS: Linux
  • Browser: Brave
  • Version: 1.67.116

Additional context
configs and scripts: https://gist.github.com/ivanjx/1a08d6e0413cb6b9bb8302ebe7db9de1

nginx log output:

2024/06/27 17:16:22 [error] 7#7: *12 [lua] user_auth.lua:28: VOUCH USER GNAME: homeassistant!
@ivanjx
Copy link
Author

ivanjx commented Jun 29, 2024

sorry for not looking into the HA integration code before opening this issue. i realized that HA does not even provide an api to query the username. however i am trying to merge changes that could make this happen.

home-assistant/core#120811

@ivanjx ivanjx linked a pull request Jun 30, 2024 that will close this issue
@bnfinet
Copy link
Member

bnfinet commented Jul 9, 2024

@ivanjx thanks for contributing to VP and improving the userinfo endpoint for HA

Are you using this successfully?

I'll give it a more thorough review when I have some time to look at VP stuff (probably September)

@ivanjx
Copy link
Author

ivanjx commented Jul 10, 2024

yes i have been using it for weeks now without any problems. the prebuilt docker container is also on my github repo packages.

@nana4rider
Copy link

@ivanjx
Hello, nice to meet you.
I’m happy to have found this issue. It’s great that we can now retrieve user information from Home Assistant!

I reviewed this PR, and I think it would be more appropriate to set customClaims to either the entire result object from responseMessage or a separately created flattened structure.

Reason

  • It looks like headers.claims does not support nested keys such as result.id.
  • If we specify headers.claims: ["result"], we get a stringify map like this, which is difficult to handle:
    map[credentials:[map[auth_provider_id:<nil> auth_provider_type:homeassistant] map[auth_provider_id:<nil> auth_provider_type:auth_header]] id:xxxxxxxxxxxxxxx is_admin:true is_owner:true mfa_modules:[map[enabled:true id:totp name:Authenticator app]] name: Test User]
  • If result is passed directly to customClaims, it would allow headers.claims to easily retrieve values such as id, is_owner, and is_admin.

I apologize if my English is not perfect, as I am posting this through translation.
Thank you for your consideration.

@ivanjx
Copy link
Author

ivanjx commented Feb 23, 2025

hi @nana4rider

indeed. i wasnt really aware that the response was wrapped and not the actual user info itself. i believe that was because i mistakenly following the other provider implementations. since the common.MapClaims only accept raw unserialized data, we must put the claims manually from the data.Result.

that being said, it has been so long after i wrote this PR that i no longer use nginx so i have moved to other solutions. i also use google oauth instead of HA currently because of how limited and restrictive HA currently in terms of authentication.

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

Successfully merging a pull request may close this issue.

3 participants