New system test creating an API Key Role for Buildkite #5434
Conversation
| @@ -134,7 +134,7 @@ def add_default_params(rubygem, statement, condition) | |||
|
|
|||
| return unless rubygem | |||
| return unless (gh = helpers.link_to_github(rubygem)).presence | |||
| return unless (@api_key_role.provider = OIDC::Provider.github_actions) | |||
| return unless (@api_key_role.provider == OIDC::Provider.github_actions) | |||
There was a problem hiding this comment.
Is the conditional with assignment instead of equality intentional or accidental? 🤔
There was a problem hiding this comment.
it intentionally was an assignment, this is where the provider would get assigned (as a default, I think)
| @@ -134,7 +134,7 @@ def add_default_params(rubygem, statement, condition) | |||
|
|
|||
| return unless rubygem | |||
| return unless (gh = helpers.link_to_github(rubygem)).presence | |||
| return unless (@api_key_role.provider = OIDC::Provider.github_actions) | |||
| return unless (@api_key_role.provider == OIDC::Provider.github_actions) | |||
There was a problem hiding this comment.
it intentionally was an assignment, this is where the provider would get assigned (as a default, I think)
|
ah, fair enough then! From the outside it looked like the expression would never fail so might not have been intentional. Would you prefer the alternative, where we just remove the hard coded principal assignment, and let than happen on form submit instead? |
yob
force-pushed
the
non-github-actions-api-key-role
branch
from
62605db to
d810930
Compare
|
I've rebased onto master and force pushed, to remove the commit from #5416 |
I think that's also OK! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5434 +/- ##
==========================================
- Coverage 97.06% 94.26% -2.80%
==========================================
Files 451 451
Lines 9392 9450 +58
==========================================
- Hits 9116 8908 -208
- Misses 276 542 +266 ☔ View full report in Codecov by Sentry. |
yob
force-pushed
the
non-github-actions-api-key-role
branch
from
d810930 to
22cbccc
Compare
The form and POSTing to create works, but the resulting API Key Role has a condition that expects a principal of "https://token.actions.githubusercontent.com"
When a gem has a source doe URI for GitHub, `add_default_params` defaults the form to common settings for GitHub OIDC tokens. That's a reasonable thing to do. However, that includes setting the principal to a Github Actions shaped value. That value is rendered on the form in hidden elements so the user doesn't have a chance to edit it. After saving the created role has a provider of Buildkite with an expected principal for GitHub Actions. Its safe to remove the statement.principal assignment completely. It's not required - when the form is submitted the OIDC::ApiKeyRole#set_statement_principals callback will set the correct principal for both GitHub Actions *and* Buildkite.
yob
force-pushed
the
non-github-actions-api-key-role
branch
from
22cbccc to
d92a7bc
Compare
|
I think this is ready for another look @segiddins. I decided to go with the alternate fix, removing the |
While working on rubygems#5434 I found the other system tests in this file were flakey on my local laptop. Adding this assertion to force the test to wait for the sign in to finish before proceeding makes them pass reliabily.
While working on rubygems#5434 I found the other system tests in this file were flakey on my local laptop. Adding this assertion to force the test to wait for the sign in to finish before proceeding makes them pass reliabily.
While working on #5434 I found the other system tests in this file were flakey on my local laptop. Adding this assertion to force the test to wait for the sign in to finish before proceeding makes them pass reliabily.
Note: depends on #5416. Once that merges, I can rebase this and the first commit will fall awayAt the moment creating an API Key Role for Buildkite on rubygems.org results in an Access Policy that requires a GitHub Actions principal:
https://token.actions.githubusercontent.com. To fix it, I have to inspect the DOM on the new form and make the principal input visible, then change it tohttps://agent.buildkite.com.This PR has two commits:
When a gem has a source doe URI for GitHub,
add_default_paramsdefaults the form to common settings for GitHub OIDC tokens.That's a reasonable thing to do. However, that includes setting the principal to a Github Actions shaped value. That value is rendered on the form in hidden elements so the user doesn't have a chance to edit it. After saving the created role has a provider of Buildkite with an expected principal for GitHub Actions.
Its safe to remove the statement.principal assignment completely. It's not required - when the form is submitted the
OIDC::ApiKeyRole#set_statement_principals callback will set the correct principal for both GitHub Actions and Buildkite.
Fixes #5376