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

(#459) prevent colon added in realm.properties when auth_users hash is empty #460

Merged
merged 1 commit into from Jul 13, 2021

Conversation

overag3
Copy link
Contributor

@overag3 overag3 commented Jan 5, 2021

Pull Request (PR) description

This changeset fix issue when @auth_config['file']['auth_users'] hash is empty, a colon added in realm.properties file when it shouldn't.
Fix also wrong indentation.

This Pull Request (PR) fixes the following issues

Fixes #459

- fix incorrect indentation
- add checks for auth_users hash
Copy link
Member

@kenyon kenyon left a comment

Choose a reason for hiding this comment

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

Thanks for the improvement. It would be even better if this had tests, to prevent such breakage from recurring.

@overag3
Copy link
Contributor Author

overag3 commented Jan 6, 2021

Test already exists in auth_spec.rb file but i found the real problem...

In params manifest(L62, L168), the type of auth_users variable is hash but in spec tests and in real.properties.erb template file, the type of auth_users needs to be a array not hash.

Besides, is line 62 necessary ?

@kenyon
Copy link
Member

kenyon commented Jan 7, 2021

Good finds. Yeah, looks like auth_users in params.pp line 62 is unused. The template also treats it as a hash, in addition to an array:

<%= @auth_config['file']['auth_users']['username'] -%>:<%= @auth_config['file']['auth_users']['password'] -%>

This module needs better data typing and better testing. 😒

@overag3
Copy link
Contributor Author

overag3 commented Jan 7, 2021

Am I modifying the params.pp file in this PR ?

I have never written spec tests before. 😟

@overag3
Copy link
Contributor Author

overag3 commented Jan 7, 2021

Good finds. Yeah, looks like auth_users in params.pp line 62 is unused. The template also treats it as a hash, in addition to an array:

<%= @auth_config['file']['auth_users']['username'] -%>:<%= @auth_config['file']['auth_users']['password'] -%>

This module needs better data typing and better testing. unamused

Yes, auth_users is a array of hashes. Example is available in :

'auth_users' => [
{
'username' => 'testuser',
'password' => 'password',
'roles' => %w[user deploy]
},
{
'username' => 'anotheruser',
'password' => 'anotherpassword',
'roles' => ['user']
}
]

Tell me what I can do to solve this issue.

Regards

@kenyon
Copy link
Member

kenyon commented Jan 8, 2021

I think this PR is fine as is since it fixes the problem. We can make other PRs for overhauling the data types and testing. I'm just waiting for anyone else to provide feedback here, otherwise I think we can merge this soon.

@ghoneycutt ghoneycutt merged commit 7f35356 into voxpupuli:master Jul 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

a colon is added to realm.properties file even when auth_users hash is empty
3 participants