-
Notifications
You must be signed in to change notification settings - Fork 19
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
Refactor to use xpasswd #149
Conversation
ping @mudler |
Signed-off-by: Mauro Morales <contact@mauromorales.com>
Expect(foo).ToNot(BeNil()) | ||
Expect(foo.Gecos).To(Equal("Created by entities")) | ||
Expect(foo.Pass).To(Equal("x")) |
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.
aren't we testing Pass anymore? what's the default in this case?
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.
this is not a real pass test, there's always an x in /etc/passwd
the real passwords are saved in the shadow file
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.
right, but it is worth checking if we write/read x
here by default - to check that we are still compliant and we do not break while generating the user in the /etc/passwd file
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.
ahh, sorry I misunderstood the question. Yeah, I decided it's not a good API to ask xpasswd for a .Pass()
because it's not representative, the x
is not the password so if I implement that I want it to return the value in the shadow file.
I would claim that for this test it is actually ok because when we Load()
the passwd file, xpasswd
validates that the list fulfills the 7 elements per item. But it indeed does not check that it specifically is an x
but as far as i understand the value there should not break the functionality
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'll change to at least check that error, and will think of a better option
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.
@mudler I've updated it to also check the password
Signed-off-by: Mauro Morales <contact@mauromorales.com>
Signed-off-by: Mauro Morales <contact@mauromorales.com>
Signed-off-by: Mauro Morales <contact@mauromorales.com>
Looking good! Thanks @mauromorales , that's indeed a good add so we can also close #128 |
* Refactor to use xpasswd Signed-off-by: Mauro Morales <contact@mauromorales.com> * Change path in tests to reflect a real path Signed-off-by: Mauro Morales <contact@mauromorales.com> * Check for an error when loading the user lists Signed-off-by: Mauro Morales <contact@mauromorales.com> * Check password Signed-off-by: Mauro Morales <contact@mauromorales.com> --------- Signed-off-by: Mauro Morales <contact@mauromorales.com> (cherry picked from commit 0446b4e)
* Refactor to use xpasswd Signed-off-by: Mauro Morales <contact@mauromorales.com> * Change path in tests to reflect a real path Signed-off-by: Mauro Morales <contact@mauromorales.com> * Check for an error when loading the user lists Signed-off-by: Mauro Morales <contact@mauromorales.com> * Check password Signed-off-by: Mauro Morales <contact@mauromorales.com> --------- Signed-off-by: Mauro Morales <contact@mauromorales.com> (cherry picked from commit 0446b4e)
passwd library is simple but also limited and includes no license, so I started working on my own version xpasswd. I'd like yip to be the first consumer. Please feel free to suggest any recommendations