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
✨ Finishing Touches
🪧 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... |
Judahmeek
force-pushed
the
cmdumar-auth-api
branch
from
8c8b1a6 to
c7c42f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
config/initializers/devise.rb (2)
17-19:⚠️ Potential issueAvoid storing potential secret keys in source control
Even though it's commented out, having potential secret keys in source code is a security risk. Store the key in an environment variable instead.
- # config.secret_key = - # 'f5c3627ab84eb5a12eab9feae096d10ac001e2a3980e87567922107429a516f39 - # 39c17512beb6c2b57330814611d42511d91d603a058391729519332b0a3e826' + # config.secret_key = ENV.fetch("DEVISE_SECRET_KEY", nil)
131-133:⚠️ Potential issueAvoid storing potential secret keys in source control
The pepper value, like the secret key, should not be stored in source code.
- # config.pepper = - # '8f277f0b4e8090a67c44746c589cd5edfaab345bf9a650db74a63b38cc431b1710fcf704021b - # 401c88db3bcd98e29a71b4eecf62bec170b138442818a3d33013' + # config.pepper = ENV.fetch("DEVISE_PEPPER", nil)
🧹 Nitpick comments (5)
db/migrate/20250124081805_devise_create_users.rb (1)
35-36: Remove extra blank lineThere's an unnecessary blank line that should be removed to maintain consistent formatting.
# t.datetime :locked_at - t.timestamps null: false🧰 Tools
🪛 RuboCop (1.73)
[convention] 35-36: Extra blank line detected.
(Layout/EmptyLines)
config/initializers/devise.rb (1)
11-316: Consider enabling more Devise modules as neededThe current configuration uses only basic Devise functionality. Consider enabling additional modules like
:trackablefor login analytics,:confirmablefor email verification, or:lockablefor security enhancement if your application requires these features.app/controllers/api/authentication_controller.rb (3)
27-28: Enhance password security policy.The password policy only checks for a minimum length of 6 characters, which is on the lower end of modern security recommendations. Consider strengthening your password requirements.
- 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 with at least one number and one special character" }, + status: :unprocessable_entity if + password.nil? || password.length < 8 || !password.match(/\d/) || !password.match(/[!@#$%^&*(),.?":{}|<>]/)
23-26: Fix line continuation in email validation.The current code has an awkward line continuation that makes it less readable. Consider fixing the formatting.
- return render json: { error: "Invalid email format" }, status: :unprocessable_entity unless - params.dig(:user, :email)&.match?(URI::MailTo::EMAIL_REGEXP) + email = params.dig(:user, :email) + return render json: { error: "Invalid email format" }, status: :unprocessable_entity unless email&.match?(URI::MailTo::EMAIL_REGEXP)
8-17: Consider adding secure token refresh mechanism.The current implementation doesn't have a token refresh mechanism, which means users will need to log in again when their token expires.
Best practice is to implement a token refresh endpoint that issues new access tokens using a longer-lived refresh token. This improves security by allowing shorter access token lifetimes while maintaining a good user experience.
Example refresh token implementation:
def refresh begin refresh_token = request.headers['Authorization']&.split(' ')&.last payload = User.decode_jwt(refresh_token) user = User.find(payload['sub']) new_token = user.generate_jwt render json: { token: new_token }, status: :ok rescue JWT::DecodeError, ActiveRecord::RecordNotFound render json: { error: "Invalid refresh token" }, status: :unauthorized end end
📜 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)
🚧 Files skipped from review as they are similar to previous changes (6)
- test/models/user_test.rb
- Gemfile
- test/factories/users.rb
- app/models/user.rb
- config/routes.rb
- app/controllers/application_controller.rb
🧰 Additional context used
🪛 RuboCop (1.73)
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)
db/migrate/20250124081805_devise_create_users.rb
[convention] 35-36: Extra blank line detected.
(Layout/EmptyLines)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test (22.x, 3.3.4)
- GitHub Check: test (22.x, 3.3.4)
- GitHub Check: test (22.x, 3.3.4)
🔇 Additional comments (4)
config/locales/devise.en.yml (1)
1-65: LGTM: Standard Devise translations fileThis file contains the standard English translations for Devise authentication messages. It covers all necessary user interactions including confirmations, failures, password resets, registrations, sessions, and error messages.
db/migrate/20250124081805_devise_create_users.rb (1)
3-43: LGTM: Standard Devise user migrationThe migration creates a users table with the essential Devise fields. The commented sections for trackable, confirmable, and lockable modules provide flexibility for future authentication feature expansion.
🧰 Tools
🪛 RuboCop (1.73)
[convention] 35-36: Extra blank line detected.
(Layout/EmptyLines)
db/schema.rb (1)
13-62: LGTM: Schema reflects migrations correctlyThe schema has been properly updated to include the users table with all the necessary fields for Devise authentication.
🧰 Tools
🪛 RuboCop (1.73)
[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)
[convention] 24-24: Use
%wor%Wfor an array of words.(Style/WordArray)
[convention] 24-24: Line is too long. [126/120]
(Layout/LineLength)
[convention] 39-43: Add timestamps when creating a new table.
(Rails/CreateTableWithTimestamps)
[convention] 42-42: Use
%wor%Wfor an array of words.(Style/WordArray)
config/initializers/devise.rb (1)
29-29: Update the mailer sender addressThe current mailer sender address is a placeholder. Replace it with a real address for production use.
- config.mailer_sender = "[email protected]" + config.mailer_sender = ENV.fetch("MAILER_SENDER", "[email protected]")
| def create | ||
| user = User.find_by(email: params[:email].downcase.strip) | ||
|
|
||
| if user&.valid_password?(params[:password]) | ||
| begin | ||
| token = user.generate_jwt | ||
| render json: { | ||
| message: "Login successful", | ||
| token: token | ||
| }, status: :ok | ||
| rescue JWT::EncodeError | ||
| render json: { error: "Authentication failed" }, status: :internal_server_error | ||
| end | ||
| else | ||
| render json: { error: "Invalid credentials" }, status: :unauthorized | ||
| end | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add rate limiting to prevent brute force attacks.
The login endpoint currently has no protection against brute force attacks. Consider implementing rate limiting based on IP address and/or account.
You could implement this by adding a before_action that checks for too many failed attempts:
before_action :check_request_rate_limit, only: [:create]
def check_request_rate_limit
# Use Rails.cache or Redis to track attempts
key = "auth_attempts:#{request.ip}"
attempts = Rails.cache.read(key) || 0
if attempts >= 5 # Limit to 5 attempts per hour
Rails.cache.write(key, attempts + 1, expires_in: 1.hour)
render json: { error: "Too many attempts. Please try again later." },
status: :too_many_requests
else
Rails.cache.write(key, attempts + 1, expires_in: 1.hour)
end
end| # frozen_string_literal: true | ||
|
|
||
| module Api | ||
| class AuthenticationController < ApplicationController | ||
| def create | ||
| user = User.find_by(email: params[:email].downcase.strip) | ||
|
|
||
| if user&.valid_password?(params[:password]) | ||
| begin | ||
| token = user.generate_jwt | ||
| render json: { | ||
| message: "Login successful", | ||
| token: token | ||
| }, status: :ok | ||
| rescue JWT::EncodeError | ||
| render json: { error: "Authentication failed" }, status: :internal_server_error | ||
| end | ||
| else | ||
| render json: { error: "Invalid credentials" }, status: :unauthorized | ||
| end | ||
| end | ||
|
|
||
| def signup | ||
| return render json: { error: "Invalid email format" }, status: :unprocessable_entity unless | ||
| params.dig(:user, :email)&.match?(URI::MailTo::EMAIL_REGEXP) | ||
|
|
||
| return render json: { error: "Password must be at least 6 characters" }, status: :unprocessable_entity if | ||
| params.dig(:user, :password)&.length.to_i < 6 | ||
|
|
||
| user = User.new(user_params) | ||
|
|
||
| if user.save | ||
| begin | ||
| token = user.generate_jwt | ||
| render json: { | ||
| message: "Signup successful", | ||
| token: token | ||
| }, status: :created | ||
| rescue JWT::EncodeError | ||
| render json: { error: "Failed to generate authentication token" }, status: :internal_server_error | ||
| end | ||
| else | ||
| render json: { | ||
| errors: user.errors.full_messages | ||
| }, status: :unprocessable_entity | ||
| end | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def user_params | ||
| params.require(:user).permit(:email, :password, :password_confirmation) | ||
| end | ||
| end |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add token revocation functionality.
The controller handles token generation but lacks functionality for token revocation or logout. Consider adding a method to invalidate tokens.
You could add a logout method:
def logout
# If using a token blacklist or revocation strategy
current_user&.revoke_jwt
render json: { message: "Logged out successfully" }, status: :ok
endThis would require implementing a revoke_jwt method in the User model and updating your routes to include a logout endpoint.
🌐 Web query:
What are the best practices for JWT token revocation?
💡 Result:
JWTs (JSON Web Tokens) present unique challenges for revocation due to their stateless nature, but several best practices ensure secure token management:
Core Strategies for JWT Revocation
1. Short-Lived Tokens with Refresh Tokens
- Short expiration times: Limit JWT validity to minutes (e.g., 15–60 minutes) to reduce exposure if compromised[7][8].
- Revoke refresh tokens: Invalidate long-lived refresh tokens (e.g., 2 weeks) to block new JWT generation. This forces reauthentication but leaves existing JWTs active until expiration[1][8].
- Example:
# Flask-JWT-Extended revoking refresh tokens @app.route("/logout", methods=["DELETE"]) @jwt_required(refresh=True) def logout(): jti = get_jwt()["jti"] db.session.add(TokenBlocklist(jti=jti)) db.session.commit() return jsonify(msg="Refresh token revoked")[2]
2. Distributed Event Systems
- Webhook notifications: Broadcast refresh token revocations to services via events (e.g.,
jwt.refresh-token.revoke). Services cache revoked user IDs/JWT IDs (jti) and validate against this list[1][6][8]. - Implementation:
// FusionAuth webhook handler router.post('/revoke', (req, res) => { const { userId, applicationId } = req.body.event; JWTManager.revoke(userId, applicationId); // Updates cached revocation list[8] });
3. Blocklist/Denylist
- Storing revoked tokens: Maintain a database/Redis cache of invalidated tokens checked during validation. Entries auto-expire based on JWT
exp[5][6][11]. - Code example:
// Node.js blocklist check middleware const isRevoked = async (req, token) => { const jti = token.jti; const exists = await redis.get(`bl_${jti}`); return !!exists; }[9][11]
4. Refresh Token Rotation
- Rotate on use: Issue new refresh tokens after each use, invalidating old ones. Limits stolen token utility[3][10].
// Rotating refresh tokens const rotateRefreshToken = (oldToken) => { invalidateToken(oldToken); return { newAccessToken, newRefreshToken }; }[3]
Additional Best Practices
- Token versioning: Include a version claim (e.g.,
tokenVersion) in JWTs. Increment globally to invalidate all prior tokens[3]. - HTTPS enforcement: Prevent token interception by requiring encrypted connections[7][12].
- Validate critical claims: Verify
iss(issuer),aud(audience), and signature during JWT validation[7][13]. - Avoid sensitive data: JWTs are base64-encoded, not encrypted. Store minimal user data[7][12].
Tradeoffs and Recommendations
| Method | Pros | Cons |
|---|---|---|
| Short-lived JWTs | Simple, no state management | Delayed revocation (until expiry) |
| Blocklist | Immediate revocation | Stateful, scalability challenges |
| Refresh token revocation | Balances security and usability | Requires frontend coordination |
| Event systems | Real-time, scalable | Complex infrastructure needed |
For most applications, combining short-lived JWTs with refresh token rotation and event-driven revocation provides robust security. Services like Auth0 or FusionAuth simplify implementation with built-in token management and webhooks[8][12].
Citations:
- 1: https://devops.com/how-to-revoke-json-web-tokens-jwts/
- 2: https://flask-jwt-extended.readthedocs.io/en/stable/blocklist_and_token_revoking.html
- 3: https://www.th4ts3cur1ty.company/jwt-security-part-4-token-revocation-and-refresh-tokens/
- 4: https://community.auth0.com/t/best-practice-for-checking-if-token-is-revoked-in-api/17460
- 5: https://www.cerberauth.com/blog/how-to-invalidate-jwt-access-token/
- 6: https://dzone.com/articles/jwt-token-revocation
- 7: https://www.softwaresecured.com/post/how-to-properly-secure-your-jwts
- 8: https://fusionauth.io/articles/tokens/revoking-jwts
- 9: https://help.salesforce.com/s/articleView?id=sf.connected_app_revoke_jwt.htm&language=en_US&type=5
- 10: https://supertokens.com/blog/revoking-access-with-a-jwt-blacklist
- 11: https://elixirforum.com/t/what-is-a-good-strategy-to-persistently-revoke-or-blacklist-bad-user-login-tokens/66848
- 12: https://auth0.com/docs/secure/tokens/token-best-practices
- 13: https://curity.io/resources/learn/jwt-best-practices/
Implement Token Revocation (Logout) Functionality
The AuthenticationController currently handles token generation for login and signup but lacks any means to invalidate tokens on logout. Given JWT’s stateless nature, simply “destroying” a token on the client is insufficient for immediate revocation. Instead, consider the following best practices when implementing a logout endpoint:
-
Blocklist/Denylist Strategy:
Add a logout method that marks the token as revoked (e.g., by saving its unique identifier in a blocklist). During authentication on subsequent requests, check against this list. -
Short-Lived Tokens + Refresh Token Rotation:
Use short-lived access tokens to limit exposure and pair them with refresh tokens. On logout, revoke the refresh token to prevent issuance of new access tokens.
For instance, you could add a logout method as follows:
def logout
# Revoke the current JWT using a blocklist or similar strategy.
# Ensure that the 'revoke_jwt' method in the User model properly invalidates the token,
# such as by storing its JTI in a token blocklist.
current_user&.revoke_jwt if current_user
render json: { message: "Logged out successfully" }, status: :ok
endAction Items:
- Implement and test a
revoke_jwtmethod in the User model to support token invalidation. - Update your routes to expose this new logout endpoint.
- Review your token strategy to consider using short-lived access tokens combined with refresh token rotation for improved security.
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