Skip to content

Generate random webhook_id and add copy button#11568

Merged
bramkragten merged 23 commits intohome-assistant:devfrom
esev:random_webhook
Feb 10, 2022
Merged

Generate random webhook_id and add copy button#11568
bramkragten merged 23 commits intohome-assistant:devfrom
esev:random_webhook

Conversation

@esev
Copy link
Copy Markdown
Contributor

@esev esev commented Feb 5, 2022 •
edited
Loading

Proposed change

Webhooks IDs are a little like passwords in that it is safest if they are not guessable by others. This PR attempts to keep users safe by default by auto-generating the webhook_id when a new webhook trigger is created. I've also added a copy-to-clipboard button for the webhook URL to encourage use of the default value by making it the simplest action.

Users still have the ability to override the default, randomly generated, webhook_id if they desire to do so. This PR adds the text "Treat this ID like a password: keep it secret, and make it hard to guess", so users are aware of the safest way to generate their own webhook_ids.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

image

Additional information

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

@esev esev mentioned this pull request Feb 5, 2022
Merged
22 tasks
@alderete
Copy link
Copy Markdown

alderete commented Feb 6, 2022 •
edited
Loading

@esev Nice! And nicely isolated from other changes proposed in #56446.

I've reviewed the existing doc, and while the changes in this PR don't require doc changes, I'm proposing some revisions to better set up for other possible (hopeful!) changes for webhooks. I'll add details that align with this PR, without requiring it to be submitted first.

Doc PR here: home-assistant/home-assistant.io#21506

bramkragten
bramkragten reviewed Feb 7, 2022
View reviewed changes
src/panels/config/automation/trigger/types/ha-automation-trigger-webhook.ts Outdated
import { mdiContentCopy } from "@mdi/js";
import { html, LitElement } from "lit";
import { customElement, property } from "lit/decorators";
import { PaperInputElement } from "@polymer/paper-input/paper-input";
Copy link
Copy Markdown
Member

@bramkragten bramkragten Feb 7, 2022

Choose a reason for hiding this comment

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

Suggested change
import { PaperInputElement } from "@polymer/paper-input/paper-input";
import type { PaperInputElement } from "@polymer/paper-input/paper-input";

Copy link
Copy Markdown
Contributor Author

@esev esev Feb 7, 2022

Choose a reason for hiding this comment

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

Done in 6721694.

zsarnett
zsarnett previously approved these changes Feb 7, 2022
View reviewed changes
@esev @zsarnett
paper-input.id no longer needed after eecdf03
d6170d4 
Co-authored-by: Zack Barett <zackbarett@hey.com>
zsarnett
zsarnett previously approved these changes Feb 7, 2022
View reviewed changes
balloob
balloob reviewed Feb 7, 2022
View reviewed changes
balloob
balloob reviewed Feb 7, 2022
View reviewed changes
balloob
balloob reviewed Feb 8, 2022
View reviewed changes
src/panels/config/automation/trigger/types/ha-automation-trigger-webhook.ts Outdated
if (webhookId === DEFAULT_WEBHOOK_ID) {
webhookId = this._generateWebhookId();
const newTrigger = { ...this.trigger, webhook_id: webhookId };
fireEvent(this, "value-changed", { value: newTrigger });
Copy link
Copy Markdown
Member

@balloob balloob Feb 8, 2022

Choose a reason for hiding this comment

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

the render method cannot have side effects. Do this in willUpdate instead.

Copy link
Copy Markdown
Contributor Author

@esev esev Feb 8, 2022

Choose a reason for hiding this comment

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

Thanks for this. That helped clean this up in 43d6c49.

balloob
balloob reviewed Feb 8, 2022
View reviewed changes
src/panels/config/automation/trigger/types/ha-automation-trigger-webhook.ts Outdated
import { HomeAssistant } from "../../../../../types";
import { handleChangeEvent } from "../ha-automation-trigger-row";

const DEFAULT_WEBHOOK_ID = "default webhook id";
Copy link
Copy Markdown
Member

@balloob balloob Feb 8, 2022

Choose a reason for hiding this comment

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

Why not make this an empty string ?

Copy link
Copy Markdown
Contributor Author

@esev esev Feb 8, 2022

Choose a reason for hiding this comment

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

It is now. The (incorrect) way I was doing it before in render caused clearing the textfield contents to regenerate the ID. Probably not what the user wanted. Your suggestion to use willUpdate made this a lot simpler. Thanks!

balloob
balloob reviewed Feb 8, 2022
View reviewed changes
src/panels/config/automation/trigger/types/ha-automation-trigger-webhook.ts Outdated
.path=${mdiContentCopy}
></ha-icon-button>
</paper-input>
<div class="helper-text">
Copy link
Copy Markdown
Member

@balloob balloob Feb 8, 2022

Choose a reason for hiding this comment

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

Use ha-textfield isntead of paper-input and you will be able to use the built-in helper text property. https://github.com/material-components/material-web/tree/master/packages/textfield#helper-text

Copy link
Copy Markdown
Contributor Author

@esev esev Feb 8, 2022

Choose a reason for hiding this comment

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

Thank you! That helped a lot.

image

esev and others added 2 commits February 8, 2022 08:26
@balloob balloob added the needs design preview PRs with this label will trigger a GitHub action to generate a gallery preview label Feb 8, 2022
@balloob
Copy link
Copy Markdown
Member

balloob commented Feb 8, 2022

Enabled design preview so it's easy to see how it will look: https://6202ebd9de68c100648f18de--home-assistant-gallery.netlify.app/#automation/editor-trigger (to try in local development: gallery/script/develop_gallery)

I think that we need to add a margin-bottom: 16px on the "trigger type" selector. Right now the input is very glued to it.

@bramkragten
Copy link
Copy Markdown
Member

bramkragten commented Feb 10, 2022

Will handle the margin globally

@bramkragten bramkragten merged commit b0b3222 into home-assistant:dev Feb 10, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Feb 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@balloob balloob balloob left review comments

@bramkragten bramkragten bramkragten approved these changes

@zsarnett zsarnett zsarnett left review comments

+1 more reviewer

@alderete alderete alderete left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed needs design preview PRs with this label will trigger a GitHub action to generate a gallery preview

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@esev @alderete @balloob @bramkragten @zsarnett @homeassistant