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? |
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"
The conditional in add_default_params is always try (it's a single =) so statement.principal is set to the GitHub Actions principal (https://token.actions.githubusercontent.com) every time the new API Key Role form is loaded. That field is hidden on the form so the user doesn't have a chance to edit it, and after saving the created role has a provider of Buildkite with an expected principal for GitHub Actions. An alternative solution would be 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 Actionas *and* Buildkite.
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.17% -2.89%
==========================================
Files 451 451
Lines 9391 9450 +59
==========================================
- Hits 9115 8900 -215
- Misses 276 550 +274 ☔ View full report in Codecov by Sentry. |
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:
Currently the fix is to change a conditional to use
==instead of=. However, it's likely that will break some other tests. A possible alternative fix is:The principal input is hidden on the form anyway, and will be set to a default value in the
OIDC::ApiKeyRole#set_statement_principalsmodel callbackFixes #5376