Fixes #37936 - Invalidate jwt for any user or users(API) #10397

girijaasoni · 2024-12-04T09:26:12Z

Link to UI PR: #10357

ShimShtein · 2024-12-09T13:51:41Z

app/controllers/api/v2/registration_tokens_controller.rb

+        end
+      end
+
+      api :DELETE, '/users/:id/registration_tokens', N_("Invalidate all JSON Web Tokens (JWTs) for a specific user.")


Suggested change

api :DELETE, '/users/:id/registration_tokens', N_("Invalidate all JSON Web Tokens (JWTs) for a specific user.")

api :DELETE, '/users/:id/registration_tokens', N_("Invalidate all registration tokens for a specific user.")

ShimShtein · 2024-12-09T14:02:47Z

test/controllers/api/v2/registration_tokens_controller_test.rb

+
+class Api::V2::RegistrationTokensControllerTest < ActionController::TestCase
+  test 'user shall invalidate tokens for self' do
+    user = User.create :login => "foo", :mail => "foo@bar.com", :auth_source => auth_sources(:one)


You can use FactoryBot.create(:user) here.

ShimShtein · 2024-12-11T09:42:17Z

test/controllers/api/v2/registration_tokens_controller_test.rb

+class Api::V2::RegistrationTokensControllerTest < ActionController::TestCase
+  test 'user shall invalidate tokens for self' do
+    user = User.create :login => "foo", :mail => "foo@bar.com", :auth_source => auth_sources(:one)
+    FactoryBot.build(:jwt_secret, token: 'test_jwt_secret', user: user)


You probably want to create the token as part of the user creation, and use the create method - to make sure the token is saved before the action.

ShimShtein · 2024-12-11T09:44:16Z

app/controllers/api/v2/registration_tokens_controller.rb

+      end
+
+      def find_resource(permission = :view_users)
+        editing_self? ? User.find(User.current.id) : User.authorized(permission).except_hidden.find(params[:id])


@ekohl what do you think, should we block changes to hidden accounts here?

I was also wondering about that.

test/controllers/api/v2/registration_tokens_controller_test.rb

app/controllers/api/v2/registration_tokens_controller.rb

nofaralfasi · 2024-12-19T14:16:40Z

@girijaasoni In the UI PR, we used the term JWT token for the phrasing. Why are we referring to it as a registration token here? Wouldn't that create confusion?

girijaasoni · 2024-12-19T14:38:50Z

@girijaasoni In the UI PR, we used the term JWT token for the phrasing. Why are we referring to it as a registration token here? Wouldn't that create confusion?

@nofaralfasi , we can handle that in the documentation. Registration tokens make more sense from user POV(specially from API POV), as we are creating a new controller. For UI, we are trying to use the existing controller where jwt is used as phrasing.

nofaralfasi · 2024-12-19T15:31:01Z

app/controllers/api/v2/registration_tokens_controller.rb

+      end
+
+      api :DELETE, "/registration_tokens", N_("Invalidate all JSON Web Tokens (JWTs) for multiple users.")
+      param :search, String, :required => true


@girijaasoni could you please share an example for this query? What does the search param should contain?

curl --user test-user1:changeme -X DELETE http://localhost:3000/api/registration_tokens/remove_multiple?search=id%20%5E%20%281%2C2%2C3%29 -H 'Content-Type: application/json'

this is the command I'm using for a scoped search query id ^ (1, 2, 3) you will need to URL encode in the search params as curl does not accept brackets and operators.

Where does the remove_multiple come from? And why not to pass an array instead?

my bad, I meant curl --user test-user1:changeme -X DELETE http://localhost:3000/api/registration_tokens?search=id%20%5E%20%281%2C2%2C3%29 -H 'Content-Type: application/json we are using scoped search for this here so we can search with id or name or whatever. If you don't understand this we can sync up

I understand how search_for works, but it still seems a bit unintuitive to me that the user needs to write queries in this format. I would have expected a simpler approach. At least, that's my perspective.

I must agree with @nofaralfasi here, for bulk actions that already exist (mostly in Katello), we provide ids param of the entities that are about to be changed/deleted.

search param is used mostly in index actions. Ideal approach is to use index action with search and then use the result in the second "destructive" call.

Although, I personally don't like REST or our? REST because of that (multiple calls instead of one), but at least we're consistent.

@shweta83 and @ShimShtein, since you both approved, WDYT about ^ (#10397 (comment))? Did I miss something?

IDs are mostly hidden value, and definitely not a user-friendly one. I would go with the search approach, from what I know, people are using naming schemes for their users, so performing an action on a group would become much easier if search is implemented. I don't see a reason to "punish" the user that wants to perform such an action with a second call (index with search condition -> invalidation with ids).
We can add an ids parameter as a "syntactic sugar" here, but I don't think it's really needed in this iteration.

Okay, I won't block on that then.

It just seems weird we didn't do the same before for other endpoints.

shweta83 · 2024-12-24T18:15:39Z

app/controllers/api/v2/registration_tokens_controller.rb

+          raise ::Foreman::Exception.new(N_("No record found for %s"), params[:id])
+        end
+        @user.jwt_secret&.destroy
+        process_success _('Successfully invalidated JWTs for %s.' % @user.login)


@girijaasoni Can we add a newline here as well?

shweta83 · 2024-12-24T18:19:27Z

@girijaasoni invalidate token for other users doesn't work for invalidating self token. Can we fix that too?

nofaralfasi · 2024-12-25T13:58:24Z

app/controllers/api/v2/registration_tokens_controller.rb

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


I find the naming a bit inconsistent between the invalidate_jwt method and this one, invalidate_tokens. Would it make sense to align their names for clarity?

ShimShtein · 2024-12-25T14:30:04Z

app/controllers/api/v2/registration_tokens_controller.rb

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


Suggested change

def invalidate_tokens

def invalidate_jwt_tokens

To be a bit more consistent with the naming.

ShimShtein · 2024-12-25T14:30:59Z

app/controllers/api/v2/registration_tokens_controller.rb

+        end
+      end
+
+      api :DELETE, '/users/:id/registration_tokens', N_("Invalidate all registration Tokens for a specific user.")


Suggested change

api :DELETE, '/users/:id/registration_tokens', N_("Invalidate all registration Tokens for a specific user.")

api :DELETE, '/users/:id/registration_tokens', N_("Invalidate all registration tokens for a specific user.")

app/controllers/api/v2/registration_tokens_controller.rb

ShimShtein · 2024-12-25T14:37:46Z

app/controllers/api/v2/registration_tokens_controller.rb

+        process_success _('Successfully invalidated JWTs for %s.' % @user.login)
+      end
+
+      api :DELETE, "/registration_tokens", N_("Invalidate all JSON Web Tokens (JWTs) for multiple users.")


Suggested change

api :DELETE, "/registration_tokens", N_("Invalidate all JSON Web Tokens (JWTs) for multiple users.")

api :DELETE, "/registration_tokens", N_("Invalidate all registration tokens for multiple users.")

ShimShtein · 2024-12-25T14:38:16Z

app/controllers/api/v2/registration_tokens_controller.rb

+      DOC
+
+      def invalidate_tokens
+        raise ::Foreman::Exception.new(N_("Please provide search parameters")) if params[:search].blank?


Suggested change

raise ::Foreman::Exception.new(N_("Please provide search parameters")) if params[:search].blank?

raise ::Foreman::Exception.new(N_("Please provide search parameter")) if params[:search].blank?

ShimShtein · 2024-12-25T14:39:36Z

app/controllers/api/v2/registration_tokens_controller.rb

+          raise ::Foreman::Exception.new(N_("No record found %s"), params[:search])
+        end
+        JwtSecret.where(user_id: @users).destroy_all
+        process_success _("Successfully invalidated JWTs for %s.\n" % @users.pluck(:login).to_sentence)


Suggested change

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

process_success _("Successfully invalidated registration tokens for %s.\n" % @users.pluck(:login).to_sentence)

test/controllers/api/v2/registration_tokens_controller_test.rb

app/controllers/api/v2/registration_tokens_controller.rb

nofaralfasi · 2024-12-30T14:04:01Z

app/controllers/api/v2/registration_tokens_controller.rb

+        process_success _("Successfully invalidated registration tokens for %s.\n" % @user.login)
+      end
+
+      api :DELETE, "/registration_tokens", N_("Invalidate all registration tokens for multiple users.")


Why didn't we use /users here?

According to REST principles, you should state what resource you are acting upon. If you wan to use nested resources (as in users/:id/registration_token) that would work only for a specific user. There is no good way to specify a sub-object of multiple parent objects. So since registration token is an object by itself, we are addressing multiple registration token objects by a search condition.

shweta83

LGTM

girijaasoni · 2025-01-06T17:10:19Z

app/controllers/api/v2/registration_tokens_controller.rb

+      end
+
+      api :DELETE, "/registration_tokens", N_("Invalidate all registration tokens for multiple users.")
+      param :search, String, :required => true


@Lennonka , could you please provide me with a description for the search parameters that I could use.

URL-encoded search query that selects users for which registration tokens will be invalidated. Search query example: id ^ (2, 4, 6)

Lennonka · 2025-01-06T17:49:35Z

Too bad we didn't use the same terminology in the UI in #10357. Is it too late to change it?

cc @girijaasoni @ShimShtein

girijaasoni · 2025-01-06T17:52:41Z

Too bad we didn't use the same terminology in the UI in #10357. Is it too late to change it?

cc @girijaasoni @ShimShtein

It is not required in UI, we are using the correct terminology there, this is specific to API

nofaralfasi · 2025-01-09T07:53:10Z

Thanks everyone!

pr-processor bot added the Not yet reviewed label Dec 4, 2024

girijaasoni mentioned this pull request Dec 4, 2024

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

Merged

girijaasoni force-pushed the jwt-api branch from ab2f671 to 6785cb3 Compare December 4, 2024 09:29

girijaasoni changed the title ~~Fixes #37936 - As a user, I want to invalidate jwt for specific user(…~~ Fixes #37936 - Invalidate jwt for any user or users(API) Dec 4, 2024

stejskalleos requested review from ekohl and ShimShtein December 6, 2024 08:14

ShimShtein reviewed Dec 9, 2024

View reviewed changes

pr-processor bot removed the Not yet reviewed label Dec 9, 2024

girijaasoni force-pushed the jwt-api branch from 6785cb3 to 201df8f Compare December 11, 2024 09:02

pr-processor bot added the Waiting on contributor label Dec 11, 2024

girijaasoni force-pushed the jwt-api branch from 201df8f to 0552336 Compare December 11, 2024 09:03

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

girijaasoni force-pushed the jwt-api branch 2 times, most recently from 28cdc1b to 85928e7 Compare December 11, 2024 09:15

ShimShtein reviewed Dec 11, 2024

View reviewed changes

pr-processor bot removed the Needs re-review label Dec 11, 2024

ShimShtein reviewed Dec 11, 2024

View reviewed changes

girijaasoni force-pushed the jwt-api branch from 85928e7 to 87ed708 Compare December 12, 2024 10:54

nofaralfasi reviewed Dec 16, 2024

View reviewed changes

test/controllers/api/v2/registration_tokens_controller_test.rb Outdated Show resolved Hide resolved

app/controllers/api/v2/registration_tokens_controller.rb Show resolved Hide resolved

app/controllers/api/v2/registration_tokens_controller.rb Outdated Show resolved Hide resolved

nofaralfasi reviewed Dec 19, 2024

View reviewed changes

girijaasoni force-pushed the jwt-api branch 7 times, most recently from 8858b6c to 6d841de Compare December 24, 2024 13:45

girijaasoni marked this pull request as draft December 24, 2024 14:14

girijaasoni force-pushed the jwt-api branch from 6d841de to d248221 Compare December 24, 2024 14:23

girijaasoni marked this pull request as ready for review December 24, 2024 14:24

girijaasoni force-pushed the jwt-api branch from d248221 to a2b09f5 Compare December 24, 2024 14:27

shweta83 reviewed Dec 24, 2024

View reviewed changes

nofaralfasi reviewed Dec 25, 2024

View reviewed changes

ShimShtein reviewed Dec 25, 2024

View reviewed changes

girijaasoni force-pushed the jwt-api branch from a2b09f5 to 2a7bb00 Compare December 26, 2024 07:06

shweta83 reviewed Dec 26, 2024

View reviewed changes

app/controllers/api/v2/registration_tokens_controller.rb Show resolved Hide resolved

girijaasoni force-pushed the jwt-api branch from 2a7bb00 to 020f9e4 Compare December 26, 2024 11:10

nofaralfasi reviewed Dec 30, 2024

View reviewed changes

ShimShtein previously approved these changes Jan 5, 2025

View reviewed changes

shweta83 approved these changes Jan 5, 2025

View reviewed changes

girijaasoni commented Jan 6, 2025

View reviewed changes

Fixes #37936 - Invalidate jwt for any user or users(API)

8f69cb2

girijaasoni dismissed ShimShtein’s stale review via 8f69cb2 January 7, 2025 09:33

girijaasoni force-pushed the jwt-api branch from 020f9e4 to 8f69cb2 Compare January 7, 2025 09:33

nofaralfasi approved these changes Jan 8, 2025

View reviewed changes

nofaralfasi merged commit 29b3843 into theforeman:develop Jan 9, 2025
51 checks passed

shweta83 mentioned this pull request Jan 13, 2025

[WIP]Add support for Registration Tokens SatelliteQE/nailgun#1260

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #37936 - Invalidate jwt for any user or users(API) #10397

Fixes #37936 - Invalidate jwt for any user or users(API) #10397

girijaasoni commented Dec 4, 2024 •

edited

Loading

ShimShtein Dec 9, 2024

ShimShtein Dec 9, 2024

ShimShtein Dec 11, 2024

ShimShtein Dec 11, 2024

nofaralfasi Dec 25, 2024

nofaralfasi commented Dec 19, 2024

girijaasoni commented Dec 19, 2024 •

edited

Loading

nofaralfasi Dec 19, 2024

girijaasoni Dec 20, 2024 •

edited

Loading

nofaralfasi Dec 23, 2024

girijaasoni Dec 24, 2024 •

edited

Loading

nofaralfasi Dec 24, 2024

ofedoren Jan 6, 2025

ofedoren Jan 6, 2025

ShimShtein Jan 6, 2025

ofedoren Jan 6, 2025

shweta83 Dec 24, 2024

shweta83 commented Dec 24, 2024

nofaralfasi Dec 25, 2024

ShimShtein Dec 25, 2024

ShimShtein Dec 25, 2024

ShimShtein Dec 25, 2024

ShimShtein Dec 25, 2024

ShimShtein Dec 25, 2024

nofaralfasi Dec 30, 2024

ShimShtein Jan 5, 2025

shweta83 left a comment

girijaasoni Jan 6, 2025

Lennonka Jan 6, 2025

Lennonka commented Jan 6, 2025

girijaasoni commented Jan 6, 2025

nofaralfasi commented Jan 9, 2025

	api :DELETE, '/users/:id/registration_tokens', N_("Invalidate all JSON Web Tokens (JWTs) for a specific user.")
	api :DELETE, '/users/:id/registration_tokens', N_("Invalidate all registration tokens for a specific user.")

	api :DELETE, "/registration_tokens", N_("Invalidate all JSON Web Tokens (JWTs) for multiple users.")
	api :DELETE, "/registration_tokens", N_("Invalidate all registration tokens for multiple users.")

	raise ::Foreman::Exception.new(N_("Please provide search parameters")) if params[:search].blank?
	raise ::Foreman::Exception.new(N_("Please provide search parameter")) if params[:search].blank?

	process_success _("Successfully invalidated JWTs for %s.\n" % @users.pluck(:login).to_sentence)
	process_success _("Successfully invalidated registration tokens for %s.\n" % @users.pluck(:login).to_sentence)

Fixes #37936 - Invalidate jwt for any user or users(API) #10397

Fixes #37936 - Invalidate jwt for any user or users(API) #10397

Conversation

girijaasoni commented Dec 4, 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

nofaralfasi commented Dec 19, 2024

girijaasoni commented Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

girijaasoni Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

girijaasoni Dec 24, 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

Choose a reason for hiding this comment

shweta83 commented Dec 24, 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

shweta83 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lennonka commented Jan 6, 2025

girijaasoni commented Jan 6, 2025

nofaralfasi commented Jan 9, 2025

girijaasoni commented Dec 4, 2024 •

edited

Loading

girijaasoni commented Dec 19, 2024 •

edited

Loading

girijaasoni Dec 20, 2024 •

edited

Loading

girijaasoni Dec 24, 2024 •

edited

Loading