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 with JWT token generation, configuration of Devise, establishment of a User model integrated with Devise and JWT, and routing setup for user authentication endpoints. The changes also update CSRF protection strategy and add database migrations for the users table. Basic test and factory files for the User model are added as placeholders. Changes
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: "test@example.com")
+ user = build(:user, email: "test@example.com")
+ 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 = "please-change-me-at-config-initializers-devise@example.com" + config.mailer_sender = ENV.fetch("MAILER_SENDER", "support@yourapp.com")
| 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.
Judahmeek
force-pushed
the
cmdumar-auth-api
branch
from
c7c42f8 to
73b4980
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
app/controllers/api/authentication_controller.rb (3)
5-21: 🛠️ Refactor suggestionAdd rate limiting to prevent brute force attacks.
The login endpoint has no protection against brute force attacks. Implement rate limiting based on IP address and/or account to enhance security.
Adding a
before_actionto check for repeated failed attempts would significantly improve security: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
1-55: 🛠️ Refactor suggestionAdd token revocation (logout) functionality.
The controller handles token generation but doesn't provide a way to invalidate tokens when users log out, which is a security concern.
Add a logout method to revoke tokens:
def logout # Revoke the current JWT using a blocklist or similar strategy current_user&.revoke_jwt if current_user render json: { message: "Logged out successfully" }, status: :ok endThis will require implementing a
revoke_jwtmethod in the User model and updating your routes.
27-28: 🛠️ Refactor suggestionStrengthen password requirements.
A 6-character minimum password is on the low side by modern security standards. NIST and security best practices recommend a minimum of 8 characters.
- return render json: { error: "Password must be at least 6 characters" }, status: :unprocessable_entity if - params.dig(:user, :password)&.length.to_i < 6 + return render json: { error: "Password must be at least 8 characters" }, status: :unprocessable_entity if + params.dig(:user, :password)&.length.to_i < 8
🧹 Nitpick comments (3)
db/schema.rb (1)
13-13: Consider using underscores for the schema version number.The schema version is using a timestamp format without underscores for readability. While this works, following Ruby conventions for numeric literals would improve readability.
-ActiveRecord::Schema[8.0].define(version: 2025_01_24_081805) do +ActiveRecord::Schema[8.0].define(version: 2025_01_24_081_805) do🧰 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)
db/migrate/20250124081805_devise_create_users.rb (1)
35-35: Remove the extra blank line.There's an unnecessary blank line that should be removed to follow Ruby style conventions.
# t.datetime :locked_at - t.timestamps null: false🧰 Tools
🪛 RuboCop (1.73)
[convention] 35-36: Extra blank line detected.
(Layout/EmptyLines)
app/controllers/api/authentication_controller.rb (1)
6-6: Use Devise's secure_compare for enhanced security.When finding a user by email, consider using
Devise.secure_compareto prevent timing attacks on the email lookup process.- user = User.find_by(email: params[:email].downcase.strip) + # First find by normalized email + email = params[:email].to_s.downcase.strip + user = nil + User.find_each do |u| + if Devise.secure_compare(u.email, email) + user = u + break + 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 due to trivial changes (2)
- Gemfile
- config/locales/devise.en.yml
🚧 Files skipped from review as they are similar to previous changes (6)
- test/factories/users.rb
- app/models/user.rb
- app/controllers/application_controller.rb
- test/models/user_test.rb
- config/routes.rb
- config/initializers/devise.rb
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/controllers/api/authentication_controller.rb (1)
app/models/user.rb (1)
generate_jwt(6-14)
🪛 RuboCop (1.73)
db/migrate/20250124081805_devise_create_users.rb
[convention] 35-36: Extra blank line detected.
(Layout/EmptyLines)
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)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test (22.x, 3.3.4)
🔇 Additional comments (3)
db/schema.rb (1)
52-62: LGTM: User table structure follows Devise best practices.The users table has been properly set up with all necessary fields for authentication, password resets, and session management. The unique indexes on email and reset_password_token are correctly implemented for data integrity and query performance.
db/migrate/20250124081805_devise_create_users.rb (1)
3-44: Migration date is set in the future.The migration timestamp (20250124081805) appears to be from January 2025, which is in the future. While this will work, it's unusual and might cause confusion during development. Typically, migrations should use the current date.
Was the future date intentional, or should it be updated to the current date?
🧰 Tools
🪛 RuboCop (1.73)
[convention] 35-36: Extra blank line detected.
(Layout/EmptyLines)
app/controllers/api/authentication_controller.rb (1)
4-54:✅ Verification successful
Add CSRF protection for the API endpoints.
The controller doesn't implement CSRF protection for API endpoints. While
protect_from_forgery with: :null_sessionmay be set in ApplicationController, consider adding token-based CSRF protection for these sensitive endpoints.
🏁 Script executed:
#!/bin/bash # Check if ApplicationController has CSRF protection echo "Checking CSRF protection in ApplicationController:" cat app/controllers/application_controller.rb | grep -A 5 "protect_from_forgery"Length of output: 322
CSRF protection already applied
TheApplicationControllerincludes:protect_from_forgery with: :null_session, if: proc { request.headers["X-Auth"] != "tutorial_secret" }Since these are stateless, JWT-authenticated endpoints that accept tokens via headers (not cookies), cookie-based CSRF isn’t a concern. No additional CSRF token setup is required.
| ## Trackable | ||
| # t.integer :sign_in_count, default: 0, null: false | ||
| # t.datetime :current_sign_in_at | ||
| # t.datetime :last_sign_in_at | ||
| # t.string :current_sign_in_ip | ||
| # t.string :last_sign_in_ip | ||
|
|
||
| ## Confirmable | ||
| # t.string :confirmation_token | ||
| # t.datetime :confirmed_at | ||
| # t.datetime :confirmation_sent_at | ||
| # t.string :unconfirmed_email # Only if using reconfirmable | ||
|
|
||
| ## Lockable | ||
| # t.integer :failed_attempts, default: 0, null: false # Only if lock strategy is :failed_attempts | ||
| # t.string :unlock_token # Only if unlock strategy is :email or :both | ||
| # t.datetime :locked_at |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider enabling additional Devise modules for enhanced security.
The migration includes commented-out fields for Trackable, Confirmable, and Lockable modules. Consider enabling some of these for better security:
- Trackable: To monitor suspicious login activity
- Confirmable: To verify email addresses
- Lockable: To prevent brute force attacks
🌐 Web query:
What are the security benefits of Devise's Trackable, Confirmable, and Lockable modules?
💡 Result:
Devise, a popular authentication solution for Ruby on Rails, offers several modules that enhance security for web applications. Let's explore the security benefits of three specific modules: Trackable, Confirmable, and Lockable.
Trackable Module
The Trackable module focuses on monitoring user authentication activities by collecting and storing valuable login data:
- It records the total sign-in count, increasing this value with each successful authentication (whether by form, OpenID, or OAuth)[3]
- It captures timestamp information, including both the current sign-in time and the previous sign-in time[3]
- It logs IP addresses, storing both the current sign-in IP and the previous sign-in IP[3]
These tracking capabilities provide significant security benefits:
- Enables detection of suspicious login patterns that might indicate unauthorized access
- Helps monitor user activity to identify potential security breaches[4]
- Provides valuable forensic data for investigating security incidents
- Allows administrators to identify unusual behavior such as logins from unexpected locations
Confirmable Module
The Confirmable module verifies email ownership through a confirmation process:
- It generates a unique confirmation token and sends an email with a verification link when users register[7]
- Accounts remain inactive until users click the confirmation link, proving they own the email address[7]
- It can handle reconfirmation when users change their email addresses[2]
From a security perspective, this module:
- Prevents the creation of spam accounts by requiring email verification[7]
- Ensures that only users with legitimate email access can create accounts
- Reduces the risk of account takeovers through email verification
- Adds an additional layer of identity verification during the registration process
Lockable Module
The Lockable module helps prevent brute-force attacks by implementing account locking mechanisms:
- It locks user accounts after a specified number of failed login attempts[4][10]
- Offers multiple unlock strategies: email-based unlocking, time-based automatic unlocking, or manual admin unlocking[10]
- Tracks failed authentication attempts to determine when to lock an account
The security advantages of this module include:
- Direct mitigation against brute-force password attacks[8]
- Significantly increases the difficulty of credential-stuffing attacks
- Alerts users to potential unauthorized access attempts via the lockout notification
- Provides administrators with visibility into potential attack patterns when multiple accounts experience lockouts[10]
By implementing these three Devise modules together, applications can create a robust security framework that monitors activity, verifies user identity, and protects against common authentication attacks.
Citations:
- 1: https://blog.appsignal.com/2023/08/02/advanced-usages-of-devise-for-rails.html
- 2: https://blog.mystrika.com/mastering-devise-authentication-in-ruby-on-rails/
- 3: https://github.com/heartcombo/devise/blob/main/lib/devise/models/trackable.rb
- 4: https://www.sitepoint.com/devise-authentication-in-depth/
- 5: https://codelila.com/understanding-devise-security-implementation-rails/
- 6: https://github.com/devise-security/devise-security/blob/master/README.md
- 7: https://231webdev.com/add-confirmable-to-devise/
- 8: https://github.com/brunofacca/zen-rails-security-checklist
- 9: https://dev.to/kevinluo201/introduction-to-devise-modules-and-enable-all-of-them-4p25
- 10: https://dev.to/angelamchoi/unlocking-the-potential-of-lockable-3pge
Enable additional Devise modules for improved security
In db/migrate/20250124081805_devise_create_users.rb, consider uncommenting and enabling the following modules:
- Trackable (sign_in_count, current_sign_in_at, last_sign_in_at, current_sign_in_ip, last_sign_in_ip)
• Detect suspicious login patterns and gather forensic data - Confirmable (confirmation_token, confirmed_at, confirmation_sent_at, unconfirmed_email)
• Enforce email verification to prevent spam or unauthorized accounts - Lockable (failed_attempts, unlock_token, locked_at)
• Mitigate brute-force attacks and notify users of potential credential-stuffing
Example migration update:
## Trackable
- # t.integer :sign_in_count, default: 0, null: false
- # t.datetime :current_sign_in_at
- # t.datetime :last_sign_in_at
- # t.string :current_sign_in_ip
- # t.string :last_sign_in_ip
+ t.integer :sign_in_count, default: 0, null: false
+ t.datetime :current_sign_in_at
+ t.datetime :last_sign_in_at
+ t.string :current_sign_in_ip
+ t.string :last_sign_in_ip
## Confirmable
- # t.string :confirmation_token
- # t.datetime :confirmed_at
- # t.datetime :confirmation_sent_at
- # t.string :unconfirmed_email
+ t.string :confirmation_token
+ t.datetime :confirmed_at
+ t.datetime :confirmation_sent_at
+ t.string :unconfirmed_email
## Lockable
- # t.integer :failed_attempts, default: 0, null: false
- # t.string :unlock_token
- # t.datetime :locked_at
+ t.integer :failed_attempts, default: 0, null: false
+ t.string :unlock_token
+ t.datetime :locked_atThese modules together strengthen your authentication layer by monitoring login activity, verifying user identities, and preventing unauthorized access.
📝 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.
| ## Trackable | |
| # t.integer :sign_in_count, default: 0, null: false | |
| # t.datetime :current_sign_in_at | |
| # t.datetime :last_sign_in_at | |
| # t.string :current_sign_in_ip | |
| # t.string :last_sign_in_ip | |
| ## Confirmable | |
| # t.string :confirmation_token | |
| # t.datetime :confirmed_at | |
| # t.datetime :confirmation_sent_at | |
| # t.string :unconfirmed_email # Only if using reconfirmable | |
| ## Lockable | |
| # t.integer :failed_attempts, default: 0, null: false # Only if lock strategy is :failed_attempts | |
| # t.string :unlock_token # Only if unlock strategy is :email or :both | |
| # t.datetime :locked_at | |
| ## Trackable | |
| t.integer :sign_in_count, default: 0, null: false | |
| t.datetime :current_sign_in_at | |
| t.datetime :last_sign_in_at | |
| t.string :current_sign_in_ip | |
| t.string :last_sign_in_ip | |
| ## Confirmable | |
| t.string :confirmation_token | |
| t.datetime :confirmed_at | |
| t.datetime :confirmation_sent_at | |
| t.string :unconfirmed_email | |
| ## Lockable | |
| t.integer :failed_attempts, default: 0, null: false | |
| t.string :unlock_token | |
| t.datetime :locked_at |
| # 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 | ||
| end |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Review JWT token expiration period.
The User model sets token expiration to 60 days, which is quite long by security standards. Consider implementing a shorter expiration period with a refresh token mechanism.
🏁 Script executed:
#!/bin/bash
# Check the JWT token expiration time in the User model
echo "Checking JWT token expiration in User model:"
cat app/models/user.rb | grep -A 10 "generate_jwt"Length of output: 320
🌐 Web query:
What is the recommended JWT token expiration time for security?
💡 Result:
Recommended JWT Token Expiration Time for Security
JWT (JSON Web Token) expiration time is a central part of token security. Setting it appropriately helps balance security risks against user experience.
Best Practice Guidance
-
Short Expiration for Access Tokens:
Security experts recommend that access tokens should have short lifetimes. The typical best practice is to set the expiration to a matter of minutes (e.g., 5–15 minutes) or, at most, a few hours[1][4][6][7]. This reduces the window in which a stolen or leaked token can be exploited by an attacker. -
Why Short Expiration Matters:
JWTs are usually stateless—once issued, they cannot be revoked until they expire. Thus, a shorter expiration minimizes the risk posed by compromised tokens[1][7][3]. When a JWT expires, users are required to re-authenticate or obtain a new token via a refresh workflow. -
Typical Recommendations:
- High-security APIs (banking, financial): 5–15 minutes[7][6].
- General web applications: 15 minutes to 1 hour[1][4][7][9].
- Longer lifetimes (days or weeks) are only appropriate for refresh tokens, which are stored more securely and rotated regularly[4][7].
-
Refresh Tokens:
Use short-lived access tokens alongside longer-lived refresh tokens. This way, users can continue their sessions seamlessly while keeping security tight: the access token expires quickly, but the refresh token (stored securely) allows for session continuity and periodic token renewal[4][7][6].
Examples from Industry Guidance
- Curity: Recommends token expiration "to minutes or hours at maximum" and cautions against validity for days or months[1].
- Snyk: Shows example code for 30-minute expirations and warns that long-lived tokens increase risk[3].
- MojoAuth: Suggests 5–15 minutes for high-security cases, 1–24 hours for typical web apps, and 7–30 days only for refresh tokens[7].
- Auth0: Emphasizes that tokens should have an explicit expiration and a strategy for renewal or revocation; otherwise, they could remain valid indefinitely[2].
Balancing Security and Usability
While shorter expiration improves security, it also means users may need to refresh tokens more frequently, which can impact user experience. The right balance depends on your application's risk profile, user expectations, and how you handle token refreshes[4][7].
Summary Table
| Token Type | Recommended Expiration | Typical Use |
|---|---|---|
| Access Token | 5–15 minutes (up to 1 hour) | API access, auth |
| Refresh Token | 7–30 days | Session continuation |
Conclusion:
Set JWT access token expiration to no more than 1 hour—preferably 15–30 minutes for most web applications, and even shorter for sensitive environments. Pair this with secure, longer-lived refresh tokens to maintain both security and usability[1][3][4][6][7].
Never issue access tokens valid for days or months. Always set explicit expiration times and have a strategy for secure token refresh and storage.
Citations:
- 1: https://curity.io/resources/learn/jwt-best-practices/
- 2: https://auth0.com/docs/secure/tokens/token-best-practices
- 3: https://snyk.io/blog/top-3-security-best-practices-for-handling-jwts/
- 4: https://zuplo.com/blog/2025/03/01/token-expiry-best-practices
- 5: https://community.auth0.com/t/what-are-the-best-practices-regarding-token-expiration/96294
- 6: https://www.softwaresecured.com/post/how-to-properly-secure-your-jwts
- 7: https://mojoauth.com/blog/understanding-jwt-expiration-time-claim-exp/
- 8: https://www.aptori.com/blog/jwt-security-best-practices-every-developer-should-know
- 9: https://learn.microsoft.com/en-us/answers/questions/845791/jwt-token-expiration-and-authorization
- 10: https://cheatsheetseries.owasp.org/cheatsheets/JSON_Web_Token_for_Java_Cheat_Sheet.html
Review and update JWT expiration and token strategy
The current generate_jwt sets the token to expire in 60 days—well beyond security best practices for access tokens. Industry guidance recommends access tokens live no more than 15 minutes–1 hour, and longer lifetimes (days/weeks) should be reserved for refresh tokens stored and rotated securely. Please:
• File app/models/user.rb
– At def generate_jwt, change
diff - exp: 60.days.from_now.to_i + exp: 15.minutes.from_now.to_i # or appropriate short-lived duration
• Introduce a refresh token mechanism:
– Generate and store a long-lived refresh token (e.g., 7–30 days) per user
– Expose an endpoint to exchange a valid refresh token for a new access token
– Implement secure storage and rotation of refresh tokens (e.g., blacklist on use)
This will align access tokens with best practices and limit exposure if a token is compromised.
Committable suggestion skipped: line range outside the PR's diff.
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