ubiquity-os / ubiquity-os-plugin-installer

A GUI to install UbiquityOS plugins.
0 stars 8 forks source link

Feat/config installer #13

Closed Keyrxng closed 1 day ago

Keyrxng commented 1 week ago

Resolves https://github.com/ubiquity-os/ubiquity-os-plugin-installer/issues/3 Requires #11 and #12 (reviewing those could be skipped and focus placed just on this PR)

Still buggy with some plugins I need to inspect the more complex ones and handle their configs better but this shows what it looks like with a plugin with a medium size config. It should be ready to over the next day.

@zugdev Idk if you want to work your magic after this is merged, branch off and PR against dev or into this before that or just review and create a spec for what's needed? What do you think?


We can still build this UI with the ability to read from a query param directly too but it's a cleaner approach housing them all imo.

QA:

https://github.com/user-attachments/assets/6095919d-a491-45b1-a5c7-05d6215ac769

Keyrxng commented 1 week ago
  1. manifest.name should reflect the repository name so we can match it to the worker/action url
  2. all plugin manifests rendering without error.
  3. redesigning the UI to accommodate the extensive configs should be spec'd
  4. I've achieved the spec as it exists for this task, following some minor tweaks to config prop options this is review ready.

QA with official plugin URL:

https://github.com/user-attachments/assets/0b0bf1fc-5564-4417-84c1-de395149c698


@zugdev I saw you say that this codebase is your scope, perhaps you want to start reviewing my PRs as I think 0x4007 is busy.

zugdev commented 1 week ago

I will review UI, yes. Regarding my contribution, I think it makes more sense for this to be merged first and only then implement styling.

Keyrxng commented 1 week ago

@gentlementlegen, is null valid for text-conversation-reward inputs for the modules? It appears so via the schema, is this valid to ship for V1 and we can decide on how we'll display all of the HTML tags?

image

image

gentlementlegen commented 1 week ago

@Keyrxng Yes it is, it means the module is disabled.

Keyrxng commented 1 week ago

@Keyrxng Yes it is, it means the module is disabled.

Understood.

That's the kinda thing I'm trying to think how can we tell the partner that for any prop on any plugin?

My first thought is to capture /**\ comments above the pluginSettingschema values or something and inject them as a description into configuration when it's auto-generated via wf.

Or present them the readme when they select a plugin which should say null === disabled or whatever somewhere?

Any better ideas?

Keyrxng commented 1 week ago

Alright well I'm going to grab QA of:

Is there anything else I need to do to get this merged, spec was minimal but it has been achieved:

  1. Org selection > config selection > plugin selection > edit > push
  2. Our config is parsed, their config is parsed and their config is updated.
  3. No LLM usage required.
zugdev commented 1 week ago

Actions doesn't seem to be building this PR and therefore no deployments are available. 0x4007 just merged template into this repo, so pulling from main should solve it, I want to help you QA it.

Keyrxng commented 1 week ago

Actions doesn't seem to be building this PR and therefore no deployments are available. 0x4007 just merged template into this repo, so pulling from main should solve it, I want to help you QA it.

Actions are failing due to env vars which I cannot edit I'm afraid. Jest is failing due to the old octokit/core imports as usual, since tests are not written for this codebase yet we can either ignore CI or I can remove the template test suites until unit tests are built.

Also afaik preview deployments are for reviewers that aren't able to QA locally like 0x4007 from mobile etc. This does run locally if you'd like to QA it.

zugdev commented 1 week ago

Yes, deployments are a mere facilitator, I understand the circumstances. I'll be testing it locally soon!

Keyrxng commented 1 week ago

Yes, deployments are a mere facilitator, I understand the circumstances. I'll be testing it locally soon!

LFFGGG

Some points for setup:

  1. Your DB auth should be tied to a GitHub App not an OAuth app. The kernel is a GitHub app, the same is required for this because we need permissions that are out of scope for a normal OAuth app.
  2. For QA it's probably best to just use your kernel app and update the callback url like I did. For prod I'm unsure right now if we'll need a new app for this or if it's safe to use the kernel.

Known problems:

Keyrxng commented 1 week ago
  1. Modify an installed plugin
  2. Remove an installed plugin
  3. Add multiple plugins (config stored contained both the newly added and the previously removed as it was not cleared from state for sake of QA)

I decided to display objects via JSON.stringify and modify the object vals directly vs my initial approach in creating inputs for each.

https://github.com/user-attachments/assets/e4331aaa-9b0f-4237-98e5-862cfef33e5a

Keyrxng commented 1 week ago

I'm going to move on and complete other tasks while reviews are pending

zugdev commented 1 week ago

If I sign in with an account that has nothing to show, we should tell that to the user.

image

The auth token is not being cleared on sign out, the only way for me to currently logout is to clear my cookies manually.

https://github.com/user-attachments/assets/15ab6e6a-ad30-4783-b4fd-6b0cf2069716

Also we have a scope error that is not allowing it to fetch my orgs correctly, I've pinpointed the issue in my review comments:

image

zugdev commented 1 week ago

I think you should try catch whatever can fail, that certainly includes HTTP requests. For instance, to get the list of app installations you need an authenticated GitHub app. I couldn't login with my org so I got deadlocked. We should at least show a message in those cases.

zugdev commented 1 week ago

Some points for setup:

  1. Your DB auth should be tied to a GitHub App not an OAuth app. The kernel is a GitHub app, the same is required for this because we need permissions that are out of scope for a normal OAuth app.

Ignore what I previously said about auth, already hid my comments, my Supabase is set for OAuth - my bad. However I still believe there should be try catches.

How should I set this up? I don't see a way to do it in Supabase's dashboard. Or did you mean signing in with a GitHub app? If that's the case, I wasn't able to.

ubiquity-os-beta[bot] commented 1 week ago

@Keyrxng, this task has been idle for a while. Please provide an update.

Keyrxng commented 1 week ago

So the only changes requested I can implement is to ensure the auth token is popped from local?

I'll also wrap main in a try-catch and capture any errors

rndquu commented 1 week ago

@Keyrxng https://github.com/ubiquity-os/ubiquity-os-plugin-installer/pull/12 is included in the current PR, correct?

Keyrxng commented 1 week ago

https://github.com/ubiquity-os/ubiquity-os-plugin-installer/pull/12 is included in the current PR, correct?

Yeah that's right, using the methods but the approach has changed.

Perhaps we should pass the read:org scope similarly to how it works in work.ubq.fi?

I setup my auth using a GitHub App not an OAuth App (more here) for a couple of reasons. Finer control over scopes and gets us access to potentially private orgs too. I used the same app as my UbiquityOS dev instance as it made sense to do the same for prod.

https://github.com/ubiquity-os/ubiquity-os-plugin-installer/blob/27b6a42b612c500cf03575670259833e3e56ec20/static/main.ts#L38-L45

  1. Collect orgs via authenticated user (our GitHub app permissions setup controls visibility)
  2. Use app to obtain all installs
  3. Map over the user's orgs and find only the orgs which have UbiquityOS installed

In general, GitHub Apps are preferred over OAuth apps. GitHub Apps use fine-grained permissions, give the user more control over which repositories the app can access, and use short-lived tokens. These properties can harden the security of your app by limiting the damage that could be done if your app's credentials were leaked.

image

Supabase auth needs updated with your GitHub app Client ID and secret.

What do you mean with this, exactly? Is it in a secrets format, or an oauth config perhaps? Where do I set this up?

You'll find that in the Authentication > Providers section on Supabase.

image

Pls make all CI workflows passing:

I'll make changes so that they pass for now no problem

zugdev commented 1 week ago

Thanks for the guide, I'll QA it soon.

Keyrxng commented 1 week ago

Thanks for the guide, I'll QA it soon.

Np. It would be better if I could QA with the production app details but I'm basing my logic on these responses.

I have only one org but my dev app is installed on my personal account too (production will only ever be installed to orgs) so it follows that the owner of the org (or admins etc if we support that) would be the one to install plugins (at least the initial setup).

image

I assume we'd support other roles besides owner but that will require additional logic for validating the user has permissions to update the config etc. Which roles would be allowed to install/update/remove, is this configurable at the partner level or UI level for example (org config sounds good for this)

Keyrxng commented 1 week ago

Empty strings CI is not failing because of empty strings in the code it presents warnings in the review diff as sometimes their usage is needed, instead it's failing because of:

Error: An error occurred: Resource not accessible by integration - https://docs.github.com/rest/checks/runs#create-a-check-run

I've implemented the requested changes and previous auth issues were not because of the code itself so I think this is ready for review.

zugdev commented 6 days ago

One feedback I'd give is since you initiated UI, you should add basic setup instructions in the README file. Not only yarn related, but also that we use GitHub App auth instead of OAuth App setup.

zugdev commented 6 days ago

I have only one org but my dev app is installed on my personal account too (production will only ever be installed to orgs) so it follows that the owner of the org (or admins etc if we support that) would be the one to install plugins (at least the initial setup).

Quick question. The app in question was created by your personal account or your org? I have setup the auth with GitHub app but that app belongs to my org, I think therefore when I login as my user, I can't fetch properly. It's not a matter of permissions since I've enabled all.

Keyrxng commented 6 days ago

I have only one org but my dev app is installed on my personal account too (production will only ever be installed to orgs) so it follows that the owner of the org (or admins etc if we support that) would be the one to install plugins (at least the initial setup).

Quick question. The app in question was created by your personal account or your org? I have setup the auth with GitHub app but that app belongs to my org, I think therefore when I login as my user, I can't fetch properly. It's not a matter of permissions since I've enabled all.

It was created under my account and then installed to my org yeah. I didn't think there would be any difference in terms of what's visible if the app controls permissions.

Keyrxng commented 6 days ago

I just created a new app belonging to my org and everything worked fine.

  1. Ensure you have Email addresses selected for Account Permissions image
  2. Ensure that you actually install the GitHub app into the org.
  3. I didn't have to but I read in the GH docs that you should remove callbacks to other apps (last resort) after migrating from OAuth > GitHub App

I'm sure with these things should work for you, please let me know if I can do anything further to help

rndquu commented 4 days ago

@Keyrxng

You use your UbiquityOS instance because we check for installs against the authenticated user's organizations that they own.

If partner doesn't have the UbiquityOS app installed anywhere (which is true for new partners) then he will not be able to install the config, correct?

Overall I still don't understand why we should mess with github app auth instead of oauth which is already working fine in work.ubq.fi.

Finer control over scopes and gets us access to potentially private orgs too.

work.ubq.fi also has access to private repos with oauth

Keyrxng commented 4 days ago

If partner doesn't have the UbiquityOS app installed anywhere (which is true for new partners) then he will not be able to install the config, correct?

That was the intention yeah

Overall I still don't understand why we should mess with github app auth instead of oauth which is already working fine in work.ubq.fi. work.ubq.fi also has access to private repos with oauth

You can't check installs using an OAuth app as they are not installable and it felt right to track using the official instance install. If we wanted OAuth for access and then confirm the installs after we'd need to auth as the app via clientId/secret or appId/privateKey.


I've refactored OAuth to use an OAuth app, I have inlined the permissions required and tested things and they work.

  1. No install check, we are just listing the user's orgs.
  2. Only ubq-testing is showing for me so I continue to assume only orgs that they own will appear.
  3. I QA'd only with my OAuth app and things went smooth, I expect no difference from the OAuth app we use for work.ubq
rndquu commented 3 days ago

@Keyrxng Trying to add the https://github.com/ubiquity-os-marketplace/command-wallet plugin to this organization. Getting the Error pushing config to GitHub: TypeError: source is not a string error:

Screenshot 2024-11-19 at 11 05 36

Does this PR create a new .ubiquity-os repository for new partners?

Keyrxng commented 3 days ago

Does this PR create a new .ubiquity-os repository for new partners?

Sorry no it didn't, I assumed that would be part of the kernel setup steps but I have added logic to create the config repo now.

https://github.com/user-attachments/assets/28fd190a-8688-42a2-9acd-fb26e09b4e54

rndquu commented 3 days ago

I have added logic to create the config repo now.

Awesome, works fine.

@Keyrxng Could you update the readme file and remove github app related setup instructions?

The Empty String Check workflow is failing but that's false positive, added https://github.com/ubiquity/ts-template/issues/82.

gentlementlegen commented 3 days ago

Tested, works, that's cool.

Some details that would be worth a change later:

Maybe it would be safer to wrap string values with quotes

image
Keyrxng commented 1 day ago

Maybe it would be safer to wrap string values with quotes

I'm not so sure. I'm using YAML and it states "Passes all of the [yaml-test-suite](https://github.com/yaml/yaml-test-suite) tests" which is the official YMAL test suite as far as I can tell.

I'd have to modify the package (I think) in order to wrap in quotes but if you feel we should, can we task it separately?

On a 32 inch screen, it feels like a lot of space is wasted (c.f. screenshot)

Agreed I intentionally left CSS alone as much as possible and hoped to have it spec-ed but will be addressed in https://github.com/ubiquity-os/ubiquity-os-plugin-installer/issues/19.

Keyrxng commented 1 day ago

@Keyrxng Could you update the readme file and remove github app related setup instructions?

I did this here aba5a19 and it includes OAuth app terminology now but the process is still the same for setup.

rndquu commented 1 day ago

@Keyrxng Could you update the readme file and remove github app related setup instructions?

I did this here aba5a19 and it includes OAuth app terminology now but the process is still the same for setup.

Could you fix the failing knip ci?

Keyrxng commented 1 day ago

My bad I didn't notice that but also weird that it failed due to the plugin-sdk

gentlementlegen commented 1 day ago

Maybe it would be safer to wrap string values with quotes

I'm not so sure. I'm using YAML and it states "Passes all of the [yaml-test-suite](https://github.com/yaml/yaml-test-suite) tests" which is the official YMAL test suite as far as I can tell.

I'd have to modify the package (I think) in order to wrap in quotes but if you feel we should, can we task it separately?

On a 32 inch screen, it feels like a lot of space is wasted (c.f. screenshot)

Agreed I intentionally left CSS alone as much as possible and hoped to have it spec-ed but will be addressed in https://github.com/ubiquity-os/ubiquity-os-plugin-installer/issues/19.

If that's not causing any issue then don't sweat on it, I just wanted to make sure. Got it, good to go.

Keyrxng commented 1 day ago

Got it, good to go.

I cannot merge, so whenever you are ready is good.