Feature/google calendar read only support #52790

BottlecapDave · 2021-07-09T10:48:43Z

Breaking change

N/A

Proposed change

This update provides the ability to configure google calendar support with either readwrite access (current behaviour) or read access. This was the result of #45229, and I needed as I wanted to add my work account but felt uncomfortable giving write access (which could be quite destructive if compromised).

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 "fixes" #45229 while still providing the existing functionality by default.

You can chane

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:

Documentation added/updated for www.home-assistant.io

Happy to do this once the PR has been approved.

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:

I have reviewed two other open pull requests in this repository.

By default the current read/write access will be given, but the user has the option to set read only access which stops the add event service from registering

# Conflicts: # homeassistant/components/google/__init__.py

allenporter

This looks like a nice change. Can you also include a pointer to a documentation update that goes along with this? (with a back reference to this PR)

I saw the issue referenced also discusses how the docs are out of date, and so fixing that would be nice. Also this includes a new configuration option that needs documentation too.

For context, I think the best practice is: Send the documentation for the next branch and code PRs at the same time referencing each other, we'll make sure it all makes sense together, then we'll get in the code, then get in the docs.

allenporter · 2021-07-24T19:29:39Z

homeassistant/components/google/__init__.py

+def get_scope(conf):
+    """Gets the google calendar scope based on the defined access level."""
+
+    scopes = ""


I was looking at some other examples of using enums for config in home assistant and came across what zha does with the RadioType enum:

core/homeassistant/components/zha/core/const.py

Line 233 in 470f2dd

class RadioType(enum.Enum):

Some of this logic can go away if the access is defined as an enum with the scope baked in, something like:

class FeatureAccess(Enum): read_only = ("https://www.googleapis.com/auth/calendar.readonly") readwrite = ("https://www.googleapis.com/auth/calendar") def __init__(self, scope: str) -> None: """...""" self._scope = scope @property def scope(self) -> str: """...""" return self._scope

Then I think you can just get the enum value from the config and pull the scope out of it all inline.

Didn't realise this was a thing. Updated.

allenporter · 2021-07-24T19:36:25Z

homeassistant/components/google/__init__.py

    """Check for the correct scopes in file."""
    with open(token_file) as tokenfile:
-        if "readonly" in tokenfile.read():
+        contents = tokenfile.read()
+        if (config.get(CONF_CALENDAR_ACCESS) is FeatureAccess.ReadWrite and "readonly" in contents) or (config.get(CONF_CALENDAR_ACCESS) is FeatureAccess.Read and "readonly" not in contents):


I would suggest making the above change then doing a check for the entire scope url to simplify this logic, and perhaps make it less error prone. That is, I think what this is trying to do is assert that the tokenfile contains the desired scope, so explicitly checking if the desired scope is in the file may be more straight forward.

(I realize this PR extends the existing pattern, so i'm more suggesting the existing pattern is a little brittle)

Updated to hopefully be more robust

This was done as a MR suggestion to simplify the code.

BottlecapDave · 2021-07-25T04:36:08Z

I've added the proposed doc changes at home-assistant/home-assistant.io#18605. I've also made the changes as suggested.

BottlecapDave · 2021-07-25T04:44:00Z

One additional thing to note - If a user wanted to downgrade their access level, they will probably hit #51490 as I too hit this issue when adding this feature. I'm trying to work out why this is happening in the specified area of code, but struggling tbh.

allenporter · 2021-07-25T06:33:44Z

homeassistant/components/google/__init__.py

    """Check for the correct scopes in file."""
    with open(token_file) as tokenfile:
-        if "readonly" in tokenfile.read():
+        contents = tokenfile.read()
+        target_scope = f"\"scope\": \"{config.get(CONF_CALENDAR_ACCESS).scope}\""


I know I asked for this to be more robust -- but maybe just check the scope to avoid over asserting on the json format. I am only saying this because I saw #53364 a few days ago where folks were compressing json and then it had a subtle side effect of breaking a regexp.

Reduced down to quoted scope. This is needed because read_write scope is a subset of read_only scope.

I completely missed that, oof, so I get it why the original check was just for the suffix then. Are you still happy with this approach?

The new approach is fine. I think you were right to bring up the potential risk of whitespace messing with the check, and this new check is a bit more robust than the old way.

frenck · 2021-07-26T06:51:46Z

homeassistant/components/google/__init__.py

 CONFIG_SCHEMA = vol.Schema(
    {
        DOMAIN: vol.Schema(
            {
                vol.Required(CONF_CLIENT_ID): cv.string,
                vol.Required(CONF_CLIENT_SECRET): cv.string,
                vol.Optional(CONF_TRACK_NEW): cv.boolean,
+                vol.Optional(CONF_CALENDAR_ACCESS, default="read_write"): cv.enum(


This PR violates ADR-0010 @allenporter

How should we proceed? I would vote revert this PR.

I was not aware of this. I assume this was done to force old integrations to the new UI flow? If this was to be converted, what would the ACs be?

I assume this was done to force old integrations to the new UI flow

That is not the sole reason.

If this was to be converted, what would the ACs be?

The "ACs"?

Sorry, the acceptance criteria of the conversion? For example, "The client id/secret and access level should be set via the UI flow"

It would need an OAuth flow (config flow). Client ID & secrets are part of the YAML configuration, not the UI.

Just looked at the integration, seeing the settings for the specific calendars, I can see this will become problematic. Let's keep this PR in for now.

Thanks for looking closer. Are we good on documentation? Forgot to update the PR links so sorry about that.

My assumption is this is like #44529 which I aborted, and I haven't put time into exploring a separate oauth UI. An alternative is we do a hybrid of yaml plus config flow for everything but oauth.

Also interestingly this is configuring the oauth scope so it's even more borderline.

OAuth scopes (variable / conditional) can be handled in an OAuth flow, that is not a problem.

Thanks for the reviews guys and sorry that what I thought was a simple PR ended up being a controversial update. I'll try and remember the ADR in the future.

What you guys are doing with Home Assistant is appreciated :)

BottlecapDave added 2 commits July 9, 2021 11:09

feat(google): Added support for read only access in google calendar.

4c385ff

By default the current read/write access will be given, but the user has the option to set read only access which stops the add event service from registering

fix(google): Updated documentation link

95054f3

This comment has been minimized.

Sign in to view

homeassistant added the cla-error label Jul 9, 2021

probot-home-assistant bot added integration: google new-feature labels Jul 9, 2021

Merge branch 'dev' into feature/google-calendar-read-only-support

103ee1a

# Conflicts: # homeassistant/components/google/__init__.py

This comment has been minimized.

Sign in to view

homeassistant added cla-needed cla-signed and removed cla-needed labels Jul 16, 2021

docs(google-calendar): Added style fixes

895c027

frenck added cla-recheck and removed cla-error cla-signed labels Jul 23, 2021

homeassistant added cla-signed and removed cla-recheck labels Jul 23, 2021

allenporter self-assigned this Jul 24, 2021

allenporter self-requested a review July 24, 2021 19:21

allenporter reviewed Jul 24, 2021

View reviewed changes

feat(calendar-google): Updated scopes to be defined on enum property.

c4cb6e8

This was done as a MR suggestion to simplify the code.

BottlecapDave mentioned this pull request Jul 25, 2021

Feat/calendar google docs home-assistant/home-assistant.io#18605

Merged

8 tasks

feat(calendar-google):Removed constants no longer needed.

d824582

allenporter reviewed Jul 25, 2021

View reviewed changes

BottlecapDave added 2 commits July 25, 2021 09:56

feat(calendar-google): Reduced scope check to minimum.

4816d6a

style: Fixed style issues

9faa56e

allenporter merged commit f3ba717 into home-assistant:dev Jul 25, 2021

frenck reviewed Jul 26, 2021

View reviewed changes

github-actions bot locked and limited conversation to collaborators Jul 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/google calendar read only support #52790

Feature/google calendar read only support #52790

BottlecapDave commented Jul 9, 2021 •

edited by allenporter

Loading

This comment has been minimized.

This comment has been minimized.

allenporter left a comment

allenporter Jul 24, 2021

BottlecapDave Jul 25, 2021

allenporter Jul 24, 2021

BottlecapDave Jul 25, 2021

BottlecapDave commented Jul 25, 2021

BottlecapDave commented Jul 25, 2021

allenporter Jul 25, 2021

BottlecapDave Jul 25, 2021

allenporter Jul 25, 2021

BottlecapDave Jul 25, 2021 •

edited

Loading

frenck Jul 26, 2021 •

edited

Loading

BottlecapDave Jul 26, 2021

frenck Jul 26, 2021

BottlecapDave Jul 26, 2021

frenck Jul 26, 2021

allenporter Jul 26, 2021

allenporter Jul 26, 2021 •

edited

Loading

frenck Jul 26, 2021

BottlecapDave Jul 26, 2021

Feature/google calendar read only support #52790

Feature/google calendar read only support #52790

Conversation

BottlecapDave commented Jul 9, 2021 • edited by allenporter Loading

Breaking change

Proposed change

Type of change

Additional information

Checklist

This comment has been minimized.

This comment has been minimized.

allenporter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BottlecapDave commented Jul 25, 2021

BottlecapDave commented Jul 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BottlecapDave Jul 25, 2021 • edited Loading

Choose a reason for hiding this comment

frenck Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allenporter Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BottlecapDave commented Jul 9, 2021 •

edited by allenporter

Loading

BottlecapDave Jul 25, 2021 •

edited

Loading

frenck Jul 26, 2021 •

edited

Loading

allenporter Jul 26, 2021 •

edited

Loading