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

don't raise error in get if certificate is empty #84

Merged
merged 4 commits into from
Sep 28, 2021
Merged

don't raise error in get if certificate is empty #84

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0ba600f
don't raise error in get if certificate is empty
rishichawda Sep 7, 2021
df7f495
add get! method, remove string return from verify_certificate
rishichawda Sep 7, 2021
c52c0fd
update tests
rishichawda Sep 7, 2021
0049fca
return only boolean on valid? method
rishichawda Sep 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion lib/win32/certstore.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,17 @@ def get(certificate_thumbprint, store_name: @store_name, store_location: @store_
cert_get(certificate_thumbprint, store_name: store_name, store_location: store_location)
end

# Return `OpenSSL::X509` certificate object if present otherwise raise a "Certificate not found!" error
# @param request [thumbprint<string>] of certificate
# @return [Object] of certificates in OpenSSL::X509 format
def get!(certificate_thumbprint, store_name: @store_name, store_location: @store_location)
cert_pem = cert_get(certificate_thumbprint, store_name: store_name, store_location: store_location)

raise ArgumentError, "Unable to retrieve the certificate" if cert_pem.empty?
Copy link

Choose a reason for hiding this comment

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

ArgumentError would be fitting if we were validating arguments here but since we are calling an external entity/store, we should either relay/wrap that entity's error or define a custom exception.


cert_pem
end

# Returns all the certificates in a store
# @param [nil]
# @return [Array] array of certificates list
Expand All @@ -103,7 +114,13 @@ def search(search_token)
# @param request[thumbprint<string>] of certificate
# @return [true, false] only true or false
def valid?(certificate_thumbprint, store_location: "", store_name: "")
cert_validate(certificate_thumbprint, store_location: store_location, store_name: store_name)
cert_validate(certificate_thumbprint, store_location: store_location, store_name: store_name).yield_self do |x|
if x.is_a?(TrueClass) || x.is_a?(FalseClass)
x
else
false
end
end
end
Comment on lines 116 to 124
Copy link

Choose a reason for hiding this comment

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

I think the implementations of valid? and cert_validate should have been other way round or somewhat swapped, i.e. valid? should contain the validation logic that cert_validate has, returning a bool, while cert_validate should use valid? to check and raise error or formatted error string. Something like below -

def valid?
  # validation logic
  # return boolean
end

def cert_validate
  # use valid? to check
  # raise error or return formatted error string (based on use case)
end


# To close and destroy pointer of open certificate store handler
Expand Down
11 changes: 3 additions & 8 deletions lib/win32/certstore/store_base.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,13 +92,8 @@ def cert_get(certificate_thumbprint, store_name:, store_location:)
thumbprint = update_thumbprint(certificate_thumbprint)
cert_pem = get_cert_pem(thumbprint, store_name: store_name, store_location: store_location)
cert_pem = format_pem(cert_pem)
if cert_pem.empty?
raise ArgumentError, "Unable to retrieve the certificate"
end

unless cert_pem.empty?
build_openssl_obj(cert_pem)
end
cert_pem.empty? ? cert_pem : build_openssl_obj(cert_pem)
Copy link

Choose a reason for hiding this comment

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

I think this should return nil or cert_pem/object. This would also clean .empty? checks at other places.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get on Teams and talk about this? I feel like we have competing agendas and I want to make sure I am doing this correctly.

end

# Listing certificate of open certstore and return list in json
Expand Down Expand Up @@ -147,6 +142,8 @@ def cert_validate(certificate_thumbprint, store_location:, store_name:)
thumbprint = update_thumbprint(certificate_thumbprint)
cert_pem = get_cert_pem(thumbprint, store_name: store_name, store_location: store_location)
cert_pem = format_pem(cert_pem)
return "Certificate not found" if cert_pem.empty?

verify_certificate(cert_pem)
end

Expand Down Expand Up @@ -223,8 +220,6 @@ def update_thumbprint(certificate_thumbprint)

# Verify OpenSSL::X509::Certificate object
def verify_certificate(cert_pem)
return "Certificate not found" if cert_pem.empty?

valid_duration?(build_openssl_obj(cert_pem))
end

Expand Down
21 changes: 20 additions & 1 deletion spec/win32/functional/win32/certstore_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,29 @@
@store.get(thumbprint, store_location: CERT_SYSTEM_STORE_CURRENT_USER, store_name: "My")
end

# passing invalid thumbprint
it "returns nil if certificate not found" do
thumbprint = "b1bc968bd4f49d622aa89a81f2150152a41d829cab"
cert_obj = @store.get(thumbprint, store_location: CERT_SYSTEM_STORE_CURRENT_USER, store_name: "My")
expect(cert_obj).to be_empty
end
end

describe "#get!" do
before { add_cert }
let(:cert_pem) { File.read('.\spec\win32\assets\GlobalSignRootCA.pem') }

# passing valid thumbprint
it "returns the certificate_object if found" do
thumbprint = "b1bc968bd4f49d622aa89a81f2150152a41d829c"
expect(@store).to receive(:cert_get).with(thumbprint, store_location: CERT_SYSTEM_STORE_CURRENT_USER, store_name: "My").and_return(cert_pem)
@store.get!(thumbprint, store_location: CERT_SYSTEM_STORE_CURRENT_USER, store_name: "My")
end

# passing invalid thumbprint
it "returns ArgumentError if certificate not found" do
thumbprint = "b1bc968bd4f49d622aa89a81f2150152a41d829cab"
expect { @store.get(thumbprint, store_location: CERT_SYSTEM_STORE_CURRENT_USER, store_name: "My") }.to raise_error(ArgumentError)
expect { @store.get!(thumbprint, store_location: CERT_SYSTEM_STORE_CURRENT_USER, store_name: "My") }.to raise_error(ArgumentError)
end
end

Expand Down
28 changes: 16 additions & 12 deletions spec/win32/unit/certstore_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,9 +131,10 @@
before(:each) do
allow_any_instance_of(certbase).to receive(:get_cert_pem).and_return("")
end
it "raises ArgumentError" do
it "returns empty string" do
store = certstore.open(store_name)
expect { store.get(thumbprint, store_location: CERT_SYSTEM_STORE_CURRENT_USER, store_name: store_name) }.to raise_error(ArgumentError, "Unable to retrieve the certificate")
cert_obj = store.get(thumbprint, store_location: CERT_SYSTEM_STORE_CURRENT_USER, store_name: store_name)
expect(cert_obj).to be_empty
end
end

Expand Down Expand Up @@ -259,9 +260,10 @@
before(:each) do
allow_any_instance_of(certbase).to receive(:get_cert_pem).and_return("")
end
it "raises Error" do
it "returns empty string" do
store = certstore.open(store_name)
expect { store.get(thumbprint, store_location: CERT_SYSTEM_STORE_CURRENT_USER, store_name: store_name) }.to raise_error(ArgumentError, "Unable to retrieve the certificate")
cert_obj = store.get(thumbprint, store_location: CERT_SYSTEM_STORE_CURRENT_USER, store_name: store_name)
expect(cert_obj).to be_empty
end
end
end
Expand Down Expand Up @@ -298,9 +300,9 @@
before(:each) do
allow_any_instance_of(certbase).to receive(:get_cert_pem).and_return("")
end
it "returns Certificate not found" do
it "returns false" do
store = certstore.open(store_name)
expect(store.valid?(thumbprint)).to eql("Certificate not found")
expect(store.valid?(thumbprint)).to eql(false)
end
end

Expand Down Expand Up @@ -597,9 +599,10 @@
before(:each) do
allow_any_instance_of(certbase).to receive(:get_cert_pem).and_return("")
end
it "returns nil" do
it "returns empty string" do
store = certstore.open(store_name, store_location: store_location)
expect { store.get(thumbprint, store_location: CERT_SYSTEM_STORE_CURRENT_USER, store_name: store_name) }.to raise_error(ArgumentError, "Unable to retrieve the certificate")
cert_obj = store.get(thumbprint, store_location: CERT_SYSTEM_STORE_CURRENT_USER, store_name: store_name)
expect(cert_obj).to be_empty
end
end

Expand Down Expand Up @@ -732,9 +735,10 @@
before(:each) do
allow_any_instance_of(certbase).to receive(:get_cert_pem).and_return("")
end
it "returns nil" do
it "returns empty string" do
store = certstore.open(store_name, store_location: store_location)
expect { store.get(thumbprint, store_location: CERT_SYSTEM_STORE_CURRENT_USER, store_name: store_name) }.to raise_error(ArgumentError, "Unable to retrieve the certificate")
cert_obj = store.get(thumbprint, store_location: CERT_SYSTEM_STORE_CURRENT_USER, store_name: store_name)
expect(cert_obj).to be_empty
end
end
end
Expand Down Expand Up @@ -775,9 +779,9 @@
before(:each) do
allow_any_instance_of(certbase).to receive(:get_cert_pem).and_return("")
end
it "returns Certificate not found" do
it "returns false" do
store = certstore.open(store_name, store_location: store_location)
expect(store.valid?(thumbprint)).to eql("Certificate not found")
expect(store.valid?(thumbprint)).to eql(false)
end
end

Expand Down