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 #8016 - Session auth in hammer #269

Merged
merged 5 commits into from
Dec 9, 2016

Conversation

tstrachota
Copy link
Member

@tstrachota tstrachota commented Nov 24, 2016

  • api connection moved to a separate object
  • credentials moved to InteractiveBasicAuth
  • new SessionAuthenticatorWrapper for handling api sessions

Requires:

TODO:

  • add logging of auth type
  • add config for turning sessions off
  • fix sessions being dropped after unauthenticated call (hammer ping)

- api connection moved to a separate object
- credentials moved to InteractiveBasicAuth
- new SessionAuthenticatorWrapper for handling api sessions
@mbacovsky
Copy link
Member

First of all thanks for this cool feature, it was on many wish lists for a long time.

I was testing it and it looks almost ready to go. I've found the following that needs some solution/discussion:

  1. it would be great to know from hammer logs what auth was used - session, credentials
  2. can we influence the expiration time from hammer side?
  3. can we turn the session auth feature off?
  4. can we set the session expired from hammer side?
  5. when session expires hammer fails with error. It should retry and obtain credentials from config or user as usually

As per our discussion I know that some might be difficult to implement right now. Filed issue to track the change may be acceptable solution in some cases.

What is really annoying is that session expires when using command that does not need auth (e.g. ping) and it makes the following command fail. Can that be fixed?

@tstrachota
Copy link
Member Author

Thanks for the review, answers below:

1. it would be great to know from hammer logs what auth was used - session, credentials
Ok, I'll add some more logging.

2. can we influence the expiration time from hammer side?
No

3. Can we turn the session auth feature off?
No, it probably makes sense to add it.

4. can we set the session expired from hammer side?
No

5. when session expires hammer fails with error. It should retry and obtain credentials from config or user as usually
I agree such behaviour would be much better. I intentionally didn't introduce it in this PR because that would make it even more complicated and difficult to review. It's something that's definitely worth adding in future.

Expiring the session after unauthenticated call is definitely a bug. I'll fix that. Please feel free to open additional issues for the rest.

@mbacovsky
Copy link
Member

@tstrachota, I did some more testing. The session expiration is set by default to 60 min. It is very annoying When after every hour of inactivity your hammer command fail. I also used hammer in a cron job and every second run failed when the interval was bigger then the expiration time. We either need to implement the retry or add possibility to disable sessions by configuration and CLI arg. What do you think?

@tstrachota
Copy link
Member Author

@mbacovsky updated and ready for another round of review

Sessions are now turned off by default and you have to enable it by setting :use_sessions: true in the config file.

@@ -8,6 +8,7 @@
# Credentials. You'll be asked for them interactively if you leave them blank here
:username: 'admin'
#:password: 'example'
:use_sessions: false
Copy link
Member

Choose a reason for hiding this comment

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

Are sessions specific to Basic Auth or can they be used with e.g. OAuth? Just to confirm this is the right place for the config.

Copy link
Member Author

Choose a reason for hiding this comment

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

Some auth methods might require sessions but in general it's imho independent.

@mbacovsky
Copy link
Member

I've tested this and now it works well for me. Thanks for fixing the auth commands, I forgot to test them.
I'm ready to merge when the question on the config is answered.

@tstrachota
Copy link
Member Author

@mbacovsky thanks. I'll bump dependency on the latest apipie-bindings prior to merge.

@mbacovsky
Copy link
Member

[test]

@mbacovsky
Copy link
Member

@tstrachota this looks good to merge. Please add deps on apipie-bindings >= 0.0.19 that was released today. Part in core was merged as it seems it can work with older bindings.

@tstrachota
Copy link
Member Author

@mbacovsky thanks for the apipie-bindings release! Dependency bumped.

@mbacovsky
Copy link
Member

@tstrachota awesome!
lgtm

@mbacovsky mbacovsky merged commit ce9ede2 into theforeman:master Dec 9, 2016
@thomasmckay
Copy link
Contributor

thomasmckay commented Dec 10, 2016

@tstrachota @mbacovsky - I need some help here. How do I replace this file with the new mechanics for auth?
https://github.com/Katello/hammer-cli-csv/blob/master/lib/hammer_cli_csv/utils/config.rb
http://projects.theforeman.org/issues/17624

Here is the PR I am working on. I get

RestClient::ServiceUnavailable (Invalid username or password):

when using the @api variable
Katello/hammer-cli-csv#143

@tstrachota tstrachota deleted the sessions_8016 branch May 17, 2017 07:48
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.

4 participants