Handle various auth credentials, remove dashboard and network packages

[austin-denoble]

Problem

Previously, the CLI relied on the non-public dashboard APIs in order to work with organizations, projects, and API keys. These implementations were handled manually through the network and dashboard packages. In a previous PR, the code for working with admin resources were mostly replaced when implementing commands for project, organization, and api-key. However, we were still using dashboard APIs for the login and target flows.

Additionally, because of how our systems currently work with authentication, the CLI needs additional business logic for working with the control and data plane APIs when the user has authenticated via a user token (login) or a service account (pc auth configure --client-id --client-secret). Because there are limitations on where these tokens can be used across APIs surfaces, the CLI needs mechanisms for creating its own API keys, and allowing users to "store" keys for specific projects.

If you're curious or have any feedback, I've captured notes and thoughts here while working on an approach that will work for the CLI with our current authentication structure.

Solution

• remove dashboard and network packages, replace dashboard functionality in target command and elsewhere with admin API.
• implement logic for caching ManagedKeys per-project in Viper, and properly creating keys for projects as needed when instantiating the go SDK for control and data plane operations
• implement TokenManager and rename the oauth2 package to auth in utils, refactor some of the cruft around building clients and credentials

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update
• Infrastructure change (CI configs, etc)
• Non-code change (docs, etc)
• None of the above: (explain here)

Test Plan

TODO

[claude]

Claude finished @austin-denoble's task —— View job

---

PR Review Complete

I've conducted a comprehensive review of this PR which refactors the authentication system and removes the dashboard/network packages. Here are my findings:

Review Tasks:

• Analyze new authentication/token management logic
• Review credential storage and caching mechanisms
• Check security best practices in auth flows
• Evaluate error handling and edge cases
• Assess code quality and maintainability
• Review performance considerations
• Note test coverage gaps

🔍 Overall Assessment

This is a substantial and well-executed refactor that modernizes the authentication system. The new approach using the official Admin API instead of internal dashboard APIs is a significant improvement for maintainability and stability.

✅ Strengths

Architecture & Design:

• Clean separation of concerns with the new TokenManager in internal/pkg/utils/auth/token.go
• Proper use of sync.Once pattern for singleton initialization
• Well-structured credential storage with mutex protection
• Smart project-scoped API key caching in ManagedKeys

Security Best Practices:

• CSRF protection in OAuth flow (randomCSRFState())
• PKCE implementation for OAuth security
• Proper token refresh handling with prefetch window (90s before expiry)
• State validation in auth callback handler
• Secure credential storage patterns

Error Handling:

• Comprehensive error handling throughout auth flows
• Graceful fallbacks when tokens are invalid
• Clear user messaging for different auth states

⚠️ Issues & Concerns

1. Critical: Race Condition in Token Refresh

// In token.go:58-66
latestToken := secrets.GetOAuth2Token()
if !tokensEqual(t.cur, &latestToken) {
    if isEmptyToken(&latestToken) {
        t.cur = nil
    } else {
        copy := latestToken  // Potential issue here
        t.cur = &copy
    }
}

The variable shadowing with copy := latestToken could be confusing. Consider renaming to tokenCopy.

2. Security: Token Expiry Manipulation
In internal/pkg/utils/auth/token.go:85-87:

if !t.cur.Expiry.IsZero() && time.Until(t.cur.Expiry) <= t.prefetch {
    t.cur.Expiry = time.Now().Add(-time.Minute)
}

Artificially setting expiry in the past is unusual. Consider using a separate flag to track refresh state rather than manipulating the expiry time.

3. Error Handling: Inconsistent Patterns
Some functions use exit.Error() while others return errors properly. For example:

• internal/pkg/cli/command/target/target.go:278 calls exit.ErrorMsg() but also returns an error
• Consider standardizing on proper error returns for better testability

4. Resource Management: HTTP Server Cleanup
In login.go:245-254, the HTTP server cleanup could be more robust:

