Skip to content
This repository has been archived by the owner on Aug 29, 2024. It is now read-only.

Updated workflow inputs #29

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

whilefoo
Copy link
Contributor

@whilefoo whilefoo commented Feb 18, 2024

This modifies the workflow inputs so that they are compatible with the new kernel.

It accepts four inputs:

  • eventName
  • event
  • installationId
  • settings

@whilefoo whilefoo requested a review from 0x4007 February 18, 2024 12:25
Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

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

Code checks out. But you didn't associate an issue specification so I have little context on what's going on here.

src/index.ts Outdated Show resolved Hide resolved
@whilefoo whilefoo changed the title feat: modified for new kernel Updated workflow inputs Feb 18, 2024
@0x4007
Copy link
Member

0x4007 commented Feb 18, 2024

I am currently doing some research on making a plugin based on new architecture. Why not just pass in the auth token from the kernel? Seems redundant that each plugin needs to figure out auth every time.

0x4007/conversation-rewards@1f4ba00#diff-1243c5424efaaa19bd8e813c5e6f6da46316e63761421b3e5f5c8ced9a36e6b6R3-R6

Comment on lines 14 to 16
settings:
description: "Plugin settings"
required: true
Copy link
Member

Choose a reason for hiding this comment

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

I like that the kernel is compiling the ubiquibot-config.yml for the plugins but what about character limits for transmitting data to GitHub Action inputs?

Our configs can get quite long. I'm wondering if it makes sense for plugins to get the config if they need it. @FernandVEYRIER is working on an npm module for this specific scenario.

@whilefoo
Copy link
Contributor Author

whilefoo commented Feb 18, 2024

I am currently doing some research on making a plugin based on new architecture. Why not just pass in the auth token from the kernel? Seems redundant that each plugin needs to figure out auth every time.

pavlovcik/conversation-rewards@1f4ba00#diff-1243c5424efaaa19bd8e813c5e6f6da46316e63761421b3e5f5c8ced9a36e6b6R3-R6

I thought about that but if the plugin is public then anybody can see the token, but it's not a problem if plugins are all private but even then anyone with the access to the repo can use the token

@0x4007
Copy link
Member

0x4007 commented Feb 18, 2024

I am currently doing some research on making a plugin based on new architecture. Why not just pass in the auth token from the kernel? Seems redundant that each plugin needs to figure out auth every time.
pavlovcik/conversation-rewards@1f4ba00#diff-1243c5424efaaa19bd8e813c5e6f6da46316e63761421b3e5f5c8ced9a36e6b6R3-R6

I thought about that but if the plugin is public then anybody can see the token, but it's not a problem if plugins are all private but even then anyone with the access to the repo can use the token

I suppose that it depends on the partner if they want to use the plugin (do they trust it? Perhaps there is a way to invoke it based on a specific commit, which is great for security)

Aside from that, technically plugins can be hosted on Cloudflare Workers etc and the logs wont be public right

@whilefoo
Copy link
Contributor Author

Actually I thought the inputs are visible in the UI but I've checked again and they are not.

I'm wondering if there's a way to generate an auth token with only read access because giving an auth token means the plugin can post comments as ubiquibot and has every permission that ubiquibot has.

@0x4007
Copy link
Member

0x4007 commented Feb 18, 2024

can post comments as ubiquibot and has every permission that ubiquibot has.

Its scoped per installation ID (per organization, not repo, I believe.)

It is the partner's responsibility if they connect a bad plugin.


ChatGPT says we can't generate a token with less permissions. It suggests to make another GitHub App with less permissions and use its token instead (not feasible, I think its annoying to ask partners to add two apps.)

@whilefoo
Copy link
Contributor Author

Its scoped per installation ID (per organization, not repo, I believe.)

Installation can be either on repo or organization.

It is the partner's responsibility if they connect a bad plugin.

But I still think it's still better to give the plugin only the read permissions (principle of least privilege). Maybe a plugin's repo is hijacked or the maintainer goes rogue.

@0x4007
Copy link
Member

0x4007 commented Feb 18, 2024

Its scoped per installation ID (per organization, not repo, I believe.)

Installation can be either on repo or organization.

I know we can install the app and scope it to repo or org BUT I am pretty sure that the installation ID will be identical across every repository. Regardless if they installed it only on two repos, or they installed it org wide.

It is the partner's responsibility if they connect a bad plugin.

But I still think it's still better to give the plugin only the read permissions (principle of least privilege). Maybe a plugin's repo is hijacked or the maintainer goes rogue.

I think its really worthwhile to figure out if we can support commit hashes in our config. For example:

workflow: ubiquibot/conversation-rewards@18b21c6ede3ed4f5a27c011f3441b888b6cdfd47

It's impossible for a rouge hacker to spoof that.

But honestly even with my level of knowledge I don't think I would specify the commit hash. Seems a bit paranoid. We should just tightly scope the permissions of the bot once we are properly in production to not be able to cause mass chaos across an organization.

@0x4007
Copy link
Member

0x4007 commented Feb 22, 2024

Consider this schema ubiquity-os/ubiquity-os-kernel#25 (comment)

@0x4007 0x4007 marked this pull request as draft February 22, 2024 22:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants