Add Authentication API #616
Conversation
WalkthroughThis pull request introduces a user authentication system utilizing the Devise and JWT gems. It includes the addition of new gem dependencies, the creation of an authentication controller for user login and signup, configuration of Devise, establishment of a user model with associated database migrations, and routing setup for user authentication endpoints. The changes facilitate user registration, login, and session management through JWT token generation. Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Hi 👋 To deploy a review app, please comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
app/models/user.rb (1)
1-1: Add frozen string literal comment for consistency.
Rubocop flags missing# frozen_string_literal: true. Aligning with this convention will help reduce string object allocations and maintain consistency.+# frozen_string_literal: true class User < ApplicationRecord🧰 Tools
🪛 rubocop (1.69.1)
[convention] 1-1: Missing frozen string literal comment.
(Style/FrozenStringLiteralComment)
🪛 GitHub Actions: Lint CI
[warning] 1-10: Missing frozen string literal comment and multiple trailing whitespace issues.
app/controllers/api/authentication_controller.rb (1)
1-1: Refine class definition style and file-level comment.
Rubocop suggests a nested class definition instead of the compact form and adding# frozen_string_literal: true. These are style conventions, but adopting them can improve code consistency.+# frozen_string_literal: true -module Api - class AuthenticationController < ApplicationController - ... - end +class Api::AuthenticationController < ApplicationController ... end🧰 Tools
🪛 rubocop (1.69.1)
[convention] 1-1: Missing frozen string literal comment.
(Style/FrozenStringLiteralComment)
[convention] 1-1: Use nested module/class definitions instead of compact style.
(Style/ClassAndModuleChildren)
🪛 GitHub Actions: Lint CI
[warning] 1-1: Missing frozen string literal comment and incorrect module/class definition style.
config/routes.rb (1)
12-14: Adopt consistent string quoting style.
Rubocop suggests using double quotes for strings like"login". This is a minor style preference, but consistency can make your codebase more uniform and reduce lint warnings.namespace :api do - post 'login', to: 'authentication#create' + post "login", to: "authentication#create" end🧰 Tools
🪛 rubocop (1.69.1)
[convention] 13-13: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
(Style/StringLiterals)
[convention] 13-13: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
(Style/StringLiterals)
db/migrate/20250124081805_devise_create_users.rb (2)
35-36: Remove the extra blank line.A blank line is detected at lines 35-36. Consider removing it to comply with RuboCop's layout convention and maintain a cleaner migration file.
35- 36-🧰 Tools
🪛 rubocop (1.69.1)
[convention] 35-36: Extra blank line detected.
(Layout/EmptyLines)
18-23: Consider removing or uncommenting the unused Devise features.The commented-out sections for Trackable, Confirmable, and Lockable can create confusion if they aren’t intended for future use. Removing them or explicitly enabling them helps keep migrations concise.
Also applies to: 24-33
config/initializers/devise.rb (1)
129-129: Remove or wrap pepper value to avoid unnecessary exposure and fix line length.The commented pepper value is overly long and may create a false impression of a real secret. If unused, remove it; otherwise, store it securely in an environment variable.
- # config.pepper = '8f277f0b4e8090a67c44746c589cd5edfaab345bf9a650db74a63b38cc431b1710fcf704021b401c88db3bcd98e29a71b4eecf62bec170b138442818a3d33013' + # config.pepper = ENV.fetch("DEVISE_PEPPER", "placeholder_value")🧰 Tools
🪛 rubocop (1.69.1)
[convention] 129-129: Line is too long. [150/120]
(Layout/LineLength)
🪛 GitHub Actions: Lint CI
[error] 129-129: Line is too long. [150/120]
Gemfile (1)
61-61: Use double-quoted strings to comply with style guidelines.Ruby style guidelines discourage single quotes unless needed for escaping. Use double quotes here.
- gem 'jwt' + gem "jwt"🧰 Tools
🪛 rubocop (1.69.1)
[convention] 61-61: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
(Style/StringLiterals)
🪛 GitHub Actions: Lint CI
[warning] 61-61: Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Gemfile(1 hunks)app/controllers/api/authentication_controller.rb(1 hunks)app/controllers/application_controller.rb(1 hunks)app/models/user.rb(1 hunks)config/initializers/devise.rb(1 hunks)config/locales/devise.en.yml(1 hunks)config/routes.rb(1 hunks)db/migrate/20250124081805_devise_create_users.rb(1 hunks)db/schema.rb(2 hunks)test/factories/users.rb(1 hunks)test/models/user_test.rb(1 hunks)
🧰 Additional context used
🪛 rubocop (1.69.1)
app/models/user.rb
[convention] 1-1: Missing frozen string literal comment.
(Style/FrozenStringLiteralComment)
[convention] 7-7: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 8-8: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 9-9: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 10-10: Trailing whitespace detected.
(Layout/TrailingWhitespace)
Gemfile
[convention] 61-61: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
(Style/StringLiterals)
config/routes.rb
[convention] 13-13: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
(Style/StringLiterals)
[convention] 13-13: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
(Style/StringLiterals)
db/migrate/20250124081805_devise_create_users.rb
[convention] 35-36: Extra blank line detected.
(Layout/EmptyLines)
app/controllers/api/authentication_controller.rb
[convention] 1-1: Missing frozen string literal comment.
(Style/FrozenStringLiteralComment)
[convention] 1-1: Use nested module/class definitions instead of compact style.
(Style/ClassAndModuleChildren)
[convention] 4-4: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 8-8: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 9-9: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 10-10: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
(Style/StringLiterals)
[convention] 10-10: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 11-11: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 14-14: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
(Style/StringLiterals)
db/schema.rb
[convention] 13-66: Block has too many lines. [45/25]
(Metrics/BlockLength)
[convention] 13-13: Use underscores(_) as thousands separator and separate every 3 digits with them.
(Style/NumericLiterals)
config/initializers/devise.rb
[convention] 17-17: Line is too long. [154/120]
(Layout/LineLength)
[convention] 27-27: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
(Style/StringLiterals)
[convention] 39-39: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
(Style/StringLiterals)
[convention] 129-129: Line is too long. [150/120]
(Layout/LineLength)
test/factories/users.rb
[convention] 1-1: Missing frozen string literal comment.
(Style/FrozenStringLiteralComment)
[convention] 3-3: Trailing whitespace detected.
(Layout/TrailingWhitespace)
test/models/user_test.rb
[convention] 1-1: Missing frozen string literal comment.
(Style/FrozenStringLiteralComment)
🪛 GitHub Actions: Lint CI
app/models/user.rb
[warning] 1-10: Missing frozen string literal comment and multiple trailing whitespace issues.
Gemfile
[warning] 61-61: Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
app/controllers/api/authentication_controller.rb
[warning] 1-1: Missing frozen string literal comment and incorrect module/class definition style.
config/initializers/devise.rb
[error] 17-17: Line is too long. [154/120]
[error] 129-129: Line is too long. [150/120]
test/factories/users.rb
[error] 2-2: Lint/EmptyBlock: Empty block detected.
🪛 Gitleaks (8.21.2)
config/initializers/devise.rb
17-17: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test (22.x, 3.3.4)
🔇 Additional comments (6)
app/controllers/application_controller.rb (1)
6-6: Ensure API usage alignment with your security strategy.
Switchingprotect_from_forgeryfrom:exceptionto:null_sessionis suitable for supporting API clients. However, confirm that this relaxation of CSRF protection doesn't inadvertently allow undesired requests. Consider isolating API logic in a separate controller or ensuring that your request validation remains secure.app/models/user.rb (1)
5-13: Consider adding token revocation or expiration checks.
Currently,generate_jwtsets a token that expires in 60 days. If you plan to implement logout or token invalidation, you may need a token blacklist or an alternative mechanism to handle revocation. Also, watch out for trailing whitespace that Rubocop has flagged on lines 7–10.def generate_jwt - JWT.encode( + token = JWT.encode( { id: id, exp: 60.days.from_now.to_i }, Rails.application.credentials.secret_key_base ) + # e.g., store token in a session table or use a revocation list if needed. + token end✅ Verification successful
Review comment is accurate - token revocation mechanism is missing
The codebase only implements JWT token generation with expiration but lacks any token revocation or invalidation mechanism. This means tokens remain valid for 60 days even after logout or if compromised. Consider implementing one of these approaches:
- Add a token blacklist table to track revoked tokens
- Use a shorter expiration time with refresh tokens
- Implement a Redis-based token store for quick revocation checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for JWT-related code and token validation echo "=== JWT Token Validation ===" rg -l "JWT" --type ruby echo -e "\n=== Token Validation/Verification ===" rg "verify|validate.*token" --type ruby echo -e "\n=== Session/Blacklist Tables ===" fd "db/migrate" --type f | xargs rg "create_table.*session|create_table.*token|create_table.*blacklist" echo -e "\n=== Token Revocation Logic ===" rg "revoke|invalidate.*token" --type ruby echo -e "\n=== Authentication Middleware/Concerns ===" fd "(middleware|concern)" --type f --extension rb | xargs rg "JWT|token"Length of output: 5075
🧰 Tools
🪛 rubocop (1.69.1)
[convention] 7-7: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 8-8: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 9-9: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 10-10: Trailing whitespace detected.
(Layout/TrailingWhitespace)
app/controllers/api/authentication_controller.rb (1)
2-15: 🛠️ Refactor suggestionValidate inputs and provide consistent error messages.
Currently, there's a single check foruser&.valid_password?(params[:password]). Consider verifying that bothparams[:email]andparams[:password]are present and well-formed, and ensure consistent error responses for missing/invalid fields. Also watch out for trailing whitespace.def create + unless params[:email].present? && params[:password].present? + return render json: { error: "Email or password missing" }, status: :bad_request + end user = User.find_by(email: params[:email]) if user&.valid_password?(params[:password]) token = user.generate_jwt render json: { message: "Login successful", token: token }, status: :ok else render json: { error: "Invalid credentials" }, status: :unauthorized end end✅ Verification successful
Input validation suggestion is appropriate and should be implemented
The suggested changes follow Rails best practices and improve security by validating parameters before processing. The error response format is consistent with the existing implementation, and the HTTP status codes are appropriate for the different error scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for authentication-related controllers and API base controllers echo "=== Authentication Controllers ===" fd "authentication|auth" --type f --extension rb app/controllers echo -e "\n=== API Controllers ===" fd "api" --type f --extension rb app/controllers echo -e "\n=== Base Controller Content ===" fd "application_controller|base_controller|api_controller" --type f --extension rb app/controllers --exec cat {} echo -e "\n=== Input Validation Patterns ===" rg "params\[:.+\]\.present\?" --type ruby app/controllersLength of output: 967
🧰 Tools
🪛 rubocop (1.69.1)
[convention] 4-4: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 8-8: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 9-9: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 10-10: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
(Style/StringLiterals)
[convention] 10-10: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 11-11: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 14-14: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
(Style/StringLiterals)
config/routes.rb (1)
4-4: Add Devise route constraints if needed.
devise_for :usersis straightforward, but ensure it's scoped according to your application design (e.g., if you need separate API or admin routes).db/schema.rb (1)
52-62: Schema updates look correct.All changes match the Devise migration, and the indexes for email and reset_password_token are properly set as unique. No further changes needed.
config/locales/devise.en.yml (1)
1-65: LGTM! Comprehensive Devise translations setup.The translations file includes all necessary keys and follows Devise's recommended structure. The messages are clear, user-friendly, and maintain security by not revealing sensitive information in error messages.
test/factories/users.rb
Outdated
| FactoryBot.define do | ||
| factory :user do | ||
|
|
||
| end | ||
| end |
There was a problem hiding this comment.
Add required attributes to the user factory.
The empty factory will cause test failures. Since this PR implements Devise authentication, the factory should include essential attributes.
Apply this diff to add the necessary attributes:
+# frozen_string_literal: true
+
FactoryBot.define do
factory :user do
-
+ sequence(:email) { |n| "user#{n}@example.com" }
+ password { 'password123' }
+ password_confirmation { 'password123' }
+ confirmed_at { Time.current } # If using confirmable module
end
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| FactoryBot.define do | |
| factory :user do | |
| end | |
| end | |
| # frozen_string_literal: true | |
| FactoryBot.define do | |
| factory :user do | |
| sequence(:email) { |n| "user#{n}@example.com" } | |
| password { 'password123' } | |
| password_confirmation { 'password123' } | |
| confirmed_at { Time.current } # If using confirmable module | |
| end | |
| end |
🧰 Tools
🪛 rubocop (1.69.1)
[convention] 1-1: Missing frozen string literal comment.
(Style/FrozenStringLiteralComment)
[convention] 3-3: Trailing whitespace detected.
(Layout/TrailingWhitespace)
🪛 GitHub Actions: Lint CI
[error] 2-2: Lint/EmptyBlock: Empty block detected.
| require "test_helper" | ||
|
|
||
| class UserTest < ActiveSupport::TestCase | ||
| # test "the truth" do | ||
| # assert true | ||
| # end | ||
| end |
There was a problem hiding this comment.
Implement essential User model tests.
The test file only contains a placeholder. Since this PR implements authentication, we need comprehensive test coverage for the User model.
Apply this diff to add essential test cases:
+# frozen_string_literal: true
+
require "test_helper"
class UserTest < ActiveSupport::TestCase
- # test "the truth" do
- # assert true
- # end
+ test "should not save user without email" do
+ user = build(:user, email: nil)
+ assert_not user.save, "Saved user without email"
+ end
+
+ test "should not save user with invalid email format" do
+ user = build(:user, email: "invalid_email")
+ assert_not user.save, "Saved user with invalid email"
+ end
+
+ test "should not save user with duplicate email" do
+ create(:user, email: "[email protected]")
+ user = build(:user, email: "[email protected]")
+ assert_not user.save, "Saved user with duplicate email"
+ end
+
+ test "should not save user with password shorter than minimum length" do
+ user = build(:user, password: "short", password_confirmation: "short")
+ assert_not user.save, "Saved user with short password"
+ end
+
+ test "should save valid user" do
+ user = build(:user)
+ assert user.save, "Could not save valid user"
+ end
endCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 rubocop (1.69.1)
[convention] 1-1: Missing frozen string literal comment.
(Style/FrozenStringLiteralComment)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
config/routes.rb (2)
12-15: Consider adding API versioning.The API routes should be versioned to ensure backward compatibility and easier future updates. Consider nesting them under a version namespace.
namespace :api do + namespace :v1 do post 'signup', to: 'authentication#signup' post 'login', to: 'authentication#create' + end end
12-15: Add rate limiting for authentication endpoints.Authentication endpoints should be rate-limited to prevent brute force attacks and DoS attempts.
Consider using the
rack-attackgem to implement rate limiting:# config/initializers/rack_attack.rb class Rack::Attack throttle('auth/ip', limit: 5, period: 60.seconds) do |req| req.ip if req.path.start_with?('/api/') && ['login', 'signup'].any? { |path| req.path.include?(path) } end endapp/controllers/api/authentication_controller.rb (1)
36-38: Enhance parameter sanitization.Consider adding additional permitted parameters that might be needed for user management.
def user_params - params.require(:user).permit(:email, :password, :password_confirmation) + params.require(:user).permit( + :email, + :password, + :password_confirmation, + :first_name, + :last_name + ).transform_values(&:strip) end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/controllers/api/authentication_controller.rb(1 hunks)config/routes.rb(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Lint CI
app/controllers/api/authentication_controller.rb
[error] 1-1: Missing frozen string literal comment and incorrect module/class definition style
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test (22.x, 3.3.4)
🔇 Additional comments (1)
app/controllers/api/authentication_controller.rb (1)
1-39: Verify CSRF protection configuration.Ensure CSRF protection is properly configured for API endpoints.
✅ Verification successful
CSRF protection is properly configured ✅
The application uses
protect_from_forgery with: :null_sessionwhich is the recommended configuration for API endpoints. This setup ensures proper CSRF protection while allowing API authentication to work correctly.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if CSRF protection is properly configured rg -l "protect_from_forgery|null_session" app/controllers/Length of output: 102
Script:
#!/bin/bash # Check the content of ApplicationController cat app/controllers/application_controller.rbLength of output: 369
🧰 Tools
🪛 GitHub Actions: Lint CI
[error] 1-1: Missing frozen string literal comment and incorrect module/class definition style
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/controllers/api/authentication_controller.rb (1)
5-21: Enhance security with rate limiting and logging.The login endpoint needs additional security measures:
- Implement rate limiting for failed login attempts
- Add logging for authentication failures
- Add request throttling to prevent brute force attacks
Consider using the
rack-attackgem for request throttling:# config/initializers/rack_attack.rb class Rack::Attack throttle('login/ip', limit: 5, period: 60.seconds) do |req| req.ip if req.path == '/api/login' && req.post? end endAnd add logging for authentication failures:
else + Rails.logger.warn("Failed login attempt for email: #{params[:email]} from IP: #{request.remote_ip}") render json: { error: 'Invalid credentials' }, status: :unauthorized end🧰 Tools
🪛 GitHub Actions: Lint CI
[warning] 15-15: Useless assignment to variable 'e' in JWT::EncodeError rescue block.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/controllers/api/authentication_controller.rb(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Lint CI
app/controllers/api/authentication_controller.rb
[warning] 15-15: Useless assignment to variable 'e' in JWT::EncodeError rescue block.
[warning] 39-39: Useless assignment to variable 'e' in JWT::EncodeError rescue block.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test (22.x, 3.3.4)
🔇 Additional comments (3)
app/controllers/api/authentication_controller.rb (3)
1-4: LGTM! Proper module structure implemented.The code follows Ruby best practices with proper module nesting and includes the frozen string literal comment.
51-53: LGTM! Proper strong parameters implementation.The code follows Rails best practices for parameter sanitization.
27-28: 🛠️ Refactor suggestionStrengthen password requirements.
The current password length requirement of 6 characters is too weak. Consider:
- Increasing minimum length to 8 characters
- Adding complexity requirements (uppercase, lowercase, numbers, special chars)
- return render json: { error: 'Password must be at least 6 characters' }, status: :unprocessable_entity if - params.dig(:user, :password)&.length.to_i < 6 + password = params.dig(:user, :password) + return render json: { + error: 'Password must be at least 8 characters and include uppercase, lowercase, number and special character' + }, status: :unprocessable_entity unless password_meets_requirements?(password) # Add this private method: + def password_meets_requirements?(password) + return false unless password&.length.to_i >= 8 + return false unless password =~ /[A-Z]/ # uppercase + return false unless password =~ /[a-z]/ # lowercase + return false unless password =~ /[0-9]/ # number + return false unless password =~ /[^A-Za-z0-9]/ # special char + true + endLikely invalid or redundant comment.
|
API Test Commands: Sign up: Login: |
|
/deploy-review-app |
|
🏗️ Building Docker image... |
This PR introduces authentication to the application using the Devise gem and JWT for token-based authentication. The changes include adding necessary gems, setting up Devise, creating authentication endpoints, and modifying controllers and routes.
Authentication Setup
deviseandjwtgems to theGemfile.Api::AuthenticationControllerto handle user login and JWT generation.ApplicationControllerto use:null_sessionfor CSRF protection when the request header contains a specific token.config/initializers/devise.rb.config/locales/devise.en.yml.User Model and Migration
Usermodel to include Devise modules and a method to generate JWT tokens.userstable.db/schema.rbto reflect the new migration.Routes
routes.rbto include Devise routes and an API endpoint for login.This change is
Summary by CodeRabbit
Release Notes
New Features
Security Improvements
Infrastructure
Localization