-
Notifications
You must be signed in to change notification settings - Fork 0
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
[PR-23132] Add the ability to store certificates in the database #1
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we have some method that can be redefined in child controller, e.g. |
||
end | ||
file = Passkit::Generator.compress_passes_files(files) | ||
send_file(file, type: 'application/vnd.apple.pkpasses', disposition: 'attachment') | ||
else | ||
file = Passkit::Factory.create_pass(@payload[:pass_class], @generator) | ||
file = Passkit::Factory.create_pass(@payload[:pass_class], generator: @generator, site_id: params[:site_id]) | ||
send_file(file, type: 'application/vnd.apple.pkpass', disposition: 'attachment') | ||
end | ||
end | ||
|
@@ -35,11 +35,10 @@ def show | |
return | ||
end | ||
|
||
pass_output_path = Passkit::Generator.new(pass).generate_and_sign | ||
|
||
response.headers["last-modified"] = pass.last_update.httpdate | ||
if request.headers["If-Modified-Since"].nil? || | ||
(pass.last_update.to_i > Time.zone.parse(request.headers["If-Modified-Since"]).to_i) | ||
pass_output_path = Passkit::Generator.new(pass).generate_and_sign | ||
send_file(pass_output_path, type: "application/vnd.apple.pkpass", disposition: "attachment") | ||
else | ||
head :not_modified | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,7 @@ module V1 | |
# @see Android: https://walletpasses.io/developer/ | ||
class RegistrationsController < ActionController::API | ||
before_action :load_pass, only: %i[create destroy] | ||
before_action :load_device, only: %i[show] | ||
before_action :load_device, only: %i[show destroy] | ||
|
||
# @return If the serial number is already registered for this device, returns HTTP status 200. | ||
# @return If registration succeeds, returns HTTP status 201. | ||
|
@@ -48,8 +48,10 @@ def show | |
# @return If the request is not authorized, returns HTTP status 401. | ||
# @return Otherwise, returns the appropriate standard HTTP status. | ||
def destroy | ||
registrations = @pass.registrations.where(passkit_device_id: params[:device_id]) | ||
registrations.delete_all | ||
render json: {}, status: :ok unless @device | ||
|
||
registrations = @pass.registrations.where(passkit_device_id: @device.id) | ||
registrations.destroy_all | ||
render json: {}, status: :ok | ||
Comment on lines
+51
to
55
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Double render happens here, you need to return after the first There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good point There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. my bad. |
||
end | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
module Passkit | ||
class Certificate < ActiveRecord::Base | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
module Passkit::CertificateSources::CertificateSource | ||
def certificate | ||
raise NotImplementedError | ||
end | ||
|
||
def password | ||
raise NotImplementedError | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
class Passkit::CertificateSources::Database | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe worth of adding db migration as well, but optional. |
||
include Passkit::CertificateSources::CertificateSource | ||
|
||
def initialize(site_id) | ||
@site_id = site_id | ||
end | ||
|
||
def certificate | ||
certificate_record.certificate | ||
end | ||
|
||
def password | ||
certificate_record.secret | ||
end | ||
|
||
private | ||
|
||
def certificate_record | ||
@certificate_record ||= Passkit::Certificate.find_by!(site_id: @site_id) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. definitely we shouldn't search here by site_id. |
||
end | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
class Passkit::CertificateSources::Env | ||
include Passkit::CertificateSources::CertificateSource | ||
|
||
def certificate | ||
Rails.root.join(ENV["PASSKIT_PRIVATE_P12_CERTIFICATE"]) | ||
end | ||
|
||
def password | ||
ENV["PASSKIT_CERTIFICATE_KEY"] | ||
end | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
class Passkit::CertificateSources::Factory | ||
def self.find_source(site_id) | ||
if Passkit.configuration.use_database_for_certificates | ||
Passkit::CertificateSources::Database.new(site_id) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can built this source in Talkable and pass source (instead of site_id) as an argument. |
||
else | ||
Passkit::CertificateSources::Env.new | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO everything site-related is part of Talkable-specific custom logic and should be in Talkable, not in gem. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, but There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
attributes = { klass: pass_class, generator: generator } | ||
attributes[:site_id] = site_id if site_id | ||
|
||
pass = Passkit::Pass.find_or_create_by!(attributes) | ||
Passkit::Generator.new(pass).generate_and_sign | ||
end | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ def initialize(pass) | |
end | ||
|
||
def generate_and_sign | ||
@pass.instance.prepare_files | ||
check_necessary_files | ||
create_temporary_directory | ||
copy_pass_to_tmp_location | ||
|
@@ -98,6 +99,10 @@ def generate_json_pass | |
pass[:semantics] = @pass.semantics if @pass.semantics | ||
pass[:userInfo] = @pass.user_info if @pass.user_info | ||
|
||
unless @pass[:sharing_prohibited] | ||
pass[:sharing] = @pass.sharing if @pass.sharing | ||
end | ||
|
||
pass[@pass.pass_type] = { | ||
headerFields: @pass.header_fields, | ||
primaryFields: @pass.primary_fields, | ||
|
@@ -123,13 +128,11 @@ def generate_json_manifest | |
File.write(@manifest_url, manifest.to_json) | ||
end | ||
|
||
CERTIFICATE = Rails.root.join(ENV["PASSKIT_PRIVATE_P12_CERTIFICATE"]) | ||
INTERMEDIATE_CERTIFICATE = Rails.root.join(ENV["PASSKIT_APPLE_INTERMEDIATE_CERTIFICATE"]) | ||
CERTIFICATE_PASSWORD = ENV["PASSKIT_CERTIFICATE_KEY"] | ||
|
||
# :nocov: | ||
def sign_manifest | ||
p12_certificate = OpenSSL::PKCS12.new(File.read(CERTIFICATE), CERTIFICATE_PASSWORD) | ||
p12_certificate = OpenSSL::PKCS12.new(File.read(certificate_source.certificate), certificate_source.password) | ||
intermediate_certificate = OpenSSL::X509::Certificate.new(File.read(INTERMEDIATE_CERTIFICATE)) | ||
|
||
flag = OpenSSL::PKCS7::DETACHED | OpenSSL::PKCS7::BINARY | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. here too, pass doesn't have site_id. |
||
end | ||
end | ||
end |
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 configuration is not needed if we make the gem accept
certificate_source
object and call#certificate
and#password
methods on it.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.
without this configuration, ENV variables will be checked each time
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.
Why check ENV variables when certificate_source is passed?