Add actions admin user impersonation via a --actions-admin-user flag #46

derwasp · 2022-09-05T14:59:13Z

Disclaimer
This the first PR in GO for me. I don't know how things are usually done.

Background

The actions-sync app seems to also be used for the GHAE instances sometimes. The issue there was that the default admin user doesn't have permission to the actions and the github orgs. The previous workaround was to temporarily add the admin user to the required orgs and then run the app, which is not really convenient for customers who don't have access to the instance console and therefore can't run ghe-org-admin-promote.

We already have an app which impersonates the actions-admin user, and this is how it was implemented: github/codeql-action-sync-tool#48
This PR does something similar.

Proposed changes

Add a new flag: --actions-admin-user which would accept the actions user. By default the flag is not set. To use the default value, a user has to pass actions-admin to it. The logic is based on this. This flag is used to impersonate the actions admin user before attempting to push the repository. Note that this logic requires the site_admin scope to be available in the token.
Fixed the tests to run with the new logic which impersonates the user before the push call: I just stubbed the endpoints.
Added the new flags to the readme.md: 👉 rendered

Unrelated changes

Added the artifact binary to the .gitignore file

Notes

To manually test this, do the following:

Create a new GHAE instance (you can use a chat op).
Create a new personal access token for the ghae-admin user.
Spin up a new codespaces instance in this repository and checkout this PR.
In your codespaces build the project: go build.
Run the actions-sync command in your codespaces like so, where destination-token is the token for the ghae-admin user:

      mkdir /tmp/cache
      actions-sync sync \
        --cache-dir "/tmp/cache" \
        --destination-token "ghp_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
        --destination-url "https://ghaebuildXXXXXXXXXXXX.XXXXXX.net/" \
        --repo-name actions/setup-node \
        --actions-admin-user actions-admin

The output should look something like this:

    @derwasp ➜ /workspaces/actions-sync $ go build
    @derwasp ➜ /workspaces/actions-sync $ actions-sync sync \
    >     --cache-dir "/tmp/cache" \
    >     --destination-token "ghp_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
    >     --destination-url "https://ghaebuildXXXXXXXXXXXX.XXXXXXXX.net/" \
    >     --repo-name actions/setup-node
    fetching * refs for actions/setup-node ...
    Getting an impersonation token for `actions-admin`..
    Running against GitHub AE, changing the repository scope to 'repo'..
    Got the impersonation token for `actions-admin`..
    syncing `actions/setup-node`
    successfully synced `actions/setup-node`

Previously the output would look something like this:

    fetching * refs for actions/setup-node ...
    syncing `actions/setup-node`
    error creating github repository `actions/setup-node`: error creating repository actions/setup-node: POST https://ghaebuildXXXXXXXXXXXX.XXXXXXXX.net/api/v3/orgs/actions/repos: 403 You need admin access to the organization before adding a repository to it. []

derwasp · 2022-09-06T11:24:26Z

I can't request a review, since I don't have permissions in the repo, so I am going to tag people in the message instead.

Tagging
@TingluoHuang you created the original issue

@talktopri you reopened the original issue

@ajaykn you seem to be the maintainer

@chrisgavin as you implemented the original logic

@zarenner since you participated in the discussions about this long ago

Team
@kgrzebien @JacobMillward @cwille97 @jidicula @easyt

JacobMillward

Just a few really minor suggestions. Otherwise, looks great!

README.md

src/push.go

test/github.go

jidicula

Small change for return type idiom

src/push.go

README.md

ajaykn · 2022-09-06T16:41:27Z

README.md

 **Arguments:**

+- `actions-admin-user` _(optional)_
+   The name of the Actions admin user, which will be used for updating the chosen action. If not specified `actions-admin` will be used.


Just a basic question:

When creating a personal access token include the repo and workflow scopes. Include the site_admin scope (optional) if you want organizations to be created as necessary.

With this impersonation to work, what kind of access scope is needed ?

When I tested this manually, I used the same scopes as I would normally would for the token: public_repo (or repo for the AE instance) and workflow and it worked for me.
So it seems like nothing is changing there.

I tested it with GHES today and for whatever reason, I needed to have a site_admin scope in my token. I will investigate further.

I believe impersonation should require the site_admin scope.

@chrisgavin should this also be true for the AE instances?

AFAIK, yes.

I just verified it. Here are the example runs with the recent code change:

GHES
A failed request with no site_admin permission:

> actions-sync sync \ > --cache-dir "/tmp/cache" \ > --destination-token "ghp_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \ > --destination-url "https://ghaebuildXXXXXXXXXXXX.XXXXXXXX.net/" \ > --repo-name actions/setup-node fetching * refs for actions/setup-node ... Getting an impersonation token for `actions-admin`.. These are the scopes we have for the current token `repo, workflow`.. The current token doesn't have the `site_admin` scope. The impersonation request for GHES requres the `site_admin` permission to be able to impersonate. For GitHub AE it's not required.error obtaining the impersonation token: Failed to impersonate Actions admin user.: POST https://derwasp-03fbba5321ba8c306.qaboot.net/api/v3/admin/users/actions-admin/authorizations: 404 Not Found []

A successful request with the site_admin permission:

> actions-sync sync \ > --cache-dir "/tmp/cache" \ > --destination-token "ghp_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \ > --destination-url "https://derwasp-XXXXXXXXXXXX.XXXXXXXX.net/" \ > --repo-name actions/setup-node fetching * refs for actions/setup-node ... Getting an impersonation token for `actions-admin`.. These are the scopes we have for the current token `repo, site_admin, workflow`.. Got the impersonation token for `actions-admin`.. syncing `actions/setup-node` successfully synced `actions/setup-node`

GHAE

Note that there's no site_admin in the scopes, yet it works just fine.

> actions-sync sync \ > --cache-dir "/tmp/cache" \ > --destination-token "ghp_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \ > --destination-url "https://ghaebuildXXXXXXXXXXXX.XXXXXXXX.net/" \ > --repo-name actions/setup-node fetching * refs for actions/setup-node ... Getting an impersonation token for `actions-admin`.. These are the scopes we have for the current token `repo, workflow`.. Running against GitHub AE, changing the repository scope to 'repo'.. Got the impersonation token for `actions-admin`.. syncing `actions/setup-node` successfully synced `actions/setup-node`

Thanks @derwasp , @chrisgavin for responses.

I think for GHAE as well, not sure why the behaviour is different, but i agree with @chrisgavin that GHAE instance also should have site_admin permissions
Otherwise, normal customers with less priviliges also be able to sync via impersonation ?

@ajaykn
Who can we verify this behavior with? Unless I am missing something, right now the GHAE instances clearly are not expecting the site_admin permission for the impersonation requests as I verified with the logs above.
I don't mind just asking for the site_admin permission in both cases, but I wanted to minimize the required permissions unless really necessary.

@derwasp It seems, we are checking for site_admin even in codeql-sync tool.
https://github.com/github/codeql-action-sync-tool/blob/main/internal/push/push.go#L107

Can we do the same regardless of how it works in ghae.
Making sure site_admin permissions needed for impersonation for ghae as well.

And we can start conversation in slack channels like actions-on-ghae or ghae-engineering to see, why the impersonation logic working regardless of site_admin.

src/push.go

derwasp · 2022-09-08T09:08:57Z

for whatever reason clicking "request review" from one person removed the review request from the other person 🤔

jidicula

Tiniest of nits but otherwise LGTM

src/push.go

Same as in here https://github.com/github/codeql-action-sync-tool/blob/33463970b845ac4bb9e4cae6adc2a8b7cd6f6142/internal/push/push.go#L106-L113 Add a warning about the missing site_admin scope in the token Add a way to ignore the actions admin impersonation.

* Require the impersonation to be explicit with the --actions-admin-user flag * Reword the log messages to look the same way the existing messages are

kgrzebien

LGTM.
Nice review conversation, covering multiple topics 🚀

jidicula

LGTM 🎸

src/push.go

Also, fixed casing in the text literals

* add a note to the readme * add a note to the autogenerated cli help

ajaykn

Looks good to me.
Thanks @derwasp for adding this enhancement.

I was wondering if we can add tests here
https://github.com/actions/actions-sync/blob/main/script/test-build#L129
Not sure if the current code design allows.
We can track separately.

derwasp changed the title ~~Add a --actions-admin-user flag~~ Add actions admin user impersonation via a --actions-admin-user flag Sep 6, 2022

derwasp marked this pull request as ready for review September 6, 2022 11:19

derwasp requested a review from a team as a code owner September 6, 2022 11:19

JacobMillward suggested changes Sep 6, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

src/push.go Outdated Show resolved Hide resolved

JacobMillward previously approved these changes Sep 6, 2022

View reviewed changes

kgrzebien reviewed Sep 6, 2022

View reviewed changes

test/github.go Show resolved Hide resolved

jidicula suggested changes Sep 6, 2022

View reviewed changes

src/push.go Outdated Show resolved Hide resolved

ajaykn reviewed Sep 6, 2022

View reviewed changes

derwasp dismissed JacobMillward’s stale review via ad5e1ae September 7, 2022 06:48

chrisgavin reviewed Sep 7, 2022

View reviewed changes

src/push.go Outdated Show resolved Hide resolved

derwasp requested review from chrisgavin and jidicula and removed request for chrisgavin and jidicula September 8, 2022 09:07

jidicula previously approved these changes Sep 8, 2022

View reviewed changes

src/push.go Outdated Show resolved Hide resolved

derwasp dismissed jidicula’s stale review via 7a78691 September 9, 2022 09:17

derwasp requested review from chrisgavin and jidicula and removed request for chrisgavin and jidicula September 9, 2022 09:17

derwasp added 4 commits September 9, 2022 09:35

Add dummy routes to the test stub to mimic the github api

e4525fb

Add the compiled binary to git ignore

669526d

Add actions-admin-user to readme

c32265a

derwasp requested review from JacobMillward and kgrzebien and removed request for JacobMillward and chrisgavin September 13, 2022 12:58

JacobMillward previously approved these changes Sep 13, 2022

View reviewed changes

derwasp requested review from chrisgavin and removed request for kgrzebien September 14, 2022 11:06

Don't enable the impersonation by default.

a7d588c

* Require the impersonation to be explicit with the --actions-admin-user flag * Reword the log messages to look the same way the existing messages are

derwasp dismissed JacobMillward’s stale review via a7d588c September 20, 2022 09:48

JacobMillward previously approved these changes Sep 20, 2022

View reviewed changes

kgrzebien previously approved these changes Sep 20, 2022

View reviewed changes

jidicula previously approved these changes Sep 20, 2022

View reviewed changes

ajaykn reviewed Sep 21, 2022

View reviewed changes

src/push.go Outdated Show resolved Hide resolved

ajaykn reviewed Sep 21, 2022

View reviewed changes

src/push.go Outdated Show resolved Hide resolved

Require site_admin for the impersonation logic

37946df

Also, fixed casing in the text literals

derwasp dismissed stale reviews from jidicula, kgrzebien, and JacobMillward via 37946df September 21, 2022 11:59

Add a note about the site_admin scope

4b5fb2c

* add a note to the readme * add a note to the autogenerated cli help

ajaykn approved these changes Sep 21, 2022

View reviewed changes

ajaykn merged commit 3a24d8e into actions:main Sep 21, 2022

derwasp deleted the derwasp/add-actions-admin-user-switch branch September 21, 2022 13:07

ajaykn mentioned this pull request Sep 26, 2022

Fix the impersonation token error when site_admin scope is missing #52

Merged

Add actions admin user impersonation via a --actions-admin-user flag #46

Add actions admin user impersonation via a --actions-admin-user flag #46

Uh oh!

Conversation

derwasp commented Sep 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Background

Proposed changes

Unrelated changes

Notes

Uh oh!

derwasp commented Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JacobMillward left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jidicula left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajaykn Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

derwasp commented Sep 8, 2022

Uh oh!

jidicula left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kgrzebien left a comment

Choose a reason for hiding this comment

Uh oh!

jidicula left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ajaykn left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

derwasp commented Sep 5, 2022 •

edited

Loading

derwasp commented Sep 6, 2022 •

edited

Loading

ajaykn Sep 21, 2022 •

edited

Loading

jidicula left a comment •

edited

Loading