-
Notifications
You must be signed in to change notification settings - Fork 76
Add comprehensive Basic Authentication example #657
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
base: main
Are you sure you want to change the base?
Conversation
- Add examples/basic_auth.rs demonstrating three approaches to Basic Auth - Add complete documentation in src/docs/goose-book/src/example/basic-auth.md - Update documentation structure and changelog - Addresses static asset authentication problem from issue tag1consulting#608 Closes tag1consulting#608
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📋 Review Summary
This pull request introduces a comprehensive example for handling Basic Authentication in Goose load tests. The changes are well-structured, with clear code and thorough documentation. The addition of three different approaches to Basic Auth is particularly valuable for users.
🔍 General Feedback
- The new
basic_auth.rsexample is excellent. It's well-commented and clearly demonstrates the different methods for handling Basic Authentication. - The accompanying documentation is also very well-written and provides a great explanation of the problem and the solutions.
- The use of environment variables for credentials is a good security practice.
I have one minor suggestion to make the code in the example slightly more efficient, but overall this is a great addition to the project.
|
There is a problem with the Gemini CLI PR review. Please check the action logs for details. |
Explain that production code could use the base64 crate instead of the custom implementation, but we avoided it to keep the example self-contained without adding extra dependencies just for demonstration purposes.
- Wrap entire example in cookies feature flag to avoid cookie_store() compilation errors - Provide clear error message when cookies feature is disabled - Use clean module-based approach with minimal #[cfg] statements - Fixes compilation error from PR tag1consulting#657 The basic_auth example demonstrates session management with Basic Authentication, which requires cookie support to maintain authentication state across requests. When cookies are disabled, users get helpful guidance on how to enable them.
| - [Closure](closure.md) *([examples/closure.rs](https://github.com/tag1consulting/goose/blob/main/examples/closure.rs))* | ||
| - [Session](session.md) *([examples/session.rs](https://github.com/tag1consulting/goose/blob/main/examples/session.rs))* | ||
| - [Basic Authentication](basic-auth.md) *([examples/basic_auth.rs](https://github.com/tag1consulting/goose/blob/main/examples/basic_auth.rs))* | ||
| - [GraphQL](graphql.md) *([examples/graphql_loadtest.rs](https://github.com/tag1consulting/goose/blob/main/examples/graphql_loadtest.rs))* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - unrelated change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, but we missed it in the earlier PR that was already merged, and it feels like overkill to create a new PR for this change.
|
|
||
| /// Simple base64 encoding function to avoid external dependencies | ||
| /// | ||
| /// Note: In production code, you could use the `base64` crate for this functionality: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"you should use - this example implementation has limitations and is not meant for production usage."
-> needs to be emphasized stronger as this implementation has issues. (particularly: "this implementation lacks padding handling and input validation, which could lead to issues with edge cases".)
LionsAd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Review: Basic Authentication Example (#657)
🎯 Core Assessment
This is a valuable and well-executed contribution that addresses a real user need (GitHub issue #608). The PR provides comprehensive guidance on Basic Authentication with three practical approaches, excellent documentation, and working code that has been tested successfully.
📋 Recommendations by Category
🔧 Technical Improvements (Quick Fixes)
Environment Variable Parsing
- Problem:
split(':')will break if password contains colons - Solution: Use
splitn(2, ':')for proper username:password parsing - Effort: One-line change in credential parsing function
Code Consistency
- Problem: Uses
println!instead of Goose logging patterns - Solution: Replace with
info!,warn!macros where appropriate - Effort: Low - simple replacements for consistency
Documentation Check
- Problem: Potential GraphQL entry duplication in overview.md
- Solution: Verify no duplicate entries exist
- Effort: Quick documentation review
🎨 Design Discussions (Working Fine As-Is)
Base64 Implementation
- Current Approach: Custom
base64_encodefunction for self-contained example - Alternative: Use
base64crate as dev dependency - Business Reality: Self-contained examples have educational value; current implementation works correctly and includes clear production guidance
Feature Flag Approach
- Current Approach: Wraps entire example in cookies feature gate
- Alternative: Only conditionally gate
cookie_store(true)call - Business Reality: Current approach provides clear user guidance and works well; over-optimization not needed
🚀 Enhancement Opportunities (Nice to Have)
Static Asset Demonstration
- Enhancement: Add
validate_and_load_static_assetscall to concretely show the static asset propagation benefit - Current: Well-documented in comments and explanations
- Value: Would strengthen the practical demonstration
Educational Clarity
- Enhancement: Brief comment explaining why custom client approach specifically solves static asset authentication
- Current: Explained in documentation
- Value: Would reinforce the key insight
(Powered by Claude Code)
LionsAd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RTBM - but the base64 recommendation should be fixed. Other improvements COULD me made.
Add comprehensive Basic Authentication example
Summary
Adds
examples/basic_auth.rsdemonstrating three approaches to HTTP Basic Authentication in Goose load tests, addressing the static asset authentication problem described in issue #608.Changes
New Files
examples/basic_auth.rs- Complete working example with three authentication approachessrc/docs/goose-book/src/example/basic-auth.md- Comprehensive documentationsrc/docs/goose-book/src/SUMMARY.mdandsrc/docs/goose-book/src/example/overview.mdApproaches Demonstrated
Custom Client with Default Headers (Recommended)
set_client_builder()Helper Function Approach
Manual Per-Request Authentication
Configuration Support
BASIC_AUTH_USERNAME/BASIC_AUTH_PASSWORD(separate variables)BASIC_AUTH="username:password"(combined format)Testing
Closes #608