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

Parser for public-keys value #2450

Merged
merged 3 commits into from
Jan 3, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
module Authentication
module AuthnJwt
module SigningKey
# This class is responsible for parsing JWK set from public-keys configuration value
class FetchPublicKeysSigningKey
tzheleznyak marked this conversation as resolved.
Show resolved Hide resolved

def initialize(
public_keys:,
logger: Rails.logger
)
@logger = logger
@public_keys = public_keys
end

def call(*)
@logger.info(LogMessages::Authentication::AuthnJwt::ParsingStaticSigningKeys.new)
signing_keys = Authentication::AuthnJwt::SigningKey::PublicSigningKeys.new(JSON.parse(@public_keys))
signing_keys.validate!
@logger.debug(LogMessages::Authentication::AuthnJwt::ParsedStaticSigningKeys.new)
{ keys: JSON::JWK::Set.new(signing_keys.value) }
end
end
end
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
module Authentication
module AuthnJwt
module SigningKey
# This class is a POJO class presents public-keys structure
tzheleznyak marked this conversation as resolved.
Show resolved Hide resolved
class PublicSigningKeys
Copy link

Choose a reason for hiding this comment

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

Authentication::AuthnJwt::SigningKey::PublicSigningKeys assumes too much for instance variable '@value'

include ActiveModel::Validations
include AttrRequired

VALID_TYPES = %w[jwks].freeze

attr_required(:type, :value)

validates(*required_attributes, presence: true)
validates(:type, inclusion: { in: VALID_TYPES, message: "'%{value}' is not a valid public-keys type" })
validate(:validate_value_is_jwks, if: -> { @type == "jwks" })

def initialize(hash)
raise Errors::Authentication::AuthnJwt::InvalidPublicKeys, "the value is not in valid JSON format" unless
hash.is_a?(Hash)

hash = hash.with_indifferent_access
required_attributes.each do |key|
send("#{key}=", hash[key])
end
end

def validate!
tzheleznyak marked this conversation as resolved.
Show resolved Hide resolved
Copy link

Choose a reason for hiding this comment

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

Authentication::AuthnJwt::SigningKey::PublicSigningKeys has missing safe method 'validate!'

raise Errors::Authentication::AuthnJwt::InvalidPublicKeys, errors.full_messages.to_sentence unless valid?
end

private

def validate_value_is_jwks
errors.add(:value, "is not a valid JWKS (RFC7517)") unless @value.is_a?(Hash) &&
@value[:keys].is_a?(Array) &&
Copy link

Choose a reason for hiding this comment

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

Authentication::AuthnJwt::SigningKey::PublicSigningKeys#validate_value_is_jwks calls '@value[:keys]' 2 times

sashaCher marked this conversation as resolved.
Show resolved Hide resolved
!@value[:keys].empty?
sashaCher marked this conversation as resolved.
Show resolved Hide resolved
end
end
end
end
end
5 changes: 5 additions & 0 deletions app/domain/errors.rb
Original file line number Diff line number Diff line change
Expand Up @@ -578,6 +578,11 @@ module AuthnJwt
msg: "Restriction '{0-restriction-name}' is invalid and not representing claim path in the token",
code: "CONJ00119E"
)

InvalidPublicKeys = ::Util::TrackableErrorClass.new(
msg: "Failed to parse 'public-keys': {0-parse-error}",
code: "CONJ00120E"
)
end

module ResourceRestrictions
Expand Down
10 changes: 10 additions & 0 deletions app/domain/logs.rb
Original file line number Diff line number Diff line change
Expand Up @@ -718,6 +718,16 @@ module AuthnJwt
msg: "Claim path has been parsed: '{0-claim-path-array}'",
code: "CONJ00142D"
)

ParsingStaticSigningKeys = ::Util::TrackableLogMessageClass.new(
msg: "Parsing JWKS from public-keys value",
code: "CONJ00143I"
)

ParsedStaticSigningKeys = ::Util::TrackableLogMessageClass.new(
msg: "Successfully parsed public-keys value",
code: "CONJ00144D"
)
end
end

Expand Down
20 changes: 0 additions & 20 deletions config/locales/en.yml

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
# frozen_string_literal: true

require 'spec_helper'

RSpec.describe('Authentication::AuthnJwt::SigningKey::FetchPublicKeysSigningKey') do

let(:log_output) { StringIO.new }
let(:logger) {
Logger.new(
log_output,
formatter: proc do | severity, time, progname, msg |
"#{severity},#{msg}\n"
end)
}

let(:string_value) { "string value" }
let(:valid_jwks) {
Net::HTTP.get_response(URI("https://www.googleapis.com/oauth2/v3/certs")).body
}
let(:invalid_public_keys_value) {
"{\"type\":\"invalid\", \"value\": #{valid_jwks} }"
}
let(:valid_public_keys_value) {
"{\"type\":\"jwks\", \"value\": #{valid_jwks} }"
}

# ____ _ _ ____ ____ ____ ___ ____ ___
# (_ _)( )_( )( ___) (_ _)( ___)/ __)(_ _)/ __)
# )( ) _ ( )__) )( )__) \__ \ )( \__ \
# (__) (_) (_)(____) (__) (____)(___/ (__) (___/

context "FetchPublicKeysSigningKey call" do
context "fails when the value is not a JSON" do
subject do
::Authentication::AuthnJwt::SigningKey::FetchPublicKeysSigningKey.new(
public_keys: string_value
).call(force_fetch: false)
end

it "raises error" do
expect { subject }
.to raise_error(JSON::ParserError)
end
end

context "fails when the value is not valid" do
subject do
::Authentication::AuthnJwt::SigningKey::FetchPublicKeysSigningKey.new(
public_keys: invalid_public_keys_value
).call(force_fetch: false)
end

it "raises error" do
expect { subject }
.to raise_error(Errors::Authentication::AuthnJwt::InvalidPublicKeys)
end
end

context "returns a JWKS object" do
subject do
::Authentication::AuthnJwt::SigningKey::FetchPublicKeysSigningKey.new(
public_keys: valid_public_keys_value
).call(force_fetch: false)
end

it "JWKS object has one key" do
expect(subject.length).to eql(1)
end

it "JWKS object key is keys" do
expect(subject.key?(:keys)).to be true
end

it "JWKS object value be a JWK Set" do
expect(subject[:keys]).to be_a(JSON::JWK::Set)
end
end

context "writes logs" do
subject do
::Authentication::AuthnJwt::SigningKey::FetchPublicKeysSigningKey.new(
public_keys: valid_public_keys_value,
logger: logger
).call(force_fetch: false)
log_output.string.split("\n")
end

it "as expected" do
expect(subject).to eql([
"INFO,CONJ00143I Parsing JWKS from public-keys value",
"DEBUG,CONJ00144D Successfully parsed public-keys value"
])
end
end
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
# frozen_string_literal: true

require 'spec_helper'

RSpec.describe('Authentication::AuthnJwt::SigningKey::PublicSigningKeys') do

invalid_cases = {
"When public-keys value is a string":
["blah",
"the value is not in valid JSON format"],
"When public-keys value is an array":
[%w[a b],
"the value is not in valid JSON format"],
"When public-keys value is an empty object":
[{},
"Type can't be blank, Type '' is not a valid public-keys type, and Value can't be blank"],
"When public-keys does not contain needed fields":
[{:key => "value", :key2 => { :key3 => "valve" }},
"Type can't be blank, Type '' is not a valid public-keys type, and Value can't be blank"],
"When public-keys type is empty and value is absent":
[{:type => ""},
"Type can't be blank, Type '' is not a valid public-keys type, and Value can't be blank"],
"When public-keys type has wrong value and value is absent":
[{:type => "yes"},
"Value can't be blank and Type 'yes' is not a valid public-keys type"],
"When public-keys type is valid and value is a string":
[{:type => "jwks", :value => "string"},
"Value is not a valid JWKS (RFC7517)"],
"When public-keys type is valid and value is an empty object":
[{:type => "jwks", :value => { } },
"Value can't be blank and Value is not a valid JWKS (RFC7517)"],
"When public-keys type is valid and value is an object with some key":
[{:type => "jwks", :value => { :some_key => "some_value" } },
"Value is not a valid JWKS (RFC7517)"],
"When public-keys type is valid and value is an object with `keys` key and string keys value":
[{:type => "jwks", :value => { :keys => "some_value" } },
"Value is not a valid JWKS (RFC7517)"],
"When public-keys type is valid and value is an object with `keys` key and empty array keys value":
[{:type => "jwks", :value => { :keys => [ ] } },
"Value is not a valid JWKS (RFC7517)"],
"When public-keys type is invalid and value is an object with `keys` key and none empty array keys value":
[{:type => "invalid", :value => { :keys => [ "some_value" ] } },
"Type 'invalid' is not a valid public-keys type"]
}

let(:valid_jwks) {
{:type => "jwks", :value => { :keys => [ "some_value" ] } }
}

context "Public-keys value validation" do
context "Invalid examples" do
invalid_cases.each do |description, (hash, expected_error_message) |
context "#{description}" do
subject do
Authentication::AuthnJwt::SigningKey::PublicSigningKeys.new(hash)
end

it "raises an error" do

expect { subject.validate! }
.to raise_error(
Errors::Authentication::AuthnJwt::InvalidPublicKeys,
"CONJ00120E Failed to parse 'public-keys': #{expected_error_message}")
end
end
end
end

context "Valid examples" do
context "When public-keys type is jwks and value meets minimal jwks requirements" do
subject do
Authentication::AuthnJwt::SigningKey::PublicSigningKeys.new(valid_jwks)
end

it "validates! does not raise error" do
expect { subject.validate! }
.not_to raise_error
end

it "type is jwks" do
expect(subject.type).to eql("jwks")
end

it "can create JWKS from value" do
expect { JSON::JWK::Set.new(subject.value) }
.not_to raise_error
end
end
end
end
end