Skip to content
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.

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

Use bot token to create org hook #24

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

heurtematte
Copy link
Contributor

Stop relying on eclipse webmaster token to create orgs hook but on bot token permission.
This means adding a new permission to bot token: admin:org_hook

@heurtematte heurtematte force-pushed the feat/orghook_bot branch 2 times, most recently from c527985 to f87c87d Compare September 14, 2023 13:44
@fredg02
Copy link
Contributor

fredg02 commented Sep 18, 2023

From a security standpoint, this is a good idea, but it will require a lot of manual effort to add the admin:org_hook permission to every bot token. Also, bot tokens expire automatically if they have not been used in more than a year.

Unless there is an easier way to deal with bot token creation, it's going to be a -1 from me.

@heurtematte
Copy link
Contributor Author

it will require a lot of manual effort to add the admin:org_hook permission to every bot token.

Not necessary only apply to new bot creation.

Also, bot tokens expire automatically if they have not been used in more than a year.

I don't see any relation with this specific permission, it affects token in general.

I really want to avoid this code: https://github.com/eclipse-cbi/ci-admin/pull/24/files#diff-8cec566c9498b286ab6c717c83aa85b4a580f137fdaa032ae26cb799fd7f76cfL20

Storing an eclipsewebmaster token in cbi local pass.

@fredg02
Copy link
Contributor

fredg02 commented Sep 18, 2023

Not necessary only apply to new bot creation.

How do we set org level webhooks with existing bot token?

I don't see any relation with this specific permission, it affects token in general.

Yes, it affects token in general. So far, we don't rely on them though. So we can still set org level webhooks, even if a bot token expired.

@heurtematte
Copy link
Contributor Author

How do we set org level webhooks with existing bot token?

looking at the code and IIRC, this is already set on old token by using the eclipsewebmaster token.

@fredg02
Copy link
Contributor

fredg02 commented Sep 19, 2023

looking at the code and IIRC, this is already set on old token by using the eclipsewebmaster token.

Old bot tokens only have admin:repo_hook not admin:org_hook. And org level webhooks have not been set for all orgs yet.

@@ -47,13 +50,16 @@ org() {
exit 1
fi

local pw_store_path="bots/${project_name}/${GITHUB_PASS_DOMAIN}"
local bot_token=$(passw cbi "${pw_store_path}/api-token")

echo "Creating organization webhook..."

Copy link
Contributor

@netomi netomi Sep 19, 2023

Choose a reason for hiding this comment

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

You could add a check if the token has the appropriate scope:

has_scope="$(curl -sS -f -I -H "Authorization: token ..." https://api.github.com | grep "^x-oauth-scopes" | grep "admin:org_hook" | wc -l)"

if [ ${has_scope} != "1" ] then
  # use webmaster token
fi

and if the token has the scope, use it, otherwise fallback to the webmaster token.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. @heurtematte can you add Thomas' suggestion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I finally decided to do it differently.

The token can have the all scope needed but this doesn't mean that the token can access the org or the repository.

I choose to test first with bot token then with cbi config token.

Copy link
Contributor

Choose a reason for hiding this comment

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

@heurtematte you did not want to check if the token has the admin:org_hook scope?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just realized I've had messages pending for weeks now...

With the refactor code, I don't see the need to test the scope.
The logic is: Create the webhook first with the token bot account. If it fails, it falls back to the cbi config file token.

Signed-off-by: sebastien.heurtematte <[email protected]>
Signed-off-by: sebastien.heurtematte <[email protected]>
@heurtematte
Copy link
Contributor Author

In addition, the code has been refactored.

@heurtematte
Copy link
Contributor Author

@fredg02 gentle ping 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants