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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Force fitbit OAuth setup to use external url #43773

Closed
wants to merge 1 commit into from

Conversation

3v1n0
Copy link
Contributor

@3v1n0 3v1n0 commented Nov 30, 2020

Proposed change

In order to be linked with fitbit account we need to use HA external
url, while current implementation might cause internal uri usage which
of course won't work for OAuth challenge

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)
  • 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.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 馃 Silver
  • 馃 Gold
  • 馃弳 Platinum

To help with the load of incoming pull requests:

In order to be linked with fitbit account we need to use HA external
url, while current implementation might cause internal uri usage which
of course won't work for OAuth challenge
@project-bot project-bot bot added this to Needs review in Dev Nov 30, 2020
Comment on lines +170 to +172
allow_internal=False,
allow_ip=False,
require_ssl=True,
Copy link
Member

Choose a reason for hiding this comment

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

This is incorrect. OAuth specification does not require an external URL. Furthermore, because of NAT loopback issues, not everybody can use an external URL, to begin with. Hence, doing that causes different issues.

IP based URLs, Non-SSL & SSL based URLs and internal and external URLs are all valid to use for OAuth authentication.

The current stable version actually looks at the current URL you are using in your browser (and must have been configured in your instance). The dev version diverts a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, on redirect/callback URL it must be external, no? How the service can access to that otherwise?

Copy link
Member

Choose a reason for hiding this comment

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

The service does not access that. With OAuth the applications do not talk directly to each other. Your browser does that via redirects. Hence, it doesn't have to be public (as long as your browser can access it).

Dev automation moved this from Needs review to Review in progress Nov 30, 2020
@frenck
Copy link
Member

frenck commented Nov 30, 2020

What actually needs to happen, is that this integration needs to be migrated onto a config flow using the OAuth2 helpers, which will actually automatically start using the actual URL the client has in their browser (as of the next Home Assistant release).

@3v1n0
Copy link
Contributor Author

3v1n0 commented Nov 30, 2020

I see, well using latest stable without this I was just getting my local IP address that was rejected by fitbit as wrong redirect URL, so that's the only way to get it working

@frenck
Copy link
Member

frenck commented Nov 30, 2020

wrong redirect URL

That means the redirect URL in the application you have registered @ FitBit, is not matching. This is a configuration error. Not a bug.

@MartinHjelmare MartinHjelmare changed the title fitbit: Force using external url for OAuth setup Force fitbit OAuth setup to use external url Dec 1, 2020
@MartinHjelmare
Copy link
Member

I'll close here now. Still, thanks for your contribution!

We'd love to see a PR to migrate this integration to use a config flow and config entries.

https://developers.home-assistant.io/docs/config_entries_config_flow_handler#configuration-via-oauth2

Dev automation moved this from Review in progress to Cancelled Dec 1, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Dec 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Cancelled
Development

Successfully merging this pull request may close these issues.

None yet

4 participants