✅ Integration tests: Sinatra, Roda, Hanami, Rails#167
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive integration tests for omniauth-identity across four popular Ruby web frameworks: Sinatra with Sequel, Roda with ROM, Hanami with ROM, and Rails with ActiveRecord via Combustion. These tests verify that omniauth-identity works correctly within actual web application contexts, covering authentication flows, registration, and ORM-specific features.
Key changes:
- Integration test specs for Sinatra, Roda, Hanami, and Rails frameworks
- Dummy applications and identity models for each framework under
spec/dummies/andspec/integration/ - Enhanced ROM adapter with additional methods (
id,persisted?, improved name handling) - Updated gemspec and gemfiles to include integration testing dependencies
Reviewed changes
Copilot reviewed 135 out of 175 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
spec/integration/hanami_spec.rb |
Integration tests for Hanami framework with ROM, including routing and ROM-specific features |
spec/integration/README.md |
Documentation for integration test structure, frameworks tested, and running instructions |
spec/dummies/sinatra_app/models/identity.rb |
Sequel-based identity model for Sinatra integration tests |
spec/dummies/sinatra_app/db/migrate/001_create_identities.rb |
Database migration for Sinatra identity table |
spec/dummies/sinatra_app/config.ru |
Rack configuration for Sinatra dummy app |
spec/dummies/sinatra_app/app.rb |
Sinatra application with OmniAuth integration |
spec/dummies/roda_app/models/roda_identity.rb |
ROM-based identity model for Roda with custom validation and persistence |
spec/dummies/roda_app/app.rb |
Roda application with OmniAuth and ROM integration |
spec/dummies/hanami_app/models/hanami_identity.rb |
ROM-based identity model for Hanami with validation logic |
spec/dummies/hanami_app/app.rb |
Rack-based Hanami-style app with OmniAuth integration |
omniauth-identity.gemspec |
Updated development dependencies and file inclusion patterns |
lib/omniauth/identity/version.rb |
Added traditional VERSION constant location |
lib/omniauth/identity/models/rom.rb |
Enhanced with id, persisted? methods and improved name handling |
lib/omniauth/identity/model.rb |
Documentation formatting improvements |
| Various gemfiles | Added integration testing dependencies and Rack version specifications |
| Various docs/ files | Removed generated documentation files |
Files not reviewed (9)
- .idea/.gitignore: Language not supported
- .idea/active-tab-highlighter.xml: Language not supported
- .idea/dbnavigator.xml: Language not supported
- .idea/developer-tools.xml: Language not supported
- .idea/git_toolbox_blame.xml: Language not supported
- .idea/git_toolbox_prj.xml: Language not supported
- .idea/inspectionProfiles/Project_Default.xml: Language not supported
- .idea/misc.xml: Language not supported
- .idea/workspace.xml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return new_unpersisted(attributes.merge(password_digest: nil)) | ||
| end | ||
|
|
||
| unless attributes[:email] && !attributes[:email].to_s.strip.empty? | ||
| return new_unpersisted(attributes.merge(password_digest: nil)) | ||
| end | ||
|
|
||
| unless attributes[:password] && !attributes[:password].to_s.strip.empty? | ||
| return new_unpersisted(attributes.merge(password_digest: nil)) |
There was a problem hiding this comment.
[nitpick] The validation logic contains duplicated patterns for checking required fields. Consider extracting this into a helper method like validate_required_field(attributes, field_name) to reduce duplication and improve maintainability.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| class SessionsController < ActionController::Base | ||
| # OmniAuth callback - successful authentication | ||
| def create | ||
| auth = request.env["omniauth.auth"] | ||
| render json: {provider: auth["provider"], uid: auth["uid"]}, status: :ok | ||
| end | ||
|
|
||
| # OmniAuth failure | ||
| def failure | ||
| render json: {error: params[:message] || "failure"}, status: :unauthorized | ||
| end | ||
| end |
Check failure
Code scanning / CodeQL
CSRF protection not enabled High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
To fix the problem, explicitly enable CSRF protection for the SessionsController class by calling protect_from_forgery with: :exception. This method should be added inside the class body, typically after the class definition but before the action methods. This change ensures Rails will check for a valid CSRF token on non-GET requests and raise an exception if the token is missing or invalid, which is the recommended secure approach. Edit spec/internal/app/controllers/sessions_controller.rb by inserting the line protect_from_forgery with: :exception directly under the class definition. No additional imports are necessary.
| @@ -1,6 +1,7 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| class SessionsController < ActionController::Base | ||
| protect_from_forgery with: :exception | ||
| # OmniAuth callback - successful authentication | ||
| def create | ||
| auth = request.env["omniauth.auth"] |
Minimum allowed line rate is |
Pull Request Test Coverage Report for Build 19610116917Details
💛 - Coveralls |
Implements #26