Make sure runner-admin has both auth_url and auth_url_v2. #4066
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements dual authorization URL support for runner-admin by introducing a new LegacyAuthorizationUrl property alongside the existing AuthorizationUrl. The change ensures backward compatibility during authentication migration by allowing runners to use both legacy and v2 authorization endpoints.
- Added
LegacyAuthorizationUrlproperty to the Runner WebApi model - Updated configuration logic to handle both authorization URLs when legacy URL is available
- Enabled v2 flow and set appropriate migration flags when using runner-admin
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/Sdk/DTWebApi/WebApi/Runner.cs | Added LegacyAuthorizationUrl property to support dual authorization URLs |
| src/Runner.Listener/Configuration/ConfigurationManager.cs | Updated configuration logic to use legacy URL when available and set migration properties |
| ClientId = new Guid(runner.RunnerAuthorization.ClientId) | ||
| }; | ||
|
|
||
| if (!string.IsNullOrEmpty(runner.RunnerAuthorization.LegacyAuthorizationUrl.AbsoluteUri)) |
There was a problem hiding this comment.
Potential null reference exception when accessing LegacyAuthorizationUrl.AbsoluteUri if LegacyAuthorizationUrl is null. Check if LegacyAuthorizationUrl is not null before accessing its AbsoluteUri property.
| if (!string.IsNullOrEmpty(runner.RunnerAuthorization.LegacyAuthorizationUrl.AbsoluteUri)) | |
| if (runner.RunnerAuthorization.LegacyAuthorizationUrl != null && | |
| !string.IsNullOrEmpty(runner.RunnerAuthorization.LegacyAuthorizationUrl.AbsoluteUri)) |
| ClientId = new Guid(runner.RunnerAuthorization.ClientId) | ||
| }; | ||
|
|
||
| if (!string.IsNullOrEmpty(runner.RunnerAuthorization.LegacyAuthorizationUrl.AbsoluteUri)) |
There was a problem hiding this comment.
Potential null reference exception when accessing LegacyAuthorizationUrl.AbsoluteUri if LegacyAuthorizationUrl is null. Check if LegacyAuthorizationUrl is not null before accessing its AbsoluteUri property.
| if (!string.IsNullOrEmpty(runner.RunnerAuthorization.LegacyAuthorizationUrl.AbsoluteUri)) | |
| if (runner.RunnerAuthorization.LegacyAuthorizationUrl != null && | |
| !string.IsNullOrEmpty(runner.RunnerAuthorization.LegacyAuthorizationUrl.AbsoluteUri)) |
| if (!string.IsNullOrEmpty(runner.RunnerAuthorization.LegacyAuthorizationUrl.AbsoluteUri)) | ||
| { | ||
| agent.Authorization.AuthorizationUrl = runner.RunnerAuthorization.LegacyAuthorizationUrl; | ||
| agent.Properties["EnableAuthMigrationByDefault"] = false; | ||
| agent.Properties["AuthorizationUrlV2"] = runner.RunnerAuthorization.AuthorizationUrl.AbsoluteUri; | ||
| } |
There was a problem hiding this comment.
The same logic for handling legacy authorization URL is duplicated in two locations. Consider extracting this into a helper method to reduce code duplication and improve maintainability.
| if (!string.IsNullOrEmpty(runner.RunnerAuthorization.LegacyAuthorizationUrl.AbsoluteUri)) | ||
| { | ||
| agent.Authorization.AuthorizationUrl = runner.RunnerAuthorization.LegacyAuthorizationUrl; | ||
| agent.Properties["EnableAuthMigrationByDefault"] = false; | ||
| agent.Properties["AuthorizationUrlV2"] = runner.RunnerAuthorization.AuthorizationUrl.AbsoluteUri; | ||
| } |
There was a problem hiding this comment.
The same logic for handling legacy authorization URL is duplicated in two locations. Consider extracting this into a helper method to reduce code duplication and improve maintainability.
6d1fead to
c8b6702
Compare
c8b6702 to
37ce6fa
Compare
| { | ||
| var runner = await _dotcomServer.ReplaceRunnerAsync(runnerSettings.PoolId, agent, runnerSettings.GitHubUrl, registerToken, publicKeyXML); | ||
| runnerSettings.ServerUrlV2 = runner.RunnerAuthorization.ServerUrl; | ||
| runnerSettings.UseV2Flow = true; // if we are using runner admin, we also need to hit broker |
There was a problem hiding this comment.
we forgot to do this for the update-runner flow, the same code exists in the add-runner flow.
https://github.com/github/actions-runner-admin/issues/2034