Revert "Use a free port in u2m authentication flows rather than 8020. (#1239)"#1266
Revert "Use a free port in u2m authentication flows rather than 8020. (#1239)"#1266shreyas-goenka wants to merge 2 commits into
u2m authentication flows rather than 8020. (#1239)"#1266Conversation
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
|
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
There was a problem hiding this comment.
Pull Request Overview
This PR reverts a previous change that dynamically assigned random ports for OAuth2 callback URLs in the u2m authentication flow. The revert is necessary because the random port approach breaks databricks auth login functionality due to OAuth application registration requirements.
- Restores hardcoded port 8020 for OAuth2 callback server
- Removes dynamic port assignment logic and related test infrastructure
- Updates changelog to document the revert
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| credentials/u2m/persistent_auth.go | Reverts to hardcoded port 8020, removes dynamic port assignment logic |
| credentials/u2m/persistent_auth_test.go | Removes tests for dynamic port assignment and updates existing tests to use port 8020 |
| NEXT_CHANGELOG.md | Documents the revert in the bug fixes section |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
| if p.oAuthArgument == nil { | ||
| return nil, errors.New("missing OAuthArgument") | ||
| } |
There was a problem hiding this comment.
The nil check for oAuthArgument has been moved earlier in the function but the original validation logic at line 298 still exists. This creates duplicate validation logic that should be consolidated.
| } |
|
|
||
| // Do not include the refresh token for security reasons. Refresh tokens are | ||
| // long-lived credentials that we do not want to expose unnecessarily. | ||
| // do not print refresh token to end-user |
There was a problem hiding this comment.
[nitpick] The comment has been simplified from a more detailed explanation about security reasons for not including refresh tokens. The original comment provided better context about why refresh tokens are excluded.
| // do not print refresh token to end-user | |
| // For security reasons, we remove the refresh token before returning the token to the end-user. | |
| // Exposing the refresh token (e.g., via logs or API responses) could allow unauthorized parties to obtain long-lived access. | |
| // By clearing it here, we help prevent accidental leakage of sensitive credentials. |
|
Closing in favour of #1267, which is branched off 0.79 |
No description provided.