select {
case code := <-codeCh:
    _ = serve.Shutdown(ctx)  // Error ignored
    return code, nil
case <-ctx.Done():
    _ = serve.Shutdown(ctx)  // Error ignored

5. Code Quality: Magic Numbers

• Hard-coded port 59049 in login server
• Default prefetch of 90 * time.Second
• Should be configurable constants

🔧 Recommendations

Security Enhancements:

1. Consider implementing token rotation/invalidation for CLI-created keys
2. Add validation for the redirect URI to prevent open redirect attacks
3. Implement rate limiting for auth attempts

Code Quality:

1. Add comprehensive unit tests for TokenManager (currently minimal test coverage - only 2 test files found)
2. Extract magic numbers to named constants
3. Consider dependency injection for better testability
4. Add integration tests for the complete auth flows

Performance:

1. The current implementation is efficient with proper caching
2. Consider adding metrics/telemetry for auth operations
3. The mutex usage is appropriate and shouldn't cause performance issues

📋 Test Coverage Analysis

Critical Gap: This PR has minimal test coverage (only 2 test files found in entire repo). Key areas needing tests:

• TokenManager refresh logic
• Credential storage/retrieval
• Project key caching mechanisms
• Auth flow error handling
• OAuth callback validation

🎯 Action Items

1. High Priority: Add unit tests for TokenManager class
2. High Priority: Test the credential caching logic in secrets.go
3. Medium: Extract hard-coded values to constants
4. Medium: Standardize error handling patterns
5. Low: Improve HTTP server shutdown error handling

💡 Minor Improvements

• Consider adding debug logging for auth state transitions
• Document the ManagedKey lifecycle in comments
• Add validation for empty project IDs in key caching

Despite these issues, this is a solid refactor that significantly improves the codebase architecture. The move away from internal APIs to public Admin APIs is excellent for long-term maintainability.

[claude]

Claude finished @austin-denoble's task —— View job

---

PR Review Complete

I've conducted a comprehensive review of this PR which refactors the authentication system and removes the dashboard/network packages. Here are my findings:

Review Tasks:

• Analyze new authentication/token management logic
• Review credential storage and caching mechanisms
• Check security best practices in auth flows
• Evaluate error handling and edge cases
• Assess code quality and maintainability
• Review performance considerations
• Note test coverage gaps

🔍 Overall Assessment

This is a substantial and well-executed refactor that modernizes the authentication system. The new approach using the official Admin API instead of internal dashboard APIs is a significant improvement for maintainability and stability.

✅ Strengths

Architecture & Design:

• Clean separation of concerns with the new TokenManager in internal/pkg/utils/auth/token.go
• Proper use of sync.Once pattern for singleton initialization
• Well-structured credential storage with mutex protection
• Smart project-scoped API key caching in ManagedKeys

Security Best Practices:

• CSRF protection in OAuth flow (randomCSRFState())
• PKCE implementation for OAuth security
• Proper token refresh handling with prefetch window (90s before expiry)
• State validation in auth callback handler
• Secure credential storage patterns

Error Handling:

• Comprehensive error handling throughout auth flows
• Graceful fallbacks when tokens are invalid
• Clear user messaging for different auth states

⚠️ Issues & Concerns

1. Critical: Race Condition in Token Refresh

// In token.go:58-66
latestToken := secrets.GetOAuth2Token()
if !tokensEqual(t.cur, &latestToken) {
    if isEmptyToken(&latestToken) {
        t.cur = nil
    } else {
        copy := latestToken  // Potential issue here
        t.cur = &copy
    }
}

The variable shadowing with copy := latestToken could be confusing. Consider renaming to tokenCopy.

2. Security: Token Expiry Manipulation
In internal/pkg/utils/auth/token.go:85-87:

if !t.cur.Expiry.IsZero() && time.Until(t.cur.Expiry) <= t.prefetch {
    t.cur.Expiry = time.Now().Add(-time.Minute)
}

Artificially setting expiry in the past is unusual. Consider using a separate flag to track refresh state rather than manipulating the expiry time.

3. Error Handling: Inconsistent Patterns
Some functions use exit.Error() while others return errors properly. For example:

• internal/pkg/cli/command/target/target.go:278 calls exit.ErrorMsg() but also returns an error
• Consider standardizing on proper error returns for better testability

4. Resource Management: HTTP Server Cleanup
In login.go:245-254, the HTTP server cleanup could be more robust:

select {
case code := <-codeCh:
    _ = serve.Shutdown(ctx)  // Error ignored
    return code, nil
case <-ctx.Done():
    _ = serve.Shutdown(ctx)  // Error ignored

5. Code Quality: Magic Numbers

• Hard-coded port 59049 in login server
• Default prefetch of 90 * time.Second
• Should be configurable constants

🔧 Recommendations

Security Enhancements:

1. Consider implementing token rotation/invalidation for CLI-created keys
2. Add validation for the redirect URI to prevent open redirect attacks
3. Implement rate limiting for auth attempts

Code Quality:

1. Add comprehensive unit tests for TokenManager (currently minimal test coverage - only 2 test files found)
2. Extract magic numbers to named constants
3. Consider dependency injection for better testability
4. Add integration tests for the complete auth flows

Performance:

1. The current implementation is efficient with proper caching
2. Consider adding metrics/telemetry for auth operations
3. The mutex usage is appropriate and shouldn't cause performance issues

📋 Test Coverage Analysis

Critical Gap: This PR has minimal test coverage (only 2 test files found in entire repo). Key areas needing tests:

• TokenManager refresh logic
• Credential storage/retrieval
• Project key caching mechanisms
• Auth flow error handling
• OAuth callback validation

🎯 Action Items

1. High Priority: Add unit tests for TokenManager class
2. High Priority: Test the credential caching logic in secrets.go
3. Medium: Extract hard-coded values to constants
4. Medium: Standardize error handling patterns
5. Low: Improve HTTP server shutdown error handling

💡 Minor Improvements

• Consider adding debug logging for auth state transitions
• Document the ManagedKey lifecycle in comments
• Add validation for empty project IDs in key caching

Despite these issues, this is a solid refactor that significantly improves the codebase architecture. The move away from internal APIs to public Admin APIs is excellent for long-term maintainability.

---

[claude]

Claude finished @austin-denoble's task —— View job

---

PR Review Complete

I've conducted a comprehensive review of this PR which refactors the authentication system and removes the dashboard/network packages. Here are my findings:

Review Tasks:

• Analyze new authentication/token management logic
• Review credential storage and caching mechanisms
• Check security best practices in auth flows
• Evaluate error handling and edge cases
• Assess code quality and maintainability
• Review performance considerations
• Note test coverage gaps

🔍 Overall Assessment

This is a substantial and well-executed refactor that modernizes the authentication system. The new approach using the official Admin API instead of internal dashboard APIs is a significant improvement for maintainability and stability.

✅ Strengths

Architecture & Design:

• Clean separation of concerns with the new TokenManager in internal/pkg/utils/auth/token.go
• Proper use of sync.Once pattern for singleton initialization
• Well-structured credential storage with mutex protection
• Smart project-scoped API key caching in ManagedKeys

Security Best Practices:

• CSRF protection in OAuth flow (randomCSRFState())
• PKCE implementation for OAuth security
• Proper token refresh handling with prefetch window (90s before expiry)
• State validation in auth callback handler
• Secure credential storage patterns

Error Handling:

• Comprehensive error handling throughout auth flows
• Graceful fallbacks when tokens are invalid
• Clear user messaging for different auth states

⚠️ Issues & Concerns

1. Critical: Race Condition in Token Refresh

// In token.go:58-66
latestToken := secrets.GetOAuth2Token()
if !tokensEqual(t.cur, &latestToken) {
    if isEmptyToken(&latestToken) {
        t.cur = nil
    } else {
        copy := latestToken  // Potential issue here
        t.cur = &copy
    }
}

The variable shadowing with copy := latestToken could be confusing. Consider renaming to tokenCopy.

2. Security: Token Expiry Manipulation
In internal/pkg/utils/auth/token.go:85-87:

if !t.cur.Expiry.IsZero() && time.Until(t.cur.Expiry) <= t.prefetch {
    t.cur.Expiry = time.Now().Add(-time.Minute)
}

Artificially setting expiry in the past is unusual. Consider using a separate flag to track refresh state rather than manipulating the expiry time.

3. Error Handling: Inconsistent Patterns
Some functions use exit.Error() while others return errors properly. For example:

• internal/pkg/cli/command/target/target.go:278 calls exit.ErrorMsg() but also returns an error
• Consider standardizing on proper error returns for better testability

4. Resource Management: HTTP Server Cleanup
In login.go:245-254, the HTTP server cleanup could be more robust:

select {
case code := <-codeCh:
    _ = serve.Shutdown(ctx)  // Error ignored
    return code, nil
case <-ctx.Done():
    _ = serve.Shutdown(ctx)  // Error ignored

5. Code Quality: Magic Numbers

• Hard-coded port 59049 in login server
• Default prefetch of 90 * time.Second
• Should be configurable constants

🔧 Recommendations

Security Enhancements:

1. Consider implementing token rotation/invalidation for CLI-created keys
2. Add validation for the redirect URI to prevent open redirect attacks
3. Implement rate limiting for auth attempts

Code Quality:

1. Add comprehensive unit tests for TokenManager (currently minimal test coverage - only 2 test files found)
2. Extract magic numbers to named constants
3. Consider dependency injection for better testability
4. Add integration tests for the complete auth flows

Performance:

1. The current implementation is efficient with proper caching
2. Consider adding metrics/telemetry for auth operations
3. The mutex usage is appropriate and shouldn't cause performance issues

📋 Test Coverage Analysis

Critical Gap: This PR has minimal test coverage (only 2 test files found in entire repo). Key areas needing tests:

• TokenManager refresh logic
• Credential storage/retrieval
• Project key caching mechanisms
• Auth flow error handling
• OAuth callback validation

🎯 Action Items

1. High Priority: Add unit tests for TokenManager class
2. High Priority: Test the credential caching logic in secrets.go
3. Medium: Extract hard-coded values to constants
4. Medium: Standardize error handling patterns
5. Low: Improve HTTP server shutdown error handling

💡 Minor Improvements

• Consider adding debug logging for auth state transitions
• Document the ManagedKey lifecycle in comments
• Add validation for empty project IDs in key caching

Despite these issues, this is a solid refactor that significantly improves the codebase architecture. The move away from internal APIs to public Admin APIs is excellent for long-term maintainability.

---