Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AO3-6672 Allow some admins to set a generic username #4994

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open
1 change: 1 addition & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ Lint/AmbiguousBlockAssociation:
Exclude:
# Exception for specs where we use change matchers:
# https://github.com/rubocop-hq/rubocop/issues/4222
- 'features/step_definitions/**/*.rb'
- 'spec/**/*.rb'

Lint/AmbiguousRegexpLiteral:
Expand Down
65 changes: 29 additions & 36 deletions app/controllers/pseuds_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,44 +26,40 @@ def index

# GET /users/:user_id/pseuds/:id
def show
if @user.blank?
raise ActiveRecord::RecordNotFound, "Couldn't find user '#{params[:user_id]}'"
end
@pseud = @user.pseuds.find_by(name: params[:id])
unless @pseud
raise ActiveRecord::RecordNotFound, "Couldn't find pseud '#{params[:id]}'"
end
raise ActiveRecord::RecordNotFound, t(".could_not_find_user", username: params[:user_id]) if @user.blank?
brianjaustin marked this conversation as resolved.
Show resolved Hide resolved

@pseud = @user.pseuds.find_by!(name: params[:id])
@page_subtitle = @pseud.name

# very similar to show under users - if you change something here, change it there too
if !(logged_in? || logged_in_as_admin?)
visible_works = @pseud.works.visible_to_all
visible_series = @pseud.series.visible_to_all
visible_bookmarks = @pseud.bookmarks.visible_to_all
else
if logged_in? || logged_in_as_admin?
visible_works = @pseud.works.visible_to_registered_user
visible_series = @pseud.series.visible_to_registered_user
visible_bookmarks = @pseud.bookmarks.visible_to_registered_user
else
visible_works = @pseud.works.visible_to_all
visible_series = @pseud.series.visible_to_all
visible_bookmarks = @pseud.bookmarks.visible_to_all
end

visible_works = visible_works.revealed.non_anon
visible_series = visible_series.exclude_anonymous

@fandoms = \
Fandom.select("tags.*, count(DISTINCT works.id) as work_count").
joins(:filtered_works).group("tags.id").merge(visible_works).
where(filter_taggings: { inherited: false }).
order('work_count DESC').load
Fandom.select("tags.*, count(DISTINCT works.id) as work_count")
.joins(:filtered_works).group("tags.id").merge(visible_works)
.where(filter_taggings: { inherited: false })
.order("work_count DESC").load

@works = visible_works.order("revised_at DESC").limit(ArchiveConfig.NUMBER_OF_ITEMS_VISIBLE_IN_DASHBOARD)
@series = visible_series.order("updated_at DESC").limit(ArchiveConfig.NUMBER_OF_ITEMS_VISIBLE_IN_DASHBOARD)
@bookmarks = visible_bookmarks.order("updated_at DESC").limit(ArchiveConfig.NUMBER_OF_ITEMS_VISIBLE_IN_DASHBOARD)

if current_user.respond_to?(:subscriptions)
@subscription = current_user.subscriptions.where(subscribable_id: @user.id,
subscribable_type: 'User').first ||
current_user.subscriptions.build(subscribable: @user)
end
return unless current_user.respond_to?(:subscriptions)

@subscription = current_user.subscriptions.where(subscribable_id: @user.id,
subscribable_type: "User").first ||
current_user.subscriptions.build(subscribable: @user)
end

# GET /pseuds/new
Expand All @@ -86,7 +82,7 @@ def create
@pseud.user_id = @user.id
old_default = @user.default_pseud
if @pseud.save
flash[:notice] = ts('Pseud was successfully created.')
flash[:notice] = t(".successfully_created")
if @pseud.is_default
# if setting this one as default, unset the attribute of the current default pseud
old_default.update_attribute(:is_default, false)
Expand All @@ -96,8 +92,8 @@ def create
render action: "new"
end
else
# user tried to add pseud he already has
flash[:error] = ts('You already have a pseud with that name.')
# user tried to add pseud they already have
flash[:error] = t(".already_have_pseud_with_name")
render action: "new"
end
end
Expand All @@ -115,12 +111,9 @@ def update
AdminActivity.log_action(current_admin, @pseud, action: "edit pseud", summary: summary)
end
# if setting this one as default, unset the attribute of the current default pseud
if @pseud.is_default and not(default == @pseud)
# if setting this one as default, unset the attribute of the current active pseud
default.update_attribute(:is_default, false)
end
flash[:notice] = ts('Pseud was successfully updated.')
redirect_to([@user, @pseud])
default.update_attribute(:is_default, false) if @pseud.is_default && default != @pseud
flash[:notice] = t(".successfully_updated")
redirect_to([@user, @pseud])
else
render action: "edit"
end
Expand All @@ -131,24 +124,24 @@ def update
def destroy
@hide_dashboard = true
if params[:cancel_button]
flash[:notice] = ts("The pseud was not deleted.")
flash[:notice] = t(".not_deleted")
redirect_to(user_pseuds_path(@user)) && return
end

@pseud = @user.pseuds.find_by(name: params[:id])
if @pseud.is_default
flash[:error] = ts("You cannot delete your default pseudonym, sorry!")
flash[:error] = t(".cannot_delete_default")
elsif @pseud.name == @user.login
flash[:error] = ts("You cannot delete the pseud matching your user name, sorry!")
flash[:error] = t(".cannot_delete_matching_username")
elsif params[:bookmarks_action] == "transfer_bookmarks"
@pseud.change_bookmarks_ownership
@pseud.replace_me_with_default
flash[:notice] = ts("The pseud was successfully deleted.")
flash[:notice] = t(".successfully_deleted")
elsif params[:bookmarks_action] == "delete_bookmarks" || @pseud.bookmarks.empty?
@pseud.replace_me_with_default
flash[:notice] = ts("The pseud was successfully deleted.")
flash[:notice] = t(".successfully_deleted")
else
render 'delete_preview' and return
render "delete_preview" and return
end

redirect_to(user_pseuds_path(@user))
Expand Down
22 changes: 15 additions & 7 deletions app/controllers/users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ class UsersController < ApplicationController

before_action :check_user_status, only: [:edit, :update, :change_username, :changed_username]
before_action :load_user, except: [:activate, :delete_confirmation, :index]
before_action :check_ownership, except: [:activate, :delete_confirmation, :edit, :index, :show, :update]
before_action :check_ownership_or_admin, only: [:edit, :update]
before_action :check_ownership, except: [:activate, :change_username, :changed_username, :delete_confirmation, :edit, :index, :show, :update]
before_action :check_ownership_or_admin, only: [:change_username, :changed_username, :edit, :update]
skip_before_action :store_location, only: [:end_first_login]

def load_user
Expand Down Expand Up @@ -48,6 +48,7 @@ def change_password
end

def change_username
authorize @user if logged_in_as_admin?
@page_subtitle = t(".browser_title")
end

Expand All @@ -70,20 +71,27 @@ def changed_password
end

def changed_username
render(:change_username) && return unless params[:new_login].present?
authorize @user if logged_in_as_admin?
render(:change_username) && return if params[:new_login].blank?

@new_login = params[:new_login]

unless @user.valid_password?(params[:password])
flash[:error] = ts('Your password was incorrect')
unless logged_in_as_admin? || @user.valid_password?(params[:password])
flash[:error] = t(".user.incorrect_password")
render(:change_username) && return
end

@user.login = @new_login
@user.ticket_number = params[:ticket_number]

if @user.save
flash[:notice] = ts('Your user name has been successfully updated.')
redirect_to @user
if logged_in_as_admin?
flash[:notice] = t(".admin.successfully_updated")
redirect_to admin_user_path(@user)
else
flash[:notice] = t(".user.successfully_updated")
redirect_to @user
end
else
@user.reload
render :change_username
Expand Down
6 changes: 3 additions & 3 deletions app/models/concerns/justifiable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,14 @@ module Justifiable
validates :ticket_number,
presence: true,
numericality: { only_integer: true },
if: :enabled?
if: :justification_enabled?

validate :ticket_number_exists_in_tracker, if: :enabled?
validate :ticket_number_exists_in_tracker, if: :justification_enabled?
end

private

def enabled?
def justification_enabled?
# Only require a ticket if the record has been changed by an admin.
User.current_user.is_a?(Admin) && changed?
end
Expand Down
2 changes: 1 addition & 1 deletion app/models/pseud.rb
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ def self.parse_byline(byline)

# Parse a string of the "pseud.name (user.login)" format into a list of
# pseuds. Usually this will be just one pseud, but if the byline is of the
# form "pseud.name" with no parenthesized user name, it'll look for any pseud
# form "pseud.name" with no parenthesized username, it'll look for any pseud
# with that name.
def self.parse_byline_ambiguous(byline)
pseud_name, login = split_byline(byline)
Expand Down
32 changes: 30 additions & 2 deletions app/models/user.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
class User < ApplicationRecord
audited redacted: [:encrypted_password, :password_salt]
include Justifiable
include WorksOwner
include PasswordResetsLimitable
include UserLoggable
Expand Down Expand Up @@ -90,7 +91,7 @@ class User < ApplicationRecord
before_update :add_renamed_at, if: :will_save_change_to_login?
after_update :update_pseud_name
after_update :send_wrangler_username_change_notification, if: :is_tag_wrangler?
after_update :log_change_if_login_was_edited
after_update :log_change_if_login_was_edited, if: :saved_change_to_login?
after_update :log_email_change, if: :saved_change_to_email?

after_commit :reindex_user_creations_after_rename
Expand Down Expand Up @@ -198,6 +199,7 @@ def unread_inbox_comments_count
uniqueness: true,
not_forbidden_name: { if: :will_save_change_to_login? }
validate :username_is_not_recently_changed, if: :will_save_change_to_login?
validate :admin_username_generic, if: :will_save_change_to_login?

# allow nil so can save existing users
validates_length_of :password,
Expand Down Expand Up @@ -525,6 +527,12 @@ def reindex_user_creations

private

# Override the default Justifiable enabled check, because we only need to justify
# username changes at the moment.
def justification_enabled?
User.current_user.is_a?(Admin) && login_changed?
end

# Create and/or return a user account for holding orphaned works
def self.fetch_orphan_account
orphan_account = User.find_or_create_by(login: "orphan_account")
Expand Down Expand Up @@ -564,11 +572,23 @@ def reindex_user_creations_after_rename
end

def add_renamed_at
return if User.current_user.is_a?(Admin)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this exception will confuse us in the future - could it be removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Conversely, there could also be confusion if there isn't an exception. For example

  1. User is suspended
  2. User is renamed
  3. User is un-suspended within 7 days
  4. User tries to rename and gets an error, even though they didn't rename themselves within the past few days.

I don't think that's particularly common, but I see that as a worse confusion than otherwise (especially since we log the action in the audits table either way)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with Brian here. Since we're already supposed to be ignoring the renamed_at when an admin goes to change a name, I think it makes sense that admin renames just ignore the renamed_at entirely.

However, we could add a separate admin_renamed_at column to the users table for general tracking purposes. That does feel like something that could be both useful and clarifying if you're looking at an audit or just a user in the database.


self.renamed_at = Time.current
end

def log_change_if_login_was_edited
create_log_item(action: ArchiveConfig.ACTION_RENAME, note: "Old Username: #{login_before_last_save}; New Username: #{login}") if saved_change_to_login?
current_admin = User.current_user if User.current_user.is_a?(Admin)
options = {
action: ArchiveConfig.ACTION_RENAME,
admin: current_admin
}
options[:note] = if current_admin
"Old Username: #{login_before_last_save}, New Username: #{login}, Changed by: #{current_admin.login}, Ticket ID: ##{ticket_number}"
else
"Old Username: #{login_before_last_save}; New Username: #{login}"
end
create_log_item(options)
end

def send_wrangler_username_change_notification
Expand All @@ -592,6 +612,8 @@ def remove_stale_from_autocomplete
end

def username_is_not_recently_changed
return if User.current_user.is_a?(Admin)

change_interval_days = ArchiveConfig.USER_RENAME_LIMIT_DAYS
return unless renamed_at && change_interval_days.days.ago <= renamed_at

Expand All @@ -601,6 +623,12 @@ def username_is_not_recently_changed
renamed_at: I18n.l(renamed_at, format: :long))
end

def admin_username_generic
return unless User.current_user.is_a?(Admin)

errors.add(:login, :admin_must_use_default) unless login == "user#{id}"
end

# Extra callback to make sure readings are deleted in an order consistent
# with the ReadingsJob.
#
Expand Down
12 changes: 10 additions & 2 deletions app/policies/user_policy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ class UserPolicy < ApplicationPolicy
# This is further restricted using ALLOWED_ATTRIBUTES_BY_ROLES.
MANAGE_ROLES = %w[superadmin legal policy_and_abuse open_doors support tag_wrangling].freeze

# Roles that are allowed to set a generic username for users.
CHANGE_USERNAME_ROLES = %w[superadmin policy_and_abuse].freeze

# Roles that allow updating the Fannish Next Of Kin of a user.
MANAGE_NEXT_OF_KIN_ROLES = %w[superadmin policy_and_abuse support].freeze

Expand All @@ -18,8 +21,8 @@ class UserPolicy < ApplicationPolicy
# Define which roles can update which attributes.
ALLOWED_ATTRIBUTES_BY_ROLES = {
"open_doors" => [roles: []],
"policy_and_abuse" => [:email, roles: []],
"superadmin" => [:email, roles: []],
"policy_and_abuse" => [:email, { roles: [] }],
"superadmin" => [:email, { roles: [] }],
"support" => %i[email],
"tag_wrangling" => [roles: []]
}.freeze
Expand Down Expand Up @@ -48,6 +51,10 @@ def can_access_creation_summary?
user_has_roles?(REVIEW_CREATIONS_ROLES)
end

def change_username?
user_has_roles?(CHANGE_USERNAME_ROLES)
end

def permitted_attributes
ALLOWED_ATTRIBUTES_BY_ROLES.values_at(*user.roles).compact.flatten
end
Expand All @@ -60,6 +67,7 @@ def can_edit_user_role?(role)
alias bulk_search? can_manage_users?
alias show? can_manage_users?
alias update? can_manage_users?
alias changed_username? change_username?

alias update_next_of_kin? can_manage_next_of_kin?

Expand Down
29 changes: 17 additions & 12 deletions app/views/admin/admin_invitations/find.html.erb
Original file line number Diff line number Diff line change
@@ -1,30 +1,35 @@
<!--Descriptive page name, messages and instructions-->
<h2 class="heading"><%h= 'Track invitations' %></h2>
<h2 class="heading"><%= t(".page_heading") %></h2>
<!--/descriptions-->

<!--subnav-->
<!--/subnav-->

<!--main content-->
<%= form_tag url_for(:controller => 'admin/admin_invitations', :action => 'find'), :method => :get do %>
<%= form_tag url_for(controller: "admin/admin_invitations", action: "find"), method: :get do %>
<dl>
<dt><%= label_tag "invitation[user_name]", t(".user_name") %>:</dt>
<dd><%= text_field_tag "invitation[user_name]", params[:invitation][:user_name] %></dd>
<dt><%= label_tag "invitation[token]", t(".token") %>:</dt>
<dd><%= text_field_tag "invitation[token]", params[:invitation][:token] %></dd>
<dt><%= label_tag "invitee_email", t(".email") %>:</dt>
<dd><%= text_field_tag "invitation[invitee_email]", params[:invitation][:invitee_email], id: "invitee_email" %></dd>
<dt><%= label_tag "invitation[user_name]", t(".username") %></dt>
<dd><%= text_field_tag "invitation[user_name]", params[:invitation][:user_name], autocomplete: "on" %></dd>
<dt><%= label_tag "invitation[token]", t(".token") %></dt>
<dd><%= text_field_tag "invitation[token]", params[:invitation][:token], autocomplete: "on" %></dd>
brianjaustin marked this conversation as resolved.
Show resolved Hide resolved
<dt><%= label_tag "invitee_email", t(".email") %></dt>
<dd>
<%= text_field_tag "invitation[invitee_email]",
params[:invitation][:invitee_email],
autocomplete: "email",
id: "invitee_email" %>
</dd>
</dl>
<p class="submit actions"><%= submit_tag "Go" %></p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missed a string, although it could really be "Search" instead of "Go."

<% end %>

<% if @user %>
<h3 class="heading"><%h= 'Invitations for' %> <%= link_to @user.login, user_invitations_path(@user) %></h3>
<%= render :partial => 'invitations/user_invitations', :locals => {:invitations => @invitations} %>
<h3 class="heading"><%= t(".invitations_for") %> <%= link_to @user.login, user_invitations_path(@user) %></h3>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this needs some interpolation so the link to the name is part of the translated text.

<%= render partial: "invitations/user_invitations", locals: { invitations: @invitations } %>
<% elsif @invitation %>
<%= render :partial => 'invitations/invitation', :locals => {:invitation => @invitation} %>
<%= render partial: "invitations/invitation", locals: { invitation: @invitation } %>
<% elsif @invitations %>
<%= render :partial => 'invitations/user_invitations', :locals => {:invitations => @invitations} %>
<%= render partial: "invitations/user_invitations", locals: { invitations: @invitations } %>
brianjaustin marked this conversation as resolved.
Show resolved Hide resolved
<% end %>
<!--/content-->

Expand Down
Loading
Loading