Fixes #37936 - As a user, I want to invalidate jwt for specific user( UI ) #10357

girijaasoni · 2024-10-21T22:53:59Z

No description provided.

ares

Leaving the comment for the the proper review, this can be easily missed but I believe is important to get it right.

ares · 2024-10-22T07:59:04Z

app/controllers/api/v2/users_controller.rb

+          render :json => { :error => _("User %s does not exist.") % @user.login}
+        elsif  !@user.can?(:edit_users)
+          deny_access N_("User %s does not have permissions to invalidate" % @user.login)
+        else @user.can?(:edit_users) && @user.invalidate_jwt.blank?


I think this is not enough. edit_users permission may have specific conditions set. This would allow any user that can edit one specific user's profile to invalidate token for all users, introducing a privilege escalation.

If you look at can? definition you can see it also accepts subject which should the permission be verified on. We need to be checking that user can edit_users on the specific @user.

Ack, I'll look into updating the subject

ares · 2024-10-22T08:00:37Z

app/helpers/users_helper.rb

@@ -33,14 +33,22 @@ def user_action_buttons(user, additional_actions = [])
        :method => :post,
        :data => { :no_turbolink => true })
    end
+
+    if user != User.current
+      additional_actions <<  display_link_if_authorized(_("Invalidate JWT"),


the exact same problem like with can? above, the auth object needs to be specified. If this is used on some users listing page, to avoid N+1 issue, an authorized object needs to be used.

I am already adding :auth_object here, is this what you're talking about?

ShimShtein · 2024-10-28T08:31:16Z

app/controllers/api/v2/users_controller.rb

@@ -111,6 +111,23 @@ def update
        end
      end

+      api :PATCH, "/users/:id/invalidate_jwt", N_("Get vm attributes of host")


Copy-paste leftovers?

yes, my bad

ShimShtein · 2024-10-28T08:33:57Z

app/controllers/api/v2/users_controller.rb

@@ -111,6 +111,23 @@ def update
        end
      end

+      api :PATCH, "/users/:id/invalidate_jwt", N_("Get vm attributes of host")
+      param :id, String, :required => true


I suppose you would want to have an array parameter here. To be able to invalidate tokens for multiple users.

currently in the scope, (atleast UI wise) we can invalidate tokens for a specific user or all users because otherwise I would have to add a select action in the UI. I was thinking, I can add another endpoint to invalidate all at once. Allowing multiple (but not all) users token invalidation can take some time to integrate

ShimShtein · 2024-10-28T09:08:57Z

app/controllers/api/v2/users_controller.rb

+      DOC
+
+      def invalidate_jwt
+        @user = User.find(params[:id])


Suggested change

@user = User.find(params[:id])

@user = User.where(id: params[:id]).authorized(:edit_user)

This will filter only the users that edit_user permission applies to them, including custom filters.

Now you will have a set of users that you can safely delete the tokens for:

JwtSecret.where(user_id: @users).destroy_all

ShimShtein · 2024-10-28T09:16:41Z

app/controllers/api/v2/users_controller.rb

@@ -12,8 +12,8 @@ class UsersController < V2::BaseController
        ['compute_attributes']

      before_action :find_optional_nested_object
-      skip_before_action :authorize, :only => [:extlogin]
-      before_action :authenticate, :only => [:extlogin]
+      skip_before_action :authorize, :only => [:extlogin, :invalidate_jwt]


Why do you need to skip authorization for invalidate_jwt action?
You can specify the action in the security block:

foreman/config/initializers/f_foreman_permissions.rb

Line 566 in eb66c09

:"api/v2/users" => [:update]

This will make sure the current user has the edit_users permission before accessing the endpoint.

ShimShtein · 2024-10-28T09:18:13Z

app/controllers/users_controller.rb

@@ -8,7 +8,7 @@ class UsersController < ApplicationController
  rescue_from ActionController::InvalidAuthenticityToken, with: :login_token_reload
  skip_before_action :require_mail, :only => [:edit, :update, :logout, :stop_impersonation]
  skip_before_action :require_login, :check_user_enabled, :authorize, :session_expiry, :update_activity_time, :set_taxonomy, :set_gettext_locale_db, :only => [:login, :logout, :extlogout]
-  skip_before_action :authorize, :only => [:extlogin, :impersonate, :stop_impersonation]
+  skip_before_action :authorize, :only => [:extlogin, :impersonate, :stop_impersonation, :invalidate_jwt]


The same as for the API endpoint. You can specify the new action in the security block:

foreman/config/initializers/f_foreman_permissions.rb

Line 565 in eb66c09

:users => [:edit, :update],

MariaAga · 2024-10-28T16:54:03Z

Tested and does as needed, one small thing that acts weird to me is that a user cant invalidate their own jwt, so for example admin doesnt have the invalidate button, but a user with edit_users permission can invalidate admins tokens.

girijaasoni · 2024-10-29T05:39:14Z

Tested and does as needed, one small thing that acts weird to me is that a user cant invalidate their own jwt, so for example admin doesnt have the invalidate button, but a user with edit_users permission can invalidate admins tokens.

It's because there will be a different button in the user details page for this, from UI Perspective, for a user to invalidate tokens for themselves, they will have it in the profile page

MariaAga · 2024-10-29T12:38:24Z

@girijaasoni I think it looks a bit weird in the UI that other users have the invalidate button, but not the current (admin) user:
admin:

other user:

girijaasoni · 2024-10-29T13:52:04Z

@girijaasoni I think it looks a bit weird in the UI that other users have the invalidate button, but not the current (admin)
Some users don't have edit_users permissions and they shall still be able to invalidate tokens for themselves which is why it was suggested that we have the invalidate option in the user details page if the user wants to do it for self.

girijaasoni · 2024-11-20T08:57:19Z

Test failures seem unrelated

app/controllers/api/v2/users_controller.rb

stejskalleos · 2024-11-20T10:10:39Z

app/controllers/api/v2/users_controller.rb

@@ -111,6 +112,25 @@ def update
        end
      end

+      api :PATCH, "/users/:id/invalidate_jwt", N_("Invalidate JWT for a specific userS")
+      description <<-DOC
+        Invalidate JWT for a specific users


If the description is one-liner, then I would go with description 'Invalidate JWT for a specific users'

app/controllers/users_controller.rb

stejskalleos · 2024-11-20T13:17:12Z

app/controllers/users_controller.rb

+  def invalidate_jwt
+    @user = find_resource(:edit_users)
+    User.find_by(id: @user.id).jwt_secret&.destroy
+    if @user.jwt_secret.blank?


Do we have to have this if statement?
How can it happen that the jwt_secret IS NOT blank?

I mean just like a safety check

I mean just like a safety check

Yes, but is it really needed?
How the User.find_by(id: @user.id).jwt_secret&.destroy can result in @user.jwt_secret not being blank, that's what I'm asking.

stejskalleos · 2024-11-20T13:19:13Z

app/controllers/api/v2/users_controller.rb

+        if params[:search].blank?
+          process_resource_error
+        end


Suggested change

if params[:search].blank?

process_resource_error

end

process_resource_error if params[:search].blank?

stejskalleos · 2024-11-20T13:20:39Z

app/controllers/api/v2/users_controller.rb

+          render :json => { :error => _("No record found for '%s'") % params[:search]}
+        else
+          response = JwtSecret.where(user_id: @users).destroy_all
+          if response.blank?


Is the if response.blank? needed? In which cases is it not blank?

I mean it's optional, just like for safety. I'm open to remove it on vote? what do you think @ShimShtein

Well, I would like to see the reasoning behind the check.
I want to know which scenario the response is not blank in.

I don't think there would be a scenario as such.
I can remove the if condition

stejskalleos · 2024-11-20T13:21:43Z

Failing tests are fixed in #10377 and fog/fog-libvirt#163

Lennonka

My suggestions for the microcopy. It would be great to get the singulars and plurals right.

Lennonka · 2024-11-20T14:25:14Z

app/controllers/api/v2/users_controller.rb

@@ -111,6 +112,26 @@ def update
        end
      end

+      api :PATCH, "/users/:id/invalidate_jwt", N_("Invalidate JWT for a specific users")


Suggested change

api :PATCH, "/users/:id/invalidate_jwt", N_("Invalidate JWT for a specific users")

api :PATCH, "/users/:id/invalidate_jwt", N_("Invalidate all JWTs for the user")

We're invalidating all JWTs for a single user, right?

for API, we are doing it for multiple users

But the user is identified by an ID that must be unique for each user. So how is it for multiple users?

for API, we are using the scoped search approach, so the search params in the curl command would be something like {"seach" : "id ^ (1,2,3)"} translating to search for id in 1, 2 ,3.

Okay. In that case I suggest:

Suggested change

api :PATCH, "/users/:id/invalidate_jwt", N_("Invalidate JWT for a specific users")

api :PATCH, "/users/:id/invalidate_jwt", N_("Invalidate JWT for specific users")

If you use the search param to get users, then there should not be any :id in the URL.
api :PATCH, "/users/invalidate_jwt"

Lennonka · 2024-11-20T14:27:52Z

app/controllers/api/v2/users_controller.rb

@@ -111,6 +112,26 @@ def update
        end
      end

+      api :PATCH, "/users/:id/invalidate_jwt", N_("Invalidate JWT for a specific users")
+      description <<-DOC
+        Invalidate JWT for a specific users


Suggested change

Invalidate JWT for a specific users

Invalidate all JSON Web Tokens (JWTs) for a user or users.

The users you specify will no longer be able to register hosts by using their JWTs.

Lennonka · 2024-11-20T14:28:50Z

app/controllers/users_controller.rb

+    User.find_by(id: @user.id).jwt_secret&.destroy
+    if @user.jwt_secret.blank?
+      process_success(
+        :success_msg => _('Successfully invalidated JWT for %s.') % @user.login


Suggested change

:success_msg => _('Successfully invalidated JWT for %s.') % @user.login

:success_msg => _('Successfully invalidated JWTs for %s.') % @user.login

We're invalidating all of user's JWTs, right?

Lennonka · 2024-11-20T14:30:00Z

app/helpers/users_helper.rb

@@ -34,6 +34,13 @@ def user_action_buttons(user, additional_actions = [])
        :data => { :no_turbolink => true })
    end

+    if user != User.current
+      additional_actions << display_link_if_authorized(_("Invalidate JWT"),


Suggested change

additional_actions << display_link_if_authorized(_("Invalidate JWT"),

additional_actions << display_link_if_authorized(_("Invalidate JWTs"),

Lennonka · 2024-11-20T14:30:21Z

app/controllers/api/v2/users_controller.rb

+        else
+          response = JwtSecret.where(user_id: @users).destroy_all
+          if response.blank?
+            process_success _('Successfully invalidated JWT for %s.' % @users.pluck(:login).to_sentence)


Suggested change

process_success _('Successfully invalidated JWT for %s.' % @users.pluck(:login).to_sentence)

process_success _('Successfully invalidated JWTs for %s.' % @users.pluck(:login).to_sentence)

ShimShtein · 2024-11-25T13:51:39Z

app/controllers/api/v2/users_controller.rb

+
+      def invalidate_jwt
+        process_resource_error if params[:search].blank?
+        @users = resource_scope_for_index(:permission => :edit_users)


I think we need to add except_hidden scope to here too

ShimShtein · 2024-11-25T13:54:02Z

app/controllers/users_controller.rb

@@ -93,6 +93,14 @@ def impersonate
    end
  end

+  def invalidate_jwt
+    @user = find_resource(:edit_users)
+    User.find_by(id: @user.id).jwt_secret&.destroy


I still think that

Suggested change

User.find_by(id: @user.id).jwt_secret&.destroy

find_resource(:edit_users).jwt_secret&.destroy

will do the trick

Lennonka · 2024-11-26T22:26:06Z

app/helpers/users_helper.rb

+      additional_actions << display_link_if_authorized(_("Invalidate JWTs"),
+        hash_for_invalidate_jwt_user_path(:id => user.id).merge(:auth_object => user, :permission => "edit_users"),
+        :method => :patch, :id => user.id,
+        :data => { :confirm => _("Invalidate tokens for %s?") % user.name })


Suggested change

:data => { :confirm => _("Invalidate tokens for %s?") % user.name })

:data => { :confirm => _("Invalidate all JSON Web Tokens for %s?") % user.name })

I strongly suggest using "JWTs" here or spelling them out because "tokens" might get confused with Personal Access Tokens.

ekohl

Please add these to PersonalAccessTokensController, not the users controller. That's already where we have all the resources to manage them.

https://restful-api-design.readthedocs.io/en/latest/ is an old document, but it's where I learned RESTful API design. Please read https://restful-api-design.readthedocs.io/en/latest/urls.html#url-structure to understand how to think about collections and resources. https://restful-api-design.readthedocs.io/en/latest/methods.html is another on the methods to use. It doesn't explicitly mention DELETE on a (sub)collection, but that doesn't mean it can't be done. I'd argue it's well defined.

ekohl · 2024-11-27T12:39:38Z

app/controllers/api/v2/users_controller.rb

@@ -111,6 +112,22 @@ def update
        end
      end

+      api :PATCH, "/users/invalidate_jwt", N_("Invalidate all JSON Web Tokens (JWTs) for a user or users.")


This now looks like it invalidates it for all users and there's no way to specify it for users. REST API wise I think this is also poor design. If you have a user that's called invalidate_jwt then it overlaps with other resources.

I'd expect DELETE /users/:id/jwt to delete all JWTs for a specific user while for all JWTs it should operate on the JWT collection, so something like DELETE /jwt. We currently don't have such an endpoint so you'd need to create that.

I think a user with invalidate_jwt would be an edge case
we are using scoped search approach in the API request so the user can use id or name or login anything for reference. From API POV, its valid for one specific user or multiple users as well. Request params would go like "{"search" : "id ^ (1, 2, 3)" } , does it make sense?

cc @ShimShtein

ekohl · 2024-11-27T12:44:07Z

app/controllers/api/v2/users_controller.rb

@@ -96,7 +97,7 @@ def create
      api :PUT, "/users/:id/", N_("Update a user")
      description <<-DOC
        Adds role 'Default role' to the user if it is not already present.
-        Only another admin can change the admin account attribute.
+      Only another admin can change the admin account attribute.


Please keep correct indenting.

Suggested change

Only another admin can change the admin account attribute.

Only another admin can change the admin account attribute.

ekohl · 2024-11-27T13:02:06Z

config/initializers/f_foreman_permissions.rb

+      :users => [:edit, :update, :invalidate_jwt],
+      :"api/v2/users" => [:update, :invalidate_jwt]


Where does invalidate_jwt come from? I don't see it defined in PermissionsList

it's an action in userscontrollers, do we need to add it in the permissions list?

girijaasoni · 2024-11-28T14:15:16Z

As per further discussion, we have decided to split the UI and the API PR. @ekohl suggests we add a separate controller for JWTs. It will be updated in the next PR
CC: @ShimShtein

pr-processor bot added the Not yet reviewed label Oct 21, 2024

ares reviewed Oct 22, 2024

View reviewed changes

pr-processor bot removed the Not yet reviewed label Oct 22, 2024

girijaasoni force-pushed the 37936 branch 4 times, most recently from 37cd91b to eeb34c3 Compare October 24, 2024 11:39

ShimShtein reviewed Oct 28, 2024

View reviewed changes

girijaasoni force-pushed the 37936 branch 3 times, most recently from bf39e67 to 7e62bf0 Compare November 20, 2024 08:39

girijaasoni force-pushed the 37936 branch from 7e62bf0 to 23180c8 Compare November 20, 2024 11:08

stejskalleos requested changes Nov 20, 2024

View reviewed changes

pr-processor bot added the Waiting on contributor label Nov 20, 2024

Lennonka suggested changes Nov 20, 2024

View reviewed changes

girijaasoni force-pushed the 37936 branch from 23180c8 to 4ad8a04 Compare November 25, 2024 12:53

pr-processor bot added Needs re-review and removed Waiting on contributor labels Nov 25, 2024

ShimShtein requested changes Nov 25, 2024

View reviewed changes

pr-processor bot added Waiting on contributor and removed Needs re-review labels Nov 25, 2024

girijaasoni force-pushed the 37936 branch from 4ad8a04 to b32f655 Compare November 26, 2024 13:04

pr-processor bot added Needs re-review and removed Waiting on contributor labels Nov 26, 2024

girijaasoni force-pushed the 37936 branch from b32f655 to b01969a Compare November 26, 2024 13:14

Lennonka reviewed Nov 26, 2024

View reviewed changes

pr-processor bot removed the Needs re-review label Nov 26, 2024

girijaasoni force-pushed the 37936 branch from b01969a to 74741ab Compare November 27, 2024 11:26

ekohl requested changes Nov 27, 2024

View reviewed changes

pr-processor bot added the Waiting on contributor label Nov 27, 2024

ekohl reviewed Nov 27, 2024

View reviewed changes

girijaasoni force-pushed the 37936 branch from 74741ab to 17f493d Compare November 28, 2024 14:13

pr-processor bot added Needs re-review and removed Waiting on contributor labels Nov 28, 2024

girijaasoni changed the title ~~Fixes #37936 - As a user, I want to invalidate jwt for specific user~~ Fixes #37936 - As a user, I want to invalidate jwt for specific user( UI ) Nov 28, 2024

Fixes #37936 - As a user, I want to invalidate jwt for specific user(UI)

05f6569

girijaasoni force-pushed the 37936 branch from 17f493d to 05f6569 Compare November 28, 2024 14:28

	@user = User.find(params[:id])
	@user = User.where(id: params[:id]).authorized(:edit_user)

	api :PATCH, "/users/:id/invalidate_jwt", N_("Invalidate JWT for a specific users")
	api :PATCH, "/users/:id/invalidate_jwt", N_("Invalidate all JWTs for the user")

	Invalidate JWT for a specific users
	Invalidate all JSON Web Tokens (JWTs) for a user or users.
	The users you specify will no longer be able to register hosts by using their JWTs.

	:success_msg => _('Successfully invalidated JWT for %s.') % @user.login
	:success_msg => _('Successfully invalidated JWTs for %s.') % @user.login

	additional_actions << display_link_if_authorized(_("Invalidate JWT"),
	additional_actions << display_link_if_authorized(_("Invalidate JWTs"),

	process_success _('Successfully invalidated JWT for %s.' % @users.pluck(:login).to_sentence)
	process_success _('Successfully invalidated JWTs for %s.' % @users.pluck(:login).to_sentence)

	User.find_by(id: @user.id).jwt_secret&.destroy
	find_resource(:edit_users).jwt_secret&.destroy

	:data => { :confirm => _("Invalidate tokens for %s?") % user.name })
	:data => { :confirm => _("Invalidate all JSON Web Tokens for %s?") % user.name })

	Only another admin can change the admin account attribute.
	Only another admin can change the admin account attribute.

		:users => [:edit, :update, :invalidate_jwt],
		:"api/v2/users" => [:update, :invalidate_jwt]

Fixes #37936 - As a user, I want to invalidate jwt for specific user( UI ) #10357

Are you sure you want to change the base?

Fixes #37936 - As a user, I want to invalidate jwt for specific user( UI ) #10357

Conversation

girijaasoni commented Oct 21, 2024 • edited Loading

ares left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

girijaasoni Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

girijaasoni Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

girijaasoni Oct 29, 2024 • edited Loading

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

MariaAga commented Oct 28, 2024

girijaasoni commented Oct 29, 2024 • edited Loading

MariaAga commented Oct 29, 2024

girijaasoni commented Oct 29, 2024 • edited Loading

girijaasoni commented Nov 20, 2024

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

stejskalleos commented Nov 20, 2024

Lennonka left a 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

Lennonka Nov 20, 2024 • edited Loading

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

Lennonka Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

ekohl left a 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

girijaasoni commented Nov 28, 2024

girijaasoni commented Oct 21, 2024 •

edited

Loading

girijaasoni Oct 22, 2024 •

edited

Loading

girijaasoni Oct 24, 2024 •

edited

Loading

girijaasoni Oct 29, 2024 •

edited

Loading

girijaasoni commented Oct 29, 2024 •

edited

Loading

girijaasoni commented Oct 29, 2024 •

edited

Loading

Lennonka Nov 20, 2024 •

edited

Loading

Lennonka Nov 26, 2024 •

edited

Loading