feat: better onboarding process

[gentlementlegen]

Resolves #3

[ubiquity-os-deployer[bot]]

5451a5b

[gentlementlegen]

What would be needed for the deployment to work

FRONTEND_URL should be added to the env, it corresponds to the redirect url on a successful login attempt. The Cypress tests fail because they use the files in the org, not the ones in my pull request that are up to date.

What are the changes

The old system of entering a PAT has been changed to use OAuth login instead. for user convenience. It allows to directly login with a click, and also to list the available repositories instead of having to manually add the repository name.

What are the drawbacks

Using the OAuth token restricts the actions that can be performed by Octokit. One of them is to set the repositories to the Ubiquibot App, as per described here https://docs.github.com/en/rest/apps/installations?apiVersion=2022-11-28#add-a-repository-to-an-app-installation

This endpoint does not work with GitHub App user access tokens, GitHub App installation access tokens, or fine-grained personal access tokens.

I don't think it is crucial, since by default apps have access to all the repositories. Yet, if we would like to inform the user about this, I thought we could display a link with a reminder like "Don't forget to check the permissions for your app ". Please let me know if anything comes to mind and I will add it. Edit: after talking with @pavlovcik it seems that this step is not required since the app already has the authorization needed, so we should be good to go.

Screenshot of the new workflow

[0x4007]

All looking good thanks. When you implement requested changes you should resolve each of the conversations. You can use the conversations as your checklist.

[rndquu]

This PR shouldn't have been merged with the failing build

@FernandVEYRIER Where should we set the FRONTEND_URL env variable, here? And what value should we use?

[0x4007]

This PR shouldn't have been merged with the failing build

@FernandVEYRIER Where should we set the FRONTEND_URL env variable, here? And what value should we use?

We had a discussion on this but I suppose it might have been more appropriate to do so here in the comments. Basically I got a bit confused because it should be tested on a deployed cloudflare URL from the current commit. But this means that we need to take the Cloudflare URL as the output of the continuous deploys and then pass it into Cypress to test.

Alternatively, if I'm understanding correctly, we can just build and run in GitHub Actions and use localhost:8080

@FernandVEYRIER can clarify

[gentlementlegen]

@rndquu The build was failing because to preserve the secrets we have to trigger pull requests on the context of the target repo and not to one opening the PR, so the workflow was actually not up to date. And yes for the local build / Cypress testing localhost:8080 can be used. For the production, the deployment url (the url where the frontend is deployed) should be used.

[molecula451]

makes more sense now if we're going to refactor these sites to a specific library/framework static site that we just start on that issue and decrease the feat addition, or maybe work both in parallel

[gentlementlegen]

So currently we should just default FRONTEND_URL to the currently deployed url for that page.

[rndquu]

I'm 99% sure this (one, two) is vulnerable to organization secrets exfiltrating

[gentlementlegen]

Is this still an open issue, there was no patch whatsoever?

[rndquu]

@FernandVEYRIER

As far as I understand cypress and knip workflows don't work as expected right now. When pull_request_target is used it means that the workflow runs in the context of the base branch (ubiquity/development) hence it has access to organization secrets but here it checks out the code in the ubiquity/development branch, not the one in the PR. So right now both cypress and knip workflows are basically running only against the ubiquity/development branch on all PRs.

If we want to run cypress and knip workflows against the PR branch then we need to modify the checkout action this way:

    steps:
    - uses: actions/checkout@v2
      with:
        ref: ${{ github.event.pull_request.head.sha }}

But the above code allows malicious actors to exfiltrate github access token, organization secrets and all this sort of stuff here or here (more info).

TBH I haven't yet seen a case when pull_request_target should really be used.

[rndquu]

So currently we should just default FRONTEND_URL to the currently deployed url for that page.

So we need to:

1. Fetch deployment URL for a PR here
2. Pass the deployment URL here

Right?

[gentlementlegen]

@FernandVEYRIER

As far as I understand cypress and knip workflows don't work as expected right now. When pull_request_target is used it means that the workflow runs in the context of the base branch (ubiquity/development) hence it has access to organization secrets but here it checks out the code in the ubiquity/development branch, not the one in the PR. So right now both cypress and knip workflows are basically running only against the ubiquity/development branch on all PRs.

If we want to run cypress and knip workflows against the PR branch then we need to modify the checkout action this way:

    steps:
    - uses: actions/checkout@v2
      with:
        ref: ${{ github.event.pull_request.head.sha }}

But the above code allows malicious actors to exfiltrate github access token, organization secrets and all this sort of stuff here or here (more info).

TBH I haven't yet seen a case when pull_request_target should really be used.

Makes sense. The only reason to use the pull_request_access was to pass the secrets indeed. Since we review the pull-requests before merging them, wouldn't it be okay to still use it? Not everyone can merge them in the repo so it's not like random malicious code can get in. It is still a security issue ofc, but if we can't use it that way then I guess we can have these workflows working properly. Otherwise, let's change when they are run.

[gentlementlegen]

So currently we should just default FRONTEND_URL to the currently deployed url for that page.

So we need to:

1. Fetch deployment URL for a PR here
2. Pass the deployment URL here

Right?

We can make it simple right now and just put the frontend URL of the production deployment. But yes that would be the correct behavior to be able to test for each deployment. Currently Supabase is set to allow redirects to https://**.ubq.fi

[rndquu]

So currently we should just default FRONTEND_URL to the currently deployed url for that page.

So we need to:

1. Fetch deployment URL for a PR here
2. Pass the deployment URL here

Right?

We can make it simple right now and just put the frontend URL of the production deployment. But yes that would be the correct behavior to be able to test for each deployment. Currently Supabase is set to allow redirects to https://**.ubq.fi

https://github.com/ubiquity/cloudflare-deploy-action/issues/4

[rndquu]

Since we review the pull-requests before merging them, wouldn't it be okay to still use it?

It doesn't matter if we review it or not. Once this change is made any PR will be able to run code with access to organization env variables:

    on: pull_request_target

    steps:
    - uses: actions/checkout@v2
      with:
        ref: ${{ github.event.pull_request.head.sha }}