Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
ajaykn merged 3 commits into from
Sep 26, 2022
Merged

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

ajaykn merged 3 commits into from
Sep 26, 2022

Conversation

@ajaykn
Copy link
Contributor

@ajaykn ajaykn commented Sep 26, 2022 •
edited
Loading

A recent PR: #46 added the support of impersonation.
For this, we need site_admin permissions needed.

When checking the site_admin scope, if missing, the error is wrapped with err
return "", errors.Wrap(err, "the current token does..)
But this doesn't work if err is nil.

we have to throw a new error in this case.

Before fix:

getting an impersonation token for `actions-admin` ...
these are the scopes we have for the current token `admin:org, repo` ...
syncing `actions/setup-node`
error creating github repository `actions/setup-node`: error retrieving authenticated user: GET https://h2floh-080c6ccdb7ad8f2c4.ghe-test.com/api/v3/user: 401 Bad credentials []

After fix:

getting an impersonation token for `actions-admin` ...
these are the scopes we have for the current token `admin:org, repo` ...
error obtaining the impersonation token: the current token doesn't have the `site_admin` scope, the impersonation function requires the `site_admin` permission to be able to impersonate.

Also in Readme.md, moved optional argument (--actions-admin-user) after required fields section.

@ajaykn ajaykn requested a review from a team as a code owner September 26, 2022 04:19
@ajaykn ajaykn requested a review from derwasp September 26, 2022 04:19
nicholasbergesen
nicholasbergesen approved these changes Sep 26, 2022
View reviewed changes
Copy link

@nicholasbergesen nicholasbergesen left a comment

Choose a reason for hiding this comment

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

lgtm

@ajaykn ajaykn merged commit 621a30f into main Sep 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@derwasp derwasp derwasp approved these changes

@nicholasbergesen nicholasbergesen nicholasbergesen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ajaykn @derwasp @nicholasbergesen