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

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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
23 changes: 20 additions & 3 deletions app/controllers/api/v2/users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Copy link
Member

Choose a reason for hiding this comment

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

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

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

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

before_action :authenticate, :only => [:extlogin, :invalidate_jwt]

api :GET, "/users/", N_("List all users")
api :GET, "/auth_source_ldaps/:auth_source_ldap_id/users", N_("List all users for LDAP authentication source")
Expand Down Expand Up @@ -96,7 +96,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.
DOC
param :id, String, :required => true
param_group :user_update
Expand All @@ -111,6 +111,23 @@ def update
end
end

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

Choose a reason for hiding this comment

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

Copy-paste leftovers?

Copy link
Contributor Author

@girijaasoni girijaasoni Oct 29, 2024

Choose a reason for hiding this comment

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

yes, my bad

param :id, String, :required => true
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

description <<-DOC
Invalidate JWT for a specific user
DOC

def invalidate_jwt
@user = User.find(params[:id])
Copy link
Member

Choose a reason for hiding this comment

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

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

if !@user
render :json => { :error => _("User %s does not exist.") % @user.login}
elsif !User.current.can?(:edit_users, @user)
deny_access N_("User %s does not have permissions to invalidate JWT." % User.current.login)
elsif User.current.can?(:edit_users, @user) && @user.invalidate_jwt.blank?
process_success _('Successfully invalidated JWT for %s.' % @user.login)
end
end

api :DELETE, "/users/:id/", N_("Delete a user")
param :id, String, :required => true

Expand Down
11 changes: 10 additions & 1 deletion app/controllers/users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Copy link
Member

Choose a reason for hiding this comment

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

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

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

before_action :require_admin, :only => :impersonate
after_action :update_activity_time, :only => :login
before_action :verify_active_session, :only => :login
Expand Down Expand Up @@ -93,6 +93,15 @@ def impersonate
end
end

def invalidate_jwt
@user = find_resource(:edit_users)
if @user.invalidate_jwt.blank?
process_success(
:success_msg => _('Successfully invalidated JWT for %s.') % @user.login
)
end
end

def stop_impersonation
if session[:impersonated_by].present?
user = User.unscoped.find_by_id(session[:impersonated_by])
Expand Down
7 changes: 7 additions & 0 deletions app/helpers/users_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
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 })
end

delete_btn = display_delete_if_authorized(
hash_for_user_path(:id => user).merge(:auth_object => user, :authorizer => authorizer),
:data => { :confirm => _("Delete %s?") % user.name })
Expand Down
4 changes: 4 additions & 0 deletions app/models/concerns/jwt_auth.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ def jwt_secret!
jwt_secret || create_jwt_secret!
end

def invalidate_jwt
User.find_by(id: id).jwt_secret&.destroy
end

# expiration: integer, eg: 4.hours.to_i
# scope: example: [{ controller: :registration, actions: [:global, :host] }]
def jwt_token!(expiration: nil, scope: [])
Expand Down
1 change: 1 addition & 0 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,7 @@
end
member do
post 'impersonate'
patch 'invalidate_jwt'
end
resources :ssh_keys, only: [:new, :create, :destroy]
end
Expand Down
1 change: 1 addition & 0 deletions config/routes/api/v2.rb
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@
resources :mail_notifications, :only => [:create, :destroy, :update]
get 'mail_notifications', :to => 'mail_notifications#user_mail_notifications', :on => :member
get 'extlogin', :to => 'users#extlogin', :on => :collection
patch 'invalidate_jwt', :to => 'users#invalidate_jwt', :on => :member
end
end

Expand Down
Loading