Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions app/controllers/users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@ def show

# POST /users
def create
# Add default password for a user if the password is not provided
params[:user][:password] ||= 'password'
user = User.new(user_params)
if user.save
render json: user, status: :created
Expand Down Expand Up @@ -70,6 +68,10 @@ def managed_users
def role_users
name = params[:name].split('_').map(&:capitalize).join(' ')
role = Role.find_by(name:)
unless role

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

role = Role.find_by!(name:)

Isn't this better?

render json: { error: "Role '#{name}' not found" }, status: :not_found
return
end
users = role.users
render json: users, status: :ok
rescue ActiveRecord::RecordNotFound => e
Expand Down
14 changes: 5 additions & 9 deletions app/models/ta.rb
Original file line number Diff line number Diff line change
@@ -1,23 +1,19 @@
# frozen_string_literal: true

class Ta < User
validates :parent_id, presence: true
Comment on lines 3 to +4

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this break existing entries?


# Get all users whose parent is the TA
# @return [Array<User>] all users that belongs to courses that is mapped to the TA
def managed_users
User.where(parent_id: id).to_a
end

def my_instructor
# code here
end

def courses_assisted_with
courses = TaMapping.where(ta_id: id)
courses.map { |c| Course.find(c.course_id) }
parent_id
end
Comment thread
coderabbitai[bot] marked this conversation as resolved.

def courses_assisted_with
courses = TaMapping.where(ta_id: id)
courses.map { |c| Course.find(c.course_id) }
Course.joins(:ta_mappings).where(ta_mappings: { user_id: id }).distinct.to_a

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is user_id present in ta_mappings?

end
end
end
79 changes: 47 additions & 32 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,24 +8,36 @@ class User < ApplicationRecord
validates :name, presence: true, uniqueness: true, allow_blank: false
# format: { with: /\A[a-z]+\z/, message: 'must be in lowercase' }
validates :email, presence: true, format: { with: URI::MailTo::EMAIL_REGEXP }
validates :password, length: { minimum: 6 }, presence: true, allow_nil: true
validates :password, presence: true, if: :password_required?
validates :password, length: { minimum: 6 }, allow_nil: true
validates :full_name, presence: true, length: { maximum: 50 }

belongs_to :role
belongs_to :institution, optional: true
belongs_to :parent, class_name: 'User', optional: true
has_many :users, foreign_key: 'parent_id', dependent: :nullify
has_many :invitations
has_many :assignments
has_many :teams_users, dependent: :destroy
has_many :teams, through: :teams_users

# Looking at invitation.rb, invitations relate to users only through participants.
# (from_id and to_id both point to AssignmentParticipant, not User).
# There is no user foreign key on the invitations table at all.
# has_many :invitations
has_many :participants

scope :students, -> { where role_id: Role::STUDENT }
scope :tas, -> { where role_id: Role::TEACHING_ASSISTANT }
scope :instructors, -> { where role_id: Role::INSTRUCTOR }
scope :administrators, -> { where role_id: Role::ADMINISTRATOR }
scope :superadministrators, -> { where role_id: Role::SUPER_ADMINISTRATOR }
# A user participates in assignments via the participants join table
has_many :assignments, through: :participants

# A user can also be the instructor of many assignments directly
# via instructor_id on the assignments table
has_many :instructed_assignments, class_name: 'Assignment', foreign_key: 'instructor_id'
has_many :teams_users, dependent: :destroy
has_many :teams, through: :teams_users

#join on role name instead of hardcoded IDs, matching create_roles_heirarchy
scope :students, -> { joins(:role).where(roles: { name: 'Student' }) }
scope :tas, -> { joins(:role).where(roles: { name: 'Teaching Assistant' }) }
scope :instructors, -> { joins(:role).where(roles: { name: 'Instructor' }) }
scope :administrators, -> { joins(:role).where(roles: { name: 'Administrator' }) }
scope :superadministrators,-> { joins(:role).where(roles: { name: 'Super Administrator' }) }

delegate :student?, to: :role
delegate :ta?, to: :role
Expand All @@ -34,17 +46,18 @@ class User < ApplicationRecord
delegate :super_administrator?, to: :role

def self.instantiate(record)
case record.role
when Role::TEACHING_ASSISTANT
case record.role.name
when /Super Administrator/
record.becomes(SuperAdministrator)
when /Teaching Assistant/
record.becomes(Ta)
when Role::INSTRUCTOR
when /Instructor/
record.becomes(Instructor)
when Role::ADMINISTRATOR
when /Administrator/
record.becomes(Administrator)
when Role::SUPER_ADMINISTRATOR
record.becomes(SuperAdministrator)
else
super
# Student or other roles remain as User
record
end
end
Comment on lines 48 to 62

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

find . -type f -name "*.rb" | grep -E "(user|spec|test)" | head -20

Repository: expertiza/reimplementation-back-end

Length of output: 948


🏁 Script executed:

cat -n app/models/user.rb | head -100

Repository: expertiza/reimplementation-back-end

Length of output: 4388


🏁 Script executed:

rg "User\.find|User\.all|\.instantiate" --type rb -A 2 -B 2

Repository: expertiza/reimplementation-back-end

Length of output: 106


🏁 Script executed:

rg "instantiate" --type rb -A 3 -B 3

Repository: expertiza/reimplementation-back-end

Length of output: 106


🏁 Script executed:

rg "instantiate" -A 3 -B 3

Repository: expertiza/reimplementation-back-end

Length of output: 3014


🏁 Script executed:

rg "\.find|\.all" --max-count 20 -B 2 -A 2

Repository: expertiza/reimplementation-back-end

Length of output: 50392


🏁 Script executed:

rg "class.*Administrator|class.*Ta|class.*Instructor" --max-count 10

Repository: expertiza/reimplementation-back-end

Length of output: 1306


🏁 Script executed:

find . -type f -name "*_spec.rb" -o -name "*_test.rb" | xargs grep -l "instantiate\|User\.find\|User\.all" 2>/dev/null | head -5

Repository: expertiza/reimplementation-back-end

Length of output: 161


🏁 Script executed:

cat -n spec/models/user_spec.rb | grep -A 30 "describe '\.instantiate'"

Repository: expertiza/reimplementation-back-end

Length of output: 1602


🏁 Script executed:

rg "User\.find\(|User\.all" app/controllers/users_controller.rb -B 2 -A 5

Repository: expertiza/reimplementation-back-end

Length of output: 1040


🏁 Script executed:

rg "def test_\|it '" spec/models/user_spec.rb | head -30

Repository: expertiza/reimplementation-back-end

Length of output: 61


Rename method to avoid colliding with Rails's instantiate signature.

The custom method overrides Active Record's instantiate(attributes, column_types = {}) with an incompatible signature expecting a model instance instead of an attributes hash. This breaks the contract with Rails's finder methods (User.find, User.all, etc.), which internally call instantiate(attributes_hash, column_types_dict) during record materialization.

Rename to cast_for_role or similar to make the intent explicit and avoid shadowing Rails internals.

Suggested fix
-  def self.instantiate(record)
+  def self.cast_for_role(record)
     case record.role.name
     when /Super Administrator/
       record.becomes(SuperAdministrator)
     when /Teaching Assistant/
       record.becomes(Ta)
     when /Instructor/
       record.becomes(Instructor)
     when /Administrator/
       record.becomes(Administrator)
     else
       # Student or other roles remain as User
       record
     end
   end

Update the controller call at app/controllers/users_controller.rb:managed_users to use the new method name.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/models/user.rb` around lines 48 - 62, The custom class method
self.instantiate is shadowing ActiveRecord.instantiate (which expects attributes
hash)—rename this method to something like self.cast_for_role(record) to avoid
breaking Rails' internal materialization; update all call sites (e.g., the
managed_users action in UsersController that currently calls User.instantiate)
to call User.cast_for_role(record) instead, and search the codebase for any
other references to instantiate to replace them so Rails' instantiate signature
remains untouched.


Expand All @@ -65,25 +78,29 @@ def self.login_user(login)
# Reset the password for the user
def reset_password
random_password = SecureRandom.alphanumeric(10)
user.password_digest = BCrypt::Password.create(random_password)
user.save
self.password = random_password
save
end

def password_required?
password_digest.blank? || !password.nil?
end

# Get instructor_id of the user, if the user is TA,
# return the id of the instructor else return the id of the user for superior roles
def instructor_id
case role
when Role::INSTRUCTOR, Role::ADMINISTRATOR, Role::SUPER_ADMINISTRATOR
case role.name
when /Instructor/, /Administrator/, /Super Administrator/
id
when Role::TEACHING_ASSISTANT
my_instructor
when /Teaching Assistant/
Ta.find(id).my_instructor
else
raise NotImplementedError, "Unknown role: #{role.name}"
end
end

def can_impersonate?(user)
return true if role.super_admin?
return true if role.super_administrator?
return true if teaching_assistant_for?(user)
return true if recursively_parent_of(user)

Expand All @@ -94,7 +111,7 @@ def recursively_parent_of(user)
p = user.parent
return false if p.nil?
return true if p == self
return false if p.role.super_admin?
return false if p.role.super_administrator?

recursively_parent_of(p)
end
Expand All @@ -103,16 +120,14 @@ def teaching_assistant_for?(student)
return false unless teaching_assistant?
return false unless student.role.name == 'Student'

# We have to use the Ta object instead of User object
# because single table inheritance is not currently functioning
ta = Ta.find(id)
return true if ta.courses_assisted_with.any? do |c|
ta.courses_assisted_with.any? do |c|
c.assignments.map(&:participants).flatten.map(&:user_id).include? student.id
end
end

def teaching_assistant?
true if role.ta?
!!role.ta?
end

def self.from_params(params)
Expand All @@ -135,8 +150,8 @@ def as_json(options = {})
institution: { only: %i[id name] }
}
})).tap do |hash|
hash['parent'] ||= { id: nil, name: nil }
hash['institution'] ||= { id: nil, name: nil }
hash['parent'] ||= { 'id' => nil, 'name' => nil }
hash['institution'] ||= { 'id' => nil, 'name' => nil }
end
end

Expand All @@ -150,7 +165,7 @@ def set_defaults
end

def generate_jwt
JWT.encode({ id: id, exp: 60.days.from_now.to_i }, Rails.application.credentials.secret_key_base)
JWT.encode({ id: id, exp: 60.days.from_now.to_i }, Rails.application.credentials.secret_key_base, 'HS256')
Comment on lines 167 to +168

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

User#generate_jwt no longer matches the app's decoder.

app/controllers/concerns/jwt_token.rb authenticates with JsonWebToken.decode, and lib/json_web_token.rb signs tokens with RS256 RSA keys. This method now emits HS256 tokens with secret_key_base, so tokens generated here will be rejected by authenticated endpoints.

Suggested fix
   def generate_jwt
-    JWT.encode({ id: id, exp: 60.days.from_now.to_i }, Rails.application.credentials.secret_key_base, 'HS256')
+    JsonWebToken.encode({ id: id }, 60.days.from_now)
   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.

Suggested change
def generate_jwt
JWT.encode({ id: id, exp: 60.days.from_now.to_i }, Rails.application.credentials.secret_key_base)
JWT.encode({ id: id, exp: 60.days.from_now.to_i }, Rails.application.credentials.secret_key_base, 'HS256')
def generate_jwt
JsonWebToken.encode({ id: id }, 60.days.from_now)
end
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/models/user.rb` around lines 167 - 168, User#generate_jwt is creating
HS256 tokens with secret_key_base which no longer matches JsonWebToken.decode
(which expects RS256 with RSA keys). Update generate_jwt to sign with the same
RSA private key and algorithm used by lib/json_web_token.rb/JsonWebToken.encode:
load the app's JWT RSA private key from credentials (the same credential used by
JsonWebToken), build the same payload (id and exp) and encode using RS256 so
tokens generated by User#generate_jwt are accepted by JsonWebToken.decode.
Ensure you reference the method name generate_jwt and the JsonWebToken
implementation so the key source and alg match exactly.

end

end
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# frozen_string_literal: true

class UpdateTeamsParticipantsUniqueIndex < ActiveRecord::Migration[8.0]
def change
remove_index :teams_participants, name: :index_teams_participants_on_participant_id_unique

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How was index_teams_participants_on_participant_id_unique being used earlier?

add_index :teams_participants, %i[team_id participant_id], unique: true,
name: :index_teams_participants_on_team_participant_unique
end
end
5 changes: 3 additions & 2 deletions db/schema.rb

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading