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

simplify fix_auth tests #23212

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

simplify fix_auth tests #23212

wants to merge 1 commit into from

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Sep 27, 2024

There is an issue in the way that encryption keys work. You can sometimes decrypt a string that has been encoded with a different passkey. You will get the wrong value out, but the system thinks that it succeeded.

Even though the odds of this are very low, since we run managiq so many times, we run across it as a sporadic test failure.

This is an issue and we're removing the offending tests. This functionality is tested in the manageiq-password gem.

@Fryguy
Copy link
Member

Fryguy commented Sep 27, 2024

Not saying we shouldn't merge but would it be better to keep the tests and just mark them as pending or xit?

@Fryguy
Copy link
Member

Fryguy commented Sep 27, 2024

also I'm surprised at the removal of some of these - I wouldn't have expected blank column to need to be removed, for example

Comment on lines -81 to -84
it "should replace for bad encryption" do
subject.fix_passwords(bad, options.merge(:invalid => "other"))
expect(bad.password).to be_encrypted("other")
end
Copy link
Member

Choose a reason for hiding this comment

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

This was the only one I expected to go away, cause I've seen this one fail - can't say the same for the others.

There is an issue in the way that encryption keys work.
You can sometimes decrypt a string that has been encoded with a different passkey.
You will get the wrong value out, but the system thinks that it succeeded.

Even though the odds of this are very low, since we run managiq so many times, we
run across it as a sporadic test failure.

This is an issue and we're removing the offending tests.
This functionality is tested in the `manageiq-password` gem.
@kbrock
Copy link
Member Author

kbrock commented Sep 27, 2024

update:

  • rebased
  • put most tests back and dropped stubbing. Just doing that one test

@miq-bot
Copy link
Member

miq-bot commented Sep 27, 2024

Checked commit kbrock@a5440c5 with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
1 file checked, 0 offenses detected
Everything looks fine. 👍

@kbrock
Copy link
Member Author

kbrock commented Sep 27, 2024

kicking

@kbrock kbrock closed this Sep 27, 2024
@kbrock kbrock reopened this Sep 27, 2024
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