[PR-23132] Add the ability to store certificates in the database #1

pofystyy · 2024-11-12T18:56:51Z

Specs will be added and modified after the review of the main logic.

zhuravel · 2024-11-12T19:02:28Z

lib/passkit/factory.rb

@@ -2,8 +2,11 @@ module Passkit
  class Factory
    class << self
      # generator is an optional ActiveRecord object, the application data for the pass
-      def create_pass(pass_class, generator = nil)
-        pass = Passkit::Pass.create!(klass: pass_class, generator: generator)
+      def create_pass(pass_class, generator: nil, site_id: nil)


IMO everything site-related is part of Talkable-specific custom logic and should be in Talkable, not in gem.
Changes here should be more universal so that we could open source our changes.

My point is you should not hardcode site_id, not that you should not allow providing extra metadata. That can be some “options” hash that would not be tied to Talkable and would work for much more cases than this one and allow using our gem in other companies potentially.

zhuravel · 2024-11-12T19:03:06Z

lib/passkit/certificate_sources/factory.rb

+class Passkit::CertificateSources::Factory
+  def self.find_source(site_id)
+    if Passkit.configuration.use_database_for_certificates
+      Passkit::CertificateSources::Database.new(site_id)


We can built this source in Talkable and pass source (instead of site_id) as an argument.

zhuravel · 2024-11-12T19:06:02Z

app/controllers/passkit/api/v1/registrations_controller.rb

+          render json: {}, status: :ok unless @device
+
+          registrations = @pass.registrations.where(passkit_device_id: @device.id)
+          registrations.destroy_all
          render json: {}, status: :ok


Double render happens here, you need to return after the first render, or better rewrite this to only have a single render.

Initial changes

good point
will be fixed

zhuravel · 2024-11-12T19:07:46Z

app/controllers/passkit/api/v1/passes_controller.rb

@@ -9,12 +9,12 @@ def create

          if @generator && @payload[:collection_name].present?
            files = @generator.public_send(@payload[:collection_name]).collect do |collection_item|
-              Passkit::Factory.create_pass(@payload[:pass_class], collection_item)
+              Passkit::Factory.create_pass(@payload[:pass_class], generator: collection_item, site_id: params[:site_id])


Could we have some method that can be redefined in child controller, e.g. pass_attributes instead of using hardcoded site_id in gem? Don’t want gem to know anything about sites.

zhuravel · 2024-11-12T19:10:05Z

README.md

+and update the configuration as follows:
+```ruby
+Passkit.configure do |config|
+  config.use_database_for_certificates = false


This configuration is not needed if we make the gem accept certificate_source object and call #certificate and #password methods on it.

without this configuration, ENV variables will be checked each time

Why check ENV variables when certificate_source is passed?

max-si-m · 2024-11-20T12:59:10Z

lib/passkit/certificate_sources/database.rb

@@ -0,0 +1,22 @@
+class Passkit::CertificateSources::Database


maybe worth of adding db migration as well, but optional.

max-si-m · 2024-11-20T13:00:08Z

lib/passkit/certificate_sources/database.rb

+  private
+
+  def certificate_record
+    @certificate_record ||= Passkit::Certificate.find_by!(site_id: @site_id)


definitely we shouldn't search here by site_id.

max-si-m · 2024-11-20T13:06:41Z

lib/passkit/generator.rb

@@ -155,5 +158,9 @@ def compress_pass_file
      end
      zip_path
    end
+
+    def certificate_source
+      @certificate_source ||= Passkit::CertificateSources::Factory.find_source(@pass.site_id)


here too, pass doesn't have site_id.

max-si-m · 2024-11-20T13:07:06Z

app/controllers/passkit/api/v1/registrations_controller.rb

+          render json: {}, status: :ok unless @device
+
+          registrations = @pass.registrations.where(passkit_device_id: @device.id)
+          registrations.destroy_all
          render json: {}, status: :ok


max-si-m · 2024-11-21T15:52:23Z

as an idea, we can pass intermediate and p12_certificate into as it looks like this is only the one place where certificate actually used

passkit/lib/passkit/generator.rb

Lines 131 to 142 in 83c9c8d

    
           def sign_manifest 
        
             p12_certificate = OpenSSL::PKCS12.new(File.read(CERTIFICATE), CERTIFICATE_PASSWORD) 
        
             intermediate_certificate = OpenSSL::X509::Certificate.new(File.read(INTERMEDIATE_CERTIFICATE)) 
        
             flag = OpenSSL::PKCS7::DETACHED | OpenSSL::PKCS7::BINARY 
        
             signed = OpenSSL::PKCS7.sign(p12_certificate.certificate, 
        
               p12_certificate.key, File.read(@manifest_url), 
        
               [intermediate_certificate], flag) 
        
             @signature_url = @temporary_path.join("signature") 
        
             File.open(@signature_url, "w") { |f| f.syswrite signed.to_der } 
        
           end

how this can be used:

module Passkit
  class Factory
    class << self
      # generator is an optional ActiveRecord object, the application data for the pass
      def create_pass(pass_class, generator = nil, options = {})
        pass = Passkit::Pass.find_or_create_by!(klass: pass_class, generator: generator, **options)
        p12_certificate = OpenSSL::PKCS12.new(db.certificate, db.password)
        intermediate_certificate = OpenSSL::X509::Certificate.new(File.read(INTERMEDIATE_CERTIFICATE))

        Passkit::Generator.new(pass).generate_and_sign(p12_certificate, intermediate_certificate)
      end
    end
  end
end

feel free to reach out and set up a quick call to with me and @zhuravel

pofystyy added 2 commits November 11, 2024 16:51

[PR-23132] general changes

f5d84c7

[PR-23132] Add the ability to store certificates in the database

49f01e3

zhuravel self-assigned this Nov 12, 2024

zhuravel requested review from zhuravel and max-si-m November 12, 2024 18:58

zhuravel reviewed Nov 12, 2024

View reviewed changes

max-si-m requested changes Nov 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PR-23132] Add the ability to store certificates in the database #1

[PR-23132] Add the ability to store certificates in the database #1

pofystyy commented Nov 12, 2024

zhuravel Nov 12, 2024

pofystyy Nov 18, 2024

zhuravel Nov 18, 2024

zhuravel Nov 12, 2024

zhuravel Nov 12, 2024 •

edited

Loading

pofystyy Nov 18, 2024

max-si-m Nov 20, 2024

zhuravel Nov 12, 2024

zhuravel Nov 12, 2024

pofystyy Nov 18, 2024

zhuravel Nov 18, 2024

max-si-m Nov 20, 2024

max-si-m Nov 20, 2024

max-si-m Nov 20, 2024

max-si-m Nov 20, 2024

max-si-m commented Nov 21, 2024 •

edited

Loading

[PR-23132] Add the ability to store certificates in the database #1

Are you sure you want to change the base?

[PR-23132] Add the ability to store certificates in the database #1

Conversation

pofystyy commented Nov 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhuravel Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

max-si-m commented Nov 21, 2024 • edited Loading

zhuravel Nov 12, 2024 •

edited

Loading

max-si-m commented Nov 21, 2024 •

edited

Loading