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.

Sign up for GitHub

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

Jump to bottom

Allow httpd password to have dollar sign in it #257

Merged
merged 2 commits into from
Aug 15, 2017
Merged

Allow httpd password to have dollar sign in it #257

merged 2 commits into from
Aug 15, 2017

Conversation

yrudman
Copy link
Contributor

@yrudman yrudman commented Aug 8, 2017

Having $ sign inside double quotation for echo argument is interpreted as variable which need to be resolved.

https://bugzilla.redhat.com/show_bug.cgi?id=1438974

@miq-bot add-label bug

\cc @bdunne

@miq-bot miq-bot added the bug label Aug 8, 2017
@yrudman
Copy link
Contributor Author

yrudman commented Aug 9, 2017

@miq-bot add-label fine/yes

bdunne
bdunne reviewed Aug 9, 2017
View reviewed changes
lib/gems/pending/appliance_console/external_httpd_authentication.rb Outdated
@@ -128,7 +128,7 @@ def configure_sssd

def configure_ipa_http_service
say("Configuring IPA HTTP Service and Keytab ...")
AwesomeSpawn.run!("/bin/echo \"#{@password}\" | /usr/bin/kinit #{@principal}")
AwesomeSpawn.run!("/bin/echo '#{@password}' | /usr/bin/kinit #{@principal}")
Copy link
Member

Choose a reason for hiding this comment

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

@yrudman Is it possible to add some tests for this?

@yrudman yrudman force-pushed the allow-httpd-password-to-have-dollar-sign branch from 4a14b47 to 0ca205b Compare August 9, 2017 18:56
@yrudman yrudman closed this Aug 10, 2017
@yrudman yrudman reopened this Aug 10, 2017
@yrudman yrudman force-pushed the allow-httpd-password-to-have-dollar-sign branch from 0ca205b to 94fcb0c Compare August 10, 2017 13:29
@Fryguy
Copy link
Member

Fryguy commented Aug 10, 2017 •
edited
Loading

Then you can't have ' in your password either. The problem here is that awesome_spawn is being used incorrectly...This should be

AwesomeSpawn.run!("/usr/bin/kinit", :params => [@principal], :stdin_data => @password)

EDIT: Additionally this keeps the password off of the command line so it's not snoopable via ps

@yrudman
having '$' sign inside double quotation for echo argument is interpre…
71b068c 
…ted as variable which need to be resolved. Single quotation need to be used to keep '$'.

https://bugzilla.redhat.com/show_bug.cgi?id=1438974
@yrudman yrudman force-pushed the allow-httpd-password-to-have-dollar-sign branch 2 times, most recently from d3f2d9a to 03e313e Compare August 11, 2017 15:41
@yrudman
Copy link
Contributor Author

yrudman commented Aug 11, 2017

thank you @Fryguy

bdunne
bdunne requested changes Aug 11, 2017
View reviewed changes
spec/appliance_console/external_httpd_authentication_spec.rb
@@ -162,4 +163,26 @@
subject.post_activation
end
end

context "#configure_ipa_http_service" do
before do
Copy link
Member

Choose a reason for hiding this comment

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

Why use a before block when there is only one test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it looks more clean to separate what we are testing from what is not important

spec/appliance_console/external_httpd_authentication_spec.rb

context "#configure_ipa_http_service" do
before do
allow(subject).to receive(:say)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we expect all of these things to happen?

Copy link
Contributor Author

@yrudman yrudman Aug 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I was thinking that before block is setting-up environment for executing expectation in it block. I am not sure what is the right way (expect vs allow) to mock some behavior which is not tested in it.
@bdunne, do you think it make sense to remove before and replace all allow with expect here?

spec/appliance_console/external_httpd_authentication_spec.rb Outdated
allow(ApplianceConsole::Principal).to receive(:new).and_return(service)
allow(service).to receive(:register)
allow(service).to receive(:name)
allow(service).to receive(:name)
Copy link
Member

Choose a reason for hiding this comment

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

Prefer expect(service).to receive(:name).twice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my mistake, forgot to delete that line, we need that only once

@yrudman yrudman force-pushed the allow-httpd-password-to-have-dollar-sign branch from 03e313e to 782e12a Compare August 11, 2017 17:29
@miq-bot
Copy link
Member

miq-bot commented Aug 11, 2017

Checked commits yrudman/manageiq-gems-pending@71b068c~...782e12a with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

@bdunne bdunne merged commit 7bea4f3 into ManageIQ:master Aug 15, 2017
@bdunne bdunne added this to the Sprint 67 Ending Aug 21, 2017 milestone Aug 15, 2017
@bdunne bdunne self-assigned this Aug 15, 2017
@yrudman yrudman deleted the allow-httpd-password-to-have-dollar-sign branch August 15, 2017 16:04
simaishi pushed a commit that referenced this pull request Aug 15, 2017
@bdunne @simaishi
Merge pull request #257 from yrudman/allow-httpd-password-to-have-dol…
5be8085 
…lar-sign

Allow httpd password to have dollar sign in it
(cherry picked from commit 7bea4f3)

https://bugzilla.redhat.com/show_bug.cgi?id=1481846
@simaishi
Copy link
Contributor

Fine backport details:

$ git log -1
commit 5be8085f865e889b33b2f6871318dcf97b0f9305
Author: Brandon Dunne <[email protected]>
Date:   Tue Aug 15 11:57:23 2017 -0400

    Merge pull request #257 from yrudman/allow-httpd-password-to-have-dollar-sign
    
    Allow httpd password to have dollar sign in it
    (cherry picked from commit 7bea4f3d0ebda0ef479110b7c61e42e9dbf72834)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1481846

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdunne bdunne bdunne approved these changes

Assignees

@bdunne bdunne

Labels
bug fine/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@yrudman @Fryguy @miq-bot @simaishi @bdunne