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 #38124 - Invalidate tokens for specific user/users #636

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

girijaasoni
Copy link
Contributor

@girijaasoni girijaasoni commented Jan 6, 2025

No description provided.

@girijaasoni 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
Copy link
Contributor

@nofaralfasi nofaralfasi left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

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

Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

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

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.

@@ -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'
)
Copy link
Member

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Comment on lines 10 to 12
success_message _('Successfully invalidated registration tokens.')
build_options
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
success_message _('Successfully invalidated registration tokens.')
build_options
success_message _('Successfully invalidated registration tokens.')
build_options

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a reason for this one? Just curious


class InvalidateMultipleCommand < HammerCLIForeman::SingleResourceCommand
action :invalidate_jwt_tokens
command_name 'invalidate_multiple'
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
command_name 'invalidate_multiple'
command_name 'invalidate-multiple'

command_name 'registration-tokens'
desc _('Manage registration tokens')

class InvalidateMultipleCommand < HammerCLIForeman::SingleResourceCommand
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
class InvalidateMultipleCommand < HammerCLIForeman::SingleResourceCommand
class InvalidateMultipleCommand < HammerCLIForeman::DeleteCommand

build_options
end

class InvalidateCommand < HammerCLIForeman::SingleResourceCommand
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
class InvalidateCommand < HammerCLIForeman::SingleResourceCommand
class InvalidateCommand < HammerCLIForeman::DeleteCommand

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants