Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cargo-shuttle): automatic login via console, login --prompt #1913

Merged
merged 12 commits into from
Nov 8, 2024

Conversation

jonaro00
Copy link
Member

@jonaro00 jonaro00 commented Nov 1, 2024

No description provided.

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

This PR implements device authentication flow and manual API key input for the Shuttle CLI. Here are the key changes:

  • Added new login.rs module implementing device auth flow with a local HTTP callback server for receiving API keys from Shuttle Console
  • Added --input flag to login command allowing manual API key entry as an alternative to browser-based auth
  • Updated API URL constants and dependencies to support both legacy (.rs) and new (.dev) platforms with appropriate HTTP handling

7 file(s) reviewed, 10 comment(s)
Edit PR Review Bot Settings | Greptile

cargo-shuttle/Cargo.toml Outdated Show resolved Hide resolved
cargo-shuttle/Cargo.toml Outdated Show resolved Hide resolved
cargo-shuttle/Cargo.toml Outdated Show resolved Hide resolved
cargo-shuttle/src/args.rs Outdated Show resolved Hide resolved
cargo-shuttle/src/lib.rs Show resolved Hide resolved
cargo-shuttle/src/login.rs Outdated Show resolved Hide resolved
cargo-shuttle/src/login.rs Outdated Show resolved Hide resolved
cargo-shuttle/src/login.rs Outdated Show resolved Hide resolved
common/src/constants.rs Outdated Show resolved Hide resolved
common/src/constants.rs Show resolved Hide resolved
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

(updates since last review)

Based on the provided files and previous review, here's my analysis of the key changes in this PR:

  • Added device auth server in login.rs that lacks proper error handling and cleanup for failed connections
  • Introduced potential security concerns with unrestricted CORS headers in device auth callback handler
  • Added hyper 1.0 dependencies alongside workspace hyper which could lead to compatibility issues

The changes focus on implementing device authentication but have some technical concerns that should be addressed around security and reliability.

I'll keep my comments brief and focused on the most critical points not already covered in the previous review.

7 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile

cargo-shuttle/src/args.rs Outdated Show resolved Hide resolved
cargo-shuttle/src/login.rs Outdated Show resolved Hide resolved
cargo-shuttle/src/login.rs Outdated Show resolved Hide resolved
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

(updates since last review)

Based on the recent changes and previous reviews, here's my summary of the key updates:

Simplified device authentication callback URL format in cargo-shuttle, focusing on port-based routing instead of full URL encoding.

  • Modified callback URL format in /cargo-shuttle/src/login.rs from URL-encoded to simple port parameter
  • Kept unrestricted CORS headers (Access-Control-Allow-Origin: *) in callback handler without addressing security concerns
  • No changes to address previously identified connection cleanup and error handling issues

The changes make the URL format cleaner but don't address the core technical concerns around security and reliability that were previously raised.

1 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile

cargo-shuttle/src/login.rs Outdated Show resolved Hide resolved
cargo-shuttle/src/login.rs Outdated Show resolved Hide resolved
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

(updates since last review)

Based on the recent changes and previous reviews, here's my summary of the key updates:

  • Added new auth models in common/src/models/auth.rs for handling token and API key messages in device authentication flow
  • Updated API URL constants in common/src/constants.rs to support both legacy (.rs) and new (.dev) platforms consistently
  • Modified cargo-shuttle/src/args.rs to support both automatic console login and manual API key input with improved command organization

The changes focus on implementing the core authentication data structures and platform configuration, while maintaining compatibility with both legacy and new platforms.

13 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings | Greptile

cargo-shuttle/src/lib.rs Show resolved Hide resolved
common/src/constants.rs Show resolved Hide resolved
common/src/models/auth.rs Show resolved Hide resolved
common/src/models/auth.rs Show resolved Hide resolved
resources/persist/Cargo.toml Show resolved Hide resolved
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

(updates since last review)

Based on the recent changes and previous reviews, here's my summary of the key updates:

This PR enhances the authentication flow in the Shuttle CLI with improved error handling and user experience.

  • Added descriptive error context in api-client/src/lib.rs for WebSocket connection failures during device authentication
  • Improved command-line help organization in args.rs by hiding development flags and adding clearer section headings
  • Enhanced console login display in lib.rs with better token formatting and readability

