Skip to content
Open
Show file tree
Hide file tree
Changes from 4 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
2 changes: 1 addition & 1 deletion apps/dashboard/app/controllers/projects_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ def templates
def project_params
params
.require(:project)
.permit(:name, :directory, :description, :icon, :id, :template)
.permit(:name, :directory, :description, :icon, :id, :template, :group_owner)
end

def show_project_params
Expand Down
35 changes: 35 additions & 0 deletions apps/dashboard/app/helpers/projects_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,4 +31,39 @@ def button_category(status)
status
end
end

def possible_user_groups(project)
user_groups = Process.groups.map{|id| Etc.getgrgid(id).name}
# accept path arg directly for debugging
root_dir = project.is_a?(Pathname) ? project : project.project_dataroot

ancestors = []
root_dir.ascend do |parent|
ancestors.push(parent) unless parent == root_dir
end

candidates = user_groups.dup

# check that ancestors are not restricted to specific groups
ancestors.each do |a|
st = a.stat
mode = st.mode & 0o777
other_x = (mode & 0o001) != 0
group_x = (mode & 0o010) != 0

next if other_x

if group_x
gname = Etc.getgrgid(st.gid).name rescue nil
return [] unless gname
# if they are, restrict candidates to that group
candidates &= [gname]
else
return []
end
break if candidates.empty?
end

candidates.sort.uniq
end
end
31 changes: 29 additions & 2 deletions apps/dashboard/app/models/project.rb
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ def importable_directories
end
end

attr_reader :id, :name, :description, :icon, :directory, :template, :files
attr_reader :id, :name, :description, :icon, :directory, :template, :files, :group_owner

validates :name, presence: { message: :required }, on: [:create, :update]
validates :id, :directory, :icon, presence: { message: :required }, on: [:update]
Expand All @@ -128,7 +128,7 @@ def initialize(attributes = {})
@directory = attributes[:directory]
@directory = File.expand_path(@directory) unless @directory.blank?
@template = attributes[:template]

@group_owner = get_group_owner
Copy link
Contributor Author

@Bubballoo3 Bubballoo3 Nov 15, 2025

Choose a reason for hiding this comment

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

Although this instance variable is not explicitly used, it is responsible for setting the initial value in the form

return if new_record?

contents = File.read(manifest_path)
Expand Down Expand Up @@ -166,6 +166,7 @@ def save
def update(attributes)
update_attrs(attributes)

return false unless update_ownership(attributes)
return false unless valid?(:update)

store_manifest(:update)
Expand Down Expand Up @@ -203,6 +204,23 @@ def remove_from_lookup
false
end

def get_group_owner
if project_dataroot.grpowned?
Etc.getgrgid(project_dataroot.stat.gid).name
else
'None'
end
end

def set_group_owner(group)
begin
FileUtils.chown(nil, Etc.getgrnam(group).gid, project_dataroot)
rescue StandardError => e
errors.add(:update, "Unable to modify group ownership with error #{e.class}:#{e.message}")
false
end
end

def icon_class
# rails will prepopulate the tag with fa- so just the name is needed
icon.sub('fas://', '')
Expand Down Expand Up @@ -299,6 +317,15 @@ def update_attrs(attributes)
end
end

def update_ownership(attributes)
new_group = attributes.fetch(:group_owner, false)
if new_group
(new_group == 'None') ? true : set_group_owner(new_group)
else
true
end
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even though this could be condensed by combining the 'None' case and the unset case, I left them separate so that we can replace true in the ternary with the ownership removal later. Otherwise we could simplify it to just

new_group = attributes.fetch(:group_owner, 'None')
(new_group == 'None') ? true : set_group_owner(new_group)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see where this is being called from. Also - None value is likely dangerous if it's ever used. It should default to the users Primary group as that will always be a valid fallback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I already removed the 'None' option from the select, as that was only necessary when we were detecting the 'possible groups', which could have been an empty set. Now that we are detecting the user's groups directly, there is no way to submit 'None'. However I see how this could create issues, maybe just sticking with nil as the default is a better idea. It should still cause set_group_owner to rescue, but ideally without any unintended effects


def make_dir
project_dataroot.mkpath unless project_dataroot.exist?
configuration_directory.mkpath unless configuration_directory.exist?
Expand Down
7 changes: 7 additions & 0 deletions apps/dashboard/app/views/projects/_form.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,13 @@
<div class="field">
<%= form.text_area :description, placeholder: I18n.t('dashboard.jobs_project_description_placeholder') %>
</div>

<% groups = edit_project_action ? possible_user_groups(@project) : [] %>
<% unless groups.empty? %>
<div class="field">
<%= form.select :group_owner, groups.unshift('None'), label: 'Group owner' %>
</div>
<% end %>
</div>
</div>
</div>
Expand Down
Loading