Deny admin self deletion and self admin rights revocation #221

stage-rl · 2023-11-30T17:36:22Z

No description provided.

jsuchal · 2023-11-30T17:44:50Z

app/models/user.rb

-  def delete_user_group
+  def before_destroy
+    if self == Current.user
+      errors.add :name, "Administrátor nemôže zmazať svojho používateľa"


Tu chceme asi povedat, ze nemozes zmazat sam seba a idealne by bolo aby ten button ani nebolo vidiet.

jsuchal · 2023-11-30T19:10:26Z

app/components/admin/groups/members_list_row_component.html.erb

 </div>
 <div class="grow shrink basis-0 flex-col justify-start items-start gap-1 inline-flex">
  <div class="text-center text-gray-900 text-lg font-medium leading-loose"><%= @user.name %></div>
  <div class="text-center text-gray-500 text-base font-normal leading-normal"><%= @user.email %></div>
 </div>
-<%= button_to admin_tenant_group_group_membership_path(Current.tenant, @group_membership.group, @group_membership), method: :delete do %>
-  <%= render Common::DeleteButtonComponent.new %>
+<% unless @group_membership.group.type == "AdminGroup" && @group_membership.user == Current.user %>


nemame na toto presne tie policy objekty ci ideme duplikovat logiku?

Kedze tu chcem ten objekt sice vymenovat, len neumoznit mazanie, tak neviem ako tu pouzit policy. Takze ak, tak daj pls vediet ako, alebo posli priklad. Neukazat, ze tam ten admin je, mi pripada cudne

Toto je zdokumentacie pundita. Cize tu by som ja cakal nieco ako

if policy(@group_membership).delete? ...

Ano, uz som si to aj ja nasiel. A spravil. A hned som mal na pozadi warning, ze urcite sa ti nebude pacit toto v komponente (lebo select do DB). Napriek tomu, ze pundit vyslovene vyzyva k pouzivaniu vo viewoch

To, kde sa vykona ten select je vecou toho ako sa tie objekty hore loadnu. Cize nemusi to nutne znamenat ziadny select navyse.

jsuchal · 2023-11-30T19:53:45Z

Príklad nemám ale logicky. Toto JE politika ako sa môže a nemôže mazať group membership. To by sme mali mať v nejakej policy.

…

On Thu, Nov 30, 2023, 20:48 Robo Lences ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In app/components/admin/groups/members_list_row_component.html.erb <https://github.com/solver-it-sro/govbox-pro/pull/221#discussion_r1411188073> : > </div> <div class="grow shrink basis-0 flex-col justify-start items-start gap-1 inline-flex"> <div class="text-center text-gray-900 text-lg font-medium leading-loose"><%= @user.name %></div> <div class="text-center text-gray-500 text-base font-normal leading-normal"><%= @user.email %></div> </div> -<%= button_to admin_tenant_group_group_membership_path(Current.tenant, @group_membership.group, @group_membership), method: :delete do %> - <%= render Common::DeleteButtonComponent.new %> +<% unless @group_membership.group.type == "AdminGroup" && @group_membership.user == Current.user %> Kedze tu chcem ten objekt sice vymenovat, len neumoznit mazanie, tak neviem ako tu pouzit policy. Takze ak, tak daj pls vediet ako, alebo posli priklad. Neukazat, ze tam ten admin je, mi pripada cudne — Reply to this email directly, view it on GitHub <https://github.com/solver-it-sro/govbox-pro/pull/221#discussion_r1411188073>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAGNZWGRJQUTSYMZR4DTP3YHDPJNAVCNFSM6AAAAABABSIX7OVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONJYGMZDIOJYG4> . You are receiving this because your review was requested.Message ID: ***@***.***>

jsuchal · 2023-12-03T19:38:14Z

app/policies/admin/group_membership_policy.rb

  def destroy?
-    @user.site_admin? || @user.admin?
+    (@user.site_admin? || @user.admin?) && !(@group_membership.user == @user && @group_membership.group.type == 'AdminGroup')


Toto by som rozdelil aspon na dva riadky lebo sa to zle cita a su to vlastne rozne podmienky

nieco v zmysle:

return false if @group_membership.user == @user return true if @user.site_admin? || @user.admin? return true

imho by sam seba si nemal vediet zmazat vobec nikdy, nejako sa mi tam nepozdava to obmedzenie na admin group

Ale tak to je odobranie sa zo skupiny (nie zmazanie usera). A admin moze chciet napr nieco otestovat, hodi sa do skupiny, a nasledne sa z nej odoberie. Aktualne mi len podpisovanie napada.

Ano myslel som tym clenstvo v skupine vseobecne. Ale pravda, ze undo by mal vediet asi spravit cize dava zmysel, aby zmazal aj sam seba pokial si neodpali admina.

jsuchal · 2023-12-03T19:38:44Z

app/policies/admin/user_policy.rb

@@ -5,6 +5,7 @@ class Admin::UserPolicy < ApplicationPolicy

  def initialize(user_logged_in, user_to_authorize)
    @user = user_logged_in


Nejako som to ponastavoval, ale pozri, ci si s tym takto OK. Vyrabam tam natvrdo metodu user, kedze Pundit celkom vyzera, ze s nou pocita (vsade je tam attr_reader :user). Aj ked aj bez nej mi to islo. Trochu sa bojim takychto zasahov mimo zdokumentovanych standardov

jsuchal · 2023-12-03T19:38:59Z

app/policies/admin/user_policy.rb

@@ -42,7 +43,6 @@ def edit?
  end

  def destroy?
-    @user.site_admin? || @user.admin?
+    (@user.site_admin? || @user.admin?) && @user_to_authorize != @user


Toto tiez rozdelme na riadky.

jsuchal · 2023-12-03T19:56:47Z

app/policies/admin/user_policy.rb

-    @user.site_admin? || @user.admin?
-  end
+    return false unless @actor.site_admin? || @actor.admin?
+    return true unless @user_to_authorize != @actor


mas tu dva zapory.

Suggested change

return true unless @user_to_authorize != @actor

return false if @user_to_authorize == @actor

jsuchal · 2023-12-03T20:21:48Z

app/models/group_membership.rb

+  private
+
+  def validate_self_admin_removal
+    return unless group.type == 'AdminGroup' && user == Current.user


Toto tu je este potrebne? Resp vidim tu este duplicitu

Beriem teda, ze hovoris, ze princip je chytat taketo chyby co najvyssie, a neduplikovat to "dole". Tak?

Kedze pouzivame pundit, ktory vytrhava security checky do samostatneho layer, tak to drzme teda tam.

jsuchal · 2023-12-03T20:22:43Z

app/models/user.rb

  has_many :message_drafts, foreign_key: :author_id
  has_many :automation_rules, class_name: 'Automation::Rule'
  has_many :filters, foreign_key: :author_id

  validates_presence_of :name, :email
  validates_uniqueness_of :name, :email, scope: :tenant_id, case_sensitive: false

-  before_destroy :delete_user_group, prepend: true
+  before_destroy :before_destroy, prepend: true


Toto bolo pekne pomenovane, ze co to urobi a teraz uz nevies co to urobi. Netreba sa hanbit mat aj 2 metody zavesene na before_destroy hook ak to dava zmysel

Deny admin self deletion and self admin rights revocation

9bedb10

stage-rl requested a review from jsuchal November 30, 2023 17:36

jsuchal reviewed Nov 30, 2023

View reviewed changes

Remove risky admin actions from view

138e7f9

jsuchal reviewed Nov 30, 2023

View reviewed changes

Use pundit policies to disable selfdestroy

5922070

stage-rl requested a review from jsuchal December 3, 2023 14:21

jsuchal reviewed Dec 3, 2023

View reviewed changes

Refactor based on PR review

24d8bf4

jsuchal reviewed Dec 3, 2023

View reviewed changes

Refactor based on PR review

4cc123a

jsuchal reviewed Dec 3, 2023

View reviewed changes

stage-rl added 2 commits December 4, 2023 08:56

Cleanup model checks

186746e

Refactor user vs actor in user policy

2fc4777

stage-rl requested a review from jsuchal December 4, 2023 08:36

jsuchal approved these changes Dec 4, 2023

View reviewed changes

stage-rl merged commit 8592c14 into main Dec 4, 2023

jsuchal deleted the GO-401/deny_self_user_deletion branch December 24, 2023 09:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deny admin self deletion and self admin rights revocation #221

Deny admin self deletion and self admin rights revocation #221

stage-rl commented Nov 30, 2023

jsuchal Nov 30, 2023

jsuchal Nov 30, 2023

stage-rl Nov 30, 2023

jsuchal Dec 1, 2023

stage-rl Dec 1, 2023

jsuchal Dec 1, 2023

jsuchal commented Nov 30, 2023 via email

jsuchal Dec 3, 2023

stage-rl Dec 3, 2023

jsuchal Dec 3, 2023

jsuchal Dec 3, 2023

stage-rl Dec 4, 2023

jsuchal Dec 3, 2023

jsuchal Dec 3, 2023

jsuchal Dec 3, 2023

stage-rl Dec 4, 2023

jsuchal Dec 4, 2023

jsuchal Dec 3, 2023

		@@ -5,6 +5,7 @@ class Admin::UserPolicy < ApplicationPolicy

		def initialize(user_logged_in, user_to_authorize)
		@user = user_logged_in

	return true unless @user_to_authorize != @actor
	return false if @user_to_authorize == @actor

Deny admin self deletion and self admin rights revocation #221

Deny admin self deletion and self admin rights revocation #221

Conversation

stage-rl commented Nov 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsuchal commented Nov 30, 2023 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment