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

MSYS-547: [win32-certstore] Verify Certificate #20

Merged
merged 2 commits into from
Apr 10, 2018

Conversation

piyushawasthi
Copy link
Contributor

Note: First merge PR: #16 after that need to rebase this branch.

Description:
This method verify certificate using thumbprint and return boolean.

Usages:

thumbprint = "‎75 e0 ab b6 13 85 12 27 1c 04 f8 5f dd de 38 e4 b7 24 2e fe"
Win32::Certstore.open("ROOT") do |store|
  cert_obj = store.verify("‎75 e0 ab b6 13 85 12 27 1c 04 f8 5f dd de 38 e4 b7 24 2e fe")
  store.close
end

Return:.

True

@btm
Copy link
Contributor

btm commented Mar 27, 2018

@piyushawasthi can you rebase this and make the shell_out changes similar to #19 please?

@harikesh-kolekar harikesh-kolekar force-pushed the piyush/verify_cert branch 3 times, most recently from 9fbf8fe to 64172a0 Compare March 28, 2018 06:52
@piyushawasthi piyushawasthi force-pushed the piyush/verify_cert branch 2 times, most recently from ec8774f to 5410f31 Compare March 28, 2018 07:37
@piyushawasthi
Copy link
Contributor Author

Hi @btm , This branch is rebased and shell_out changes done. Also fixed failing test-cases and tested manually. So this is ready to merge.

@btm
Copy link
Contributor

btm commented Mar 28, 2018

It's hard to tell what the methods here do exactly. For example, I think I would name the valid_duration method valid_duration? instead, which would indicate to me that it's going to return true or false. Similarly name verify something like valid_certificate?.

We should probably start adding YaRD documentation to the library to be more formally explicit about the methods. http://www.rubydoc.info/gems/yard/file/docs/GettingStarted.md

I'll probably make a separate chore card to go back around and do that on code we've already committed.

@piyushawasthi
Copy link
Contributor Author

Hi @btm - I have update the method name vaild_certificate?(certificate_thumbprint). Please correct me if I am wrong for better understanding I think all method should be look similar.
for example either we keep method names as:

store.add(certificate_obj)
store.get(certificate_thumbprint)
store.list(certstore_handler)
store.delete(certificate_thumbprint)
store.search(certificate_name)
store.valid?(certificate_thumbprint)

OR

store.add_certificate(certificate_obj)
store.get_certificate(certificate_thumbprint)
store.list_certificates
store.delete_certificate(certificate_thumbprint)
store.search_certificate(certificate_name)
store.valid_certificate?(certificate_thumbprint)

Please let me know your thought about it.

@piyushawasthi piyushawasthi requested review from btm and rhass April 3, 2018 15:11
@btm
Copy link
Contributor

btm commented Apr 5, 2018

@piyushawasthi I remember thinking about this a while ago, but I forget what we were thinking. The first thing to come to mind was store.add sounds like I'm adding a certificate store, not a cert. Secondly, certificate feels like a word that's going to be easy to misspell, although I had no problem typing it just now. But maybe add_cert is a better pattern. Either way.

Copy link
Contributor

@btm btm left a comment

Choose a reason for hiding this comment

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

this looks fine. I'll leave it to your choice what to do about naming. this will need a rebase to merge.

@btm btm merged commit fb66bc1 into chef:master Apr 10, 2018
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.

2 participants