I'll keep my comments focused on points not already covered in previous reviews.

4 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

cargo-shuttle/src/lib.rs Outdated Show resolved Hide resolved
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

(updates since last review)

Based on the recent changes to cargo-shuttle/src/lib.rs and previous reviews, here's my summary of the key updates:

  • Added WebSocket-based device authentication flow in cargo-shuttle/src/lib.rs with improved error handling and user feedback
  • Enhanced login command with automatic console redirection and manual API key input options
  • Improved platform compatibility by supporting both legacy (.rs) and new (.dev) authentication flows

I'll keep my comments focused on points not already covered in previous reviews.

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

(updates since last review)

Based on the recent changes to cargo-shuttle/src/lib.rs and previous reviews, here's my summary of the key updates:

  • Added WebSocket ping/pong mechanism in cargo-shuttle/src/lib.rs to maintain connection stability during authentication
  • Split WebSocket connection into separate tx/rx streams for better error isolation and handling
  • Enhanced error reporting for different WebSocket message types (Text, Pong, Close) with more descriptive context

I've focused only on the most recent changes not covered in previous reviews, keeping the summary concise and specific to the latest modifications.

1 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile

cargo-shuttle/src/lib.rs Show resolved Hide resolved
cargo-shuttle/src/lib.rs Outdated Show resolved Hide resolved
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

(updates since last review)

Based on the recent changes to cargo-shuttle/src/lib.rs and previous reviews, here's my summary of the key updates:

  • Added read_ws_until_text helper function in cargo-shuttle/src/lib.rs to streamline WebSocket message handling and improve code organization
  • Enhanced WebSocket error handling with specific context for different message types (Text, Binary, Close)
  • Improved connection cleanup by properly handling WebSocket close frames and error conditions

I'll keep my comments focused on points not already covered in previous reviews.

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

(updates since last review)

Based on the recent changes and previous reviews, here's my summary of the key updates:

Moved WebSocket message handling logic to improve code organization and reliability in cargo-shuttle.

  • Moved read_ws_until_text from lib.rs to util.rs for better code organization and reusability
  • Enhanced WebSocket stream handling with proper type bounds (StreamExt + Unpin) in util.rs
  • Added explicit error propagation for WebSocket message handling in lib.rs

I've focused only on the most recent changes not covered in previous reviews, keeping the summary concise and specific to the latest modifications.

2 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile

cargo-shuttle/src/util.rs Show resolved Hide resolved
cargo-shuttle/src/util.rs Outdated Show resolved Hide resolved
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

(updates since last review)

Based on the recent changes and previous reviews, here's my summary of the key updates:

The PR simplifies WebSocket message handling in the Shuttle CLI's authentication flow.

  • Simplified trait bounds in cargo-shuttle/src/util.rs by combining Stream and StreamExt into a single bound for read_ws_until_text
  • Improved type constraints for WebSocket stream handling with cleaner syntax while maintaining functionality
  • Streamlined message filtering to only process text messages in the WebSocket stream

I've focused only on the most recent changes not covered in previous reviews, keeping the summary concise and specific to the latest modifications.

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

(updates since last review)

Based on the recent changes and previous reviews, here's my summary of the key updates:

  • Renamed input field to prompt in cargo-shuttle/src/args.rs LoginArgs struct while maintaining backward compatibility via alias
  • Updated login command help text to better reflect the manual API key input option
  • Added proper conflict handling between prompt and api_key options in LoginArgs struct

I've focused only on the most recent changes not covered in previous reviews, keeping the summary concise and specific to the latest modifications.

2 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

cargo-shuttle/src/lib.rs Show resolved Hide resolved
@jonaro00 jonaro00 changed the title feat(cargo-shuttle): automatic login via console, login --input feat(cargo-shuttle): automatic login via console, login --prompt Nov 8, 2024
@jonaro00 jonaro00 merged commit 792e681 into main Nov 8, 2024
31 of 33 checks passed
@jonaro00 jonaro00 deleted the device-auth branch November 8, 2024 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants