Fixes #38124 - Invalidate tokens for specific user/users #636

girijaasoni · 2025-01-06T17:38:43Z

No description provided.

nofaralfasi

The failing tests appear to be related to your changes. Could you please split this PR into two commits? One for the relevant changes and another for the formatting updates. This will make it easier to review and distinguish between the two.

girijaasoni · 2025-01-07T11:51:03Z

The failing tests appear to be related to your changes. Could you please split this PR into two commits? One for the relevant changes and another for the formatting updates. This will make it easier to review and distinguish between the two.
Thanks @nofaralfasi , I have updated it accordingly

ofedoren

Thanks, @girijaasoni, overall LGTM, but there are some changes needed, please see inline comments.

Also, it would be nice to have functional tests for this. It will probably require you to generate new test/data/3.14 from Foreman with the API changes, the instructions on how to do so are here: https://github.com/theforeman/hammer-cli-foreman/tree/master/test/data . Please, ignore the first 2 steps.

ofedoren · 2025-01-07T12:32:10Z

lib/hammer_cli_foreman.rb

@@ -32,144 +31,114 @@ def self.exception_handler_class
    require 'hammer_cli_foreman/common_parameter'
    require 'hammer_cli_foreman/defaults'

-    HammerCLI::MainCommand.lazy_subcommand('auth', _("Foreman connection login/logout"),
-      'HammerCLIForeman::Auth', 'hammer_cli_foreman/auth'
-    )


I know these come from autofix done by VSCode rubocop, but could you please either remove those changes or extract them into a separate commit?

This applies to the changes in user.rb, which are not directly required, as well.

ofedoren · 2025-01-07T12:35:35Z

lib/hammer_cli_foreman.rb

-                                            'HammerCLIForeman::Registration', 'hammer_cli_foreman/registration')
-  rescue => e
-    handler = HammerCLIForeman::ExceptionHandler.new(:context => {}, :adapter => :base)
+    HammerCLI::MainCommand.lazy_subcommand('registration-tokens', _('Manipulate registration tokens'),


Since it's heavily tightened to the users, we don't need this as a global/main command. It should be enough to add this as a subcommand to the users command.

we need a main command as we have another endpoint which is not tightened to the users.
api its like api/registration_tokens?search="query" and one which is specific to users that goes like
api/users/:id/registration_tokens

we need a main command as we have another endpoint which is not tightened to the users. api its like api/registration_tokens?search="query"

But logically it's still users-only-related. Even --search option will suggest that it will be used for searching for the users. It's the same as with PATs or SSH keys.

@ShimShtein , what do you think about this?

ofedoren · 2025-01-07T12:44:22Z

lib/hammer_cli_foreman/registration_tokens.rb

+      success_message _('Successfully invalidated registration tokens.')
+      build_options


Suggested change

success_message _('Successfully invalidated registration tokens.')

build_options

success_message _('Successfully invalidated registration tokens.')

build_options

Is there a reason for this one? Just curious

Just mine OCD, can be ignored.

ofedoren · 2025-01-07T12:44:39Z

lib/hammer_cli_foreman/registration_tokens.rb

+
+    class InvalidateMultipleCommand < HammerCLIForeman::SingleResourceCommand
+      action :invalidate_jwt_tokens
+      command_name 'invalidate_multiple'


Suggested change

command_name 'invalidate_multiple'

command_name 'invalidate-multiple'

ofedoren · 2025-01-07T12:51:11Z

lib/hammer_cli_foreman/registration_tokens.rb

+    command_name 'registration-tokens'
+    desc _('Manage registration tokens')
+
+    class InvalidateMultipleCommand < HammerCLIForeman::SingleResourceCommand


Suggested change

class InvalidateMultipleCommand < HammerCLIForeman::SingleResourceCommand

class InvalidateMultipleCommand < HammerCLIForeman::DeleteCommand

ofedoren · 2025-01-07T12:51:24Z

lib/hammer_cli_foreman/registration_tokens.rb

+      build_options
+    end
+
+    class InvalidateCommand < HammerCLIForeman::SingleResourceCommand


Suggested change

class InvalidateCommand < HammerCLIForeman::SingleResourceCommand

class InvalidateCommand < HammerCLIForeman::DeleteCommand

girijaasoni · 2025-01-17T11:11:36Z

lib/hammer_cli_foreman/registration_tokens.rb

+    class InvalidateCommand < HammerCLIForeman::DeleteCommand
+      action :invalidate_jwt
+      command_name 'invalidate'
+      success_message _('Successfully invalidated registration tokens for %{user}.')


DEPENDS ON REF
We need to merge that before we merge this

ofedoren · 2025-01-17T12:16:53Z

I'd still like to see some tests before merging: #636 (review)

We can update this PR later, it will anyway wait for the Foreman's one.

ofedoren

Thanks, @girijaasoni, LGTM, just one small thing:

ofedoren · 2025-01-22T13:53:32Z

lib/hammer_cli_foreman/registration_tokens.rb

+    class InvalidateMultipleCommand < HammerCLIForeman::DeleteCommand
+      action :invalidate_jwt_tokens
+      command_name 'invalidate-multiple'
+      success_message _('Successfully invalidated registration tokens for %{users}.')


failure_message is missing for both commands.

We don't need a failure message because it will not fail in any case. We are handling other cases like missing params or no record found or permissions etc. We don't have a failure message for API as well. It can fail in UI but not in API.

missing params or no record found or permissions etc

Those are error states and API returns related messages, e.g. Missing permissions.

It can fail in UI but not in API.

If I call the API with the wrong user id, it fails.

We don't need a failure message

Yes, we do. It's expected way to show why the command failed, please see other commands.

girijaasoni · 2025-01-23T13:18:32Z

Tests to be written in a follow up PR

ofedoren

Thanks, @girijaasoni ! Let's get this in.

pr-processor bot added the Not yet reviewed label Jan 6, 2025

girijaasoni force-pushed the 38124 branch from 9333aff to 1c01cfc Compare January 6, 2025 17:39

girijaasoni changed the title ~~Fixes #38124 - As a user, I want ton invalidate tokens for specific u…~~ Fixes #38124 - Invalidate tokens for specific user/users Jan 6, 2025

girijaasoni force-pushed the 38124 branch from 1c01cfc to 2e41f4d Compare January 6, 2025 17:45

nofaralfasi reviewed Jan 7, 2025

View reviewed changes

pr-processor bot removed the Not yet reviewed label Jan 7, 2025

ofedoren requested changes Jan 7, 2025

View reviewed changes

pr-processor bot added the Waiting on contributor label Jan 7, 2025

girijaasoni force-pushed the 38124 branch from 458cc91 to 5316605 Compare January 9, 2025 11:27

pr-processor bot added Needs re-review and removed Waiting on contributor labels Jan 9, 2025

girijaasoni force-pushed the 38124 branch from 5316605 to 23d38f5 Compare January 9, 2025 11:28

pr-processor bot removed the Needs re-review label Jan 14, 2025

girijaasoni force-pushed the 38124 branch from 23d38f5 to 92336e0 Compare January 17, 2025 11:02

girijaasoni commented Jan 17, 2025

View reviewed changes

ofedoren requested changes Jan 22, 2025

View reviewed changes

pr-processor bot added the Waiting on contributor label Jan 22, 2025

Fixes #38124 - Invalidate tokens for specific user/users

1d6dcde

girijaasoni force-pushed the 38124 branch from 92336e0 to 1d6dcde Compare January 23, 2025 13:17

pr-processor bot added Needs re-review and removed Waiting on contributor labels Jan 23, 2025

ofedoren approved these changes Jan 23, 2025

View reviewed changes

pr-processor bot removed the Needs re-review label Jan 23, 2025

ofedoren merged commit fcea924 into theforeman:master Jan 23, 2025
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #38124 - Invalidate tokens for specific user/users #636

Fixes #38124 - Invalidate tokens for specific user/users #636

girijaasoni commented Jan 6, 2025 •

edited

Loading

nofaralfasi left a comment

girijaasoni commented Jan 7, 2025

ofedoren left a comment

ofedoren Jan 7, 2025

ofedoren Jan 7, 2025

girijaasoni Jan 9, 2025

ofedoren Jan 14, 2025

girijaasoni Jan 15, 2025

ofedoren Jan 7, 2025

girijaasoni Jan 9, 2025

ofedoren Jan 14, 2025

ofedoren Jan 7, 2025

ofedoren Jan 7, 2025

ofedoren Jan 7, 2025

girijaasoni Jan 17, 2025

ofedoren commented Jan 17, 2025

ofedoren left a comment

ofedoren Jan 22, 2025

girijaasoni Jan 23, 2025 •

edited

Loading

ofedoren Jan 23, 2025

girijaasoni commented Jan 23, 2025

ofedoren left a comment

		success_message _('Successfully invalidated registration tokens.')
		build_options

	command_name 'invalidate_multiple'
	command_name 'invalidate-multiple'

	class InvalidateMultipleCommand < HammerCLIForeman::SingleResourceCommand
	class InvalidateMultipleCommand < HammerCLIForeman::DeleteCommand

	class InvalidateCommand < HammerCLIForeman::SingleResourceCommand
	class InvalidateCommand < HammerCLIForeman::DeleteCommand

Fixes #38124 - Invalidate tokens for specific user/users #636

Fixes #38124 - Invalidate tokens for specific user/users #636

Conversation

girijaasoni commented Jan 6, 2025 • edited Loading

nofaralfasi left a comment

Choose a reason for hiding this comment

girijaasoni commented Jan 7, 2025

ofedoren 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

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

ofedoren commented Jan 17, 2025

ofedoren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

girijaasoni Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

girijaasoni commented Jan 23, 2025

ofedoren left a comment

Choose a reason for hiding this comment

girijaasoni commented Jan 6, 2025 •

edited

Loading

girijaasoni Jan 23, 2025 •

edited

Loading