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.

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

Point add redirect uri #101967

Closed
wants to merge 4 commits into from
Closed

Point add redirect uri #101967

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ee052bd
close window after auth
fredrike Oct 13, 2023
5a5904c
updated config_flow
fredrike Oct 13, 2023
6546224
update tests
fredrike Oct 15, 2023
6264b0a
full coverage, simplify user_flow
fredrike Oct 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
127 changes: 64 additions & 63 deletions homeassistant/components/point/config_flow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,17 @@
from collections import OrderedDict
import logging

from pypoint import PointSession
from aiohttp import web_response
from pypoint import MINUT_AUTH_URL, PointSession
import voluptuous as vol

from homeassistant import config_entries
from homeassistant import config_entries, data_entry_flow
from homeassistant.components.http import HomeAssistantView
from homeassistant.const import CONF_CLIENT_ID, CONF_CLIENT_SECRET
from homeassistant.core import callback
from homeassistant.helpers.aiohttp_client import async_get_clientsession

from .const import DOMAIN
from .const import CONF_REDIRECT_URI, DOMAIN

AUTH_CALLBACK_PATH = "/api/minut"
AUTH_CALLBACK_NAME = "api:minut"
Expand Down Expand Up @@ -44,109 +45,106 @@ class PointFlowHandler(config_entries.ConfigFlow, domain=DOMAIN):
"""Handle a config flow."""

VERSION = 1
code: str | None = None

@property
def schema(self):
"""Return current schema."""
return vol.Schema(
{
vol.Required(CONF_REDIRECT_URI): str,
Copy link
Member

Choose a reason for hiding this comment

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

Wait, why doesn't this implement an OAuth flow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes but not using home-assistant's oauth.

I've been trying to implement using the guidelines but I don't quite understand all parts and have not found an integration that does it by the book either.

The userbase for Point is quite small, perhaps it is better to pull this integration and use hacs instead.

Copy link
Member

Choose a reason for hiding this comment

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

I've been trying to implement using the guidelines but I don't quite understand all parts and have not found an integration that does it by the book either.

There are many integrations doing oauth flows? They all use the same constructs.

The thing is, the redirect URL should not even be a question... With the default flows, it will handle that for the user.

perhaps it is better to pull this integration and use hacs instead.

I do not understand what the latter reasoning has to do with anything? Even as a HACS integration, one should be using the right constructs, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are many integrations doing oauth flows? They all use the same constructs.

Could you please name a few for reference.

perhaps it is better to pull this integration and use hacs instead.

I do not understand what the latter reasoning has to do with anything? Even as a HACS integration, one should be using the right constructs, right?

The reasoning is that I don't feel I have the time to keep up with the high code quality standards. I did another PR #100843 but that was rejected due to 1. I updated the yaml 2. Once I've rewritten the flow to not use yaml the pr should be split up, this is the first stage of the split.

I feel that I've already spent too much time on this, implementing "the right" oauth flow will take me another 6-8 hours which I don't have atm. Either accept this pr (with guidelines on how to make it appropriate) or pull support for Point (the integration is currently broken as Minut have change the oauth flow).

Copy link
Member

@frenck frenck Oct 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Could you please name a few for reference.

Search for AbstractOAuth2FlowHandler usage in config_flow.py files across our codebase, we currently have 26 integrations implementing it.

Some ones that have been implemented fairly recently (and thus had recent reviews) are Twitch, Fitbit and YouTube.

The reasoning is that I don't feel I have the time to keep up with the high code quality standards.

This has nothing to do with coding standards. 🤷

Either accept this pr (with guidelines on how to make it appropriate) or pull support for Point (the integration is currently broken as Minut have change the oauth flow).

I really want to help, but let me be very clear: These kinds of statements don't help and are not a factor in merging PRs. If anything, they make me not wanting to help out and I don't see how this would be fruitful in any way.

Copy link
Member

Choose a reason for hiding this comment

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

Looking upstream at the minute point docs, they implement a really standard OAuth2 flow, which we fully support out of the box. The amount of code used by this integration can be reduced quite a bit and simplified.

Should not take more than 1-2 hours to fix/adjust I think (including import of existing configuration).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking upstream at the minute point docs, they implement a really standard OAuth2 flow, which we fully support out of the box. The amount of code used by this integration can be reduced quite a bit and simplified.

Should not take more than 1-2 hours to fix/adjust I think (including import of existing configuration).

Sounds great, I'll have a look at it!

}
)

def __init__(self) -> None:
"""Initialize flow."""
self.flow_impl = None
self.client_id = None
self.client_secret = None
self.redirect_uri = None

async def async_step_import(self, user_input=None):
"""Handle external yaml configuration."""
if self._async_current_entries():
return self.async_abort(reason="already_setup")

self.flow_impl = DOMAIN
flow = self.hass.data[DATA_FLOW_IMPL][DOMAIN]
self.client_id = flow[CONF_CLIENT_ID]
self.client_secret = flow[CONF_CLIENT_SECRET]

return await self.async_step_auth()

async def async_step_user(self, user_input=None):
"""Handle a flow start."""
flows = self.hass.data.get(DATA_FLOW_IMPL, {})

if self._async_current_entries():
return self.async_abort(reason="already_setup")

if not flows:
_LOGGER.debug("no flows")
if not self.flow_impl:
return self.async_abort(reason="no_flows")

if len(flows) == 1:
self.flow_impl = list(flows)[0]
return await self.async_step_auth()

if user_input is not None:
self.flow_impl = user_input["flow_impl"]
return await self.async_step_auth()

return self.async_show_form(
step_id="user",
data_schema=vol.Schema({vol.Required("flow_impl"): vol.In(list(flows))}),
)
return await self.async_step_auth()

async def async_step_auth(self, user_input=None):
"""Create an entry for auth."""
if self._async_current_entries():
return self.async_abort(reason="external_setup")

errors = {}
return self.async_abort(reason="already_setup")

if user_input is not None:
errors["base"] = "follow_link"

try:
async with asyncio.timeout(10):
url = await self._get_authorization_url()
except asyncio.TimeoutError:
return self.async_abort(reason="authorize_url_timeout")
except Exception: # pylint: disable=broad-except
_LOGGER.exception("Unexpected error generating auth url")
return self.async_abort(reason="unknown_authorize_url_generation")
self.redirect_uri = user_input.get(CONF_REDIRECT_URI)

try:
async with asyncio.timeout(10):
url = await self._get_authorization_url()
except asyncio.TimeoutError:
return self.async_abort(reason="authorize_url_timeout")
except Exception: # pylint: disable=broad-except
_LOGGER.exception("Unexpected error generating auth url")
return self.async_abort(reason="unknown_authorize_url_generation")
return self.async_external_step(
step_id="code",
url=url,
)

return self.async_show_form(
step_id="auth",
description_placeholders={"authorization_url": url},
errors=errors,
data_schema=self.schema,
)

async def _get_authorization_url(self):
"""Create Minut Point session and get authorization url."""
flow = self.hass.data[DATA_FLOW_IMPL][self.flow_impl]
client_id = flow[CONF_CLIENT_ID]
client_secret = flow[CONF_CLIENT_SECRET]
point_session = PointSession(
async_get_clientsession(self.hass),
client_id,
client_secret,
client_id=self.client_id,
client_secret=self.client_secret,
redirect_uri=self.redirect_uri,
)

self.hass.http.register_view(MinutAuthCallbackView())
return point_session.create_authorization_url(
MINUT_AUTH_URL, state=self.flow_id
)[0]

return point_session.get_authorization_url

async def async_step_code(self, code=None):
async def async_step_code(self, user_input=None):
"""Received code for authentication."""
if user_input is not None:
self.code = user_input
return self.async_external_step_done(next_step_id="finish")

async def async_step_finish(self, user_input=None):
"""Create point session and entries."""
if self._async_current_entries():
return self.async_abort(reason="already_setup")

code = self.code
if code is None:
return self.async_abort(reason="no_code")

_LOGGER.debug(
"Should close all flows below %s",
self._async_in_progress(),
)
# Remove notification if no other discovery config entries in progress

return await self._async_create_session(code)

async def _async_create_session(self, code):
"""Create point session and entries."""

flow = self.hass.data[DATA_FLOW_IMPL][DOMAIN]
client_id = flow[CONF_CLIENT_ID]
client_secret = flow[CONF_CLIENT_SECRET]
client_id = self.client_id
client_secret = self.client_secret
point_session = PointSession(
async_get_clientsession(self.hass),
client_id,
client_secret,
redirect_uri=self.redirect_uri,
)
token = await point_session.get_access_token(code)
_LOGGER.debug("Got new token")
Expand Down Expand Up @@ -181,9 +179,12 @@ async def get(request):
"""Receive authorization code."""
hass = request.app["hass"]
if "code" in request.query:
hass.async_create_task(
hass.config_entries.flow.async_init(
DOMAIN, context={"source": "code"}, data=request.query["code"]
)
result = await hass.config_entries.flow.async_configure(
flow_id=request.query["state"], user_input=request.query["code"]
)
return "OK!"
if result["type"] == data_entry_flow.FlowResultType.EXTERNAL_STEP_DONE:
return web_response.Response(
headers={"content-type": "text/html"},
text="<script>window.close()</script>Success! This window can be closed",
)
return "Error authenticating Minut Point."
2 changes: 2 additions & 0 deletions homeassistant/components/point/const.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
SCAN_INTERVAL = timedelta(minutes=1)

CONF_WEBHOOK_URL = "webhook_url"
CONF_REDIRECT_URI = "redirect_uri"

EVENT_RECEIVED = "point_webhook_received"
SIGNAL_UPDATE_ENTITY = "point_update"
SIGNAL_WEBHOOK = "point_webhook"
Expand Down
8 changes: 2 additions & 6 deletions homeassistant/components/point/strings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,10 @@
{
"config": {
"step": {
"user": {
"title": "[%key:common::config_flow::title::oauth2_pick_implementation%]",
"description": "[%key:common::config_flow::description::confirm_setup%]",
"data": { "flow_impl": "Provider" }
},
"auth": {
"title": "Authenticate Point",
"description": "Please follow the link below and **Accept** access to your Minut account, then come back and press **Submit** below.\n\n[Link]({authorization_url})"
"description": "Please fill Redirect URI below, **Submit** and **Accept** access to your Minut account in the popup dialog.",
"data": { "redirect_uri": "Redirect URI" }
}
},
"create_entry": {
Expand Down
Loading
Loading