-
Notifications
You must be signed in to change notification settings - Fork 97
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.
|
There was a problem hiding this 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.
lib/hammer_cli_foreman.rb
Outdated
@@ -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' | |||
) |
There was a problem hiding this comment.
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.
lib/hammer_cli_foreman.rb
Outdated
'HammerCLIForeman::Registration', 'hammer_cli_foreman/registration') | ||
rescue => e | ||
handler = HammerCLIForeman::ExceptionHandler.new(:context => {}, :adapter => :base) | ||
HammerCLI::MainCommand.lazy_subcommand('registration-tokens', _('Manipulate registration tokens'), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
success_message _('Successfully invalidated registration tokens.') | ||
build_options |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
success_message _('Successfully invalidated registration tokens.') | |
build_options | |
success_message _('Successfully invalidated registration tokens.') | |
build_options |
There was a problem hiding this comment.
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' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
command_name 'invalidate_multiple' | |
command_name 'invalidate-multiple' |
command_name 'registration-tokens' | ||
desc _('Manage registration tokens') | ||
|
||
class InvalidateMultipleCommand < HammerCLIForeman::SingleResourceCommand |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class InvalidateMultipleCommand < HammerCLIForeman::SingleResourceCommand | |
class InvalidateMultipleCommand < HammerCLIForeman::DeleteCommand |
build_options | ||
end | ||
|
||
class InvalidateCommand < HammerCLIForeman::SingleResourceCommand |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class InvalidateCommand < HammerCLIForeman::SingleResourceCommand | |
class InvalidateCommand < HammerCLIForeman::DeleteCommand |
No description provided.