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

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

Please feel free to review the code and test the PR while I continue writing the tests!
WIP: currently writing tests

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  [email protected]?(: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.

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 3 times, most recently from 47ec499 to 37cd91b Compare October 24, 2024 11:35

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

eeb34c3

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

ShimShtein reviewed Oct 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

girijaasoni commented Oct 21, 2024 •

edited

Loading

ares left a comment

ares Oct 22, 2024

girijaasoni Oct 22, 2024 •

edited

Loading

ares Oct 22, 2024

girijaasoni Oct 24, 2024 •

edited

Loading

ShimShtein Oct 28, 2024

girijaasoni Oct 29, 2024 •

edited

Loading

ShimShtein Oct 28, 2024

girijaasoni Oct 29, 2024

ShimShtein Oct 28, 2024

ShimShtein Oct 28, 2024

ShimShtein Oct 28, 2024

MariaAga commented Oct 28, 2024

girijaasoni commented Oct 29, 2024 •

edited

Loading

MariaAga commented Oct 29, 2024

girijaasoni commented Oct 29, 2024 •

edited

Loading

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

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

Are you sure you want to change the base?

Fixes #37936 - As a user, I want to invalidate jwt for specific user #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 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