Skip to content

initial work on security schema (only jwt bearer) #143

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

Open
muhammadn wants to merge 16 commits into
base: master
Choose a base branch
from
Open

initial work on security schema (only jwt bearer) #143

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
5ffec5c
initial work on security schema (only jwt bearer)
muhammadn Feb 15, 2023
ef238a1
add more code for jwt token
muhammadn Feb 16, 2023
57c9be1
add security
muhammadn Feb 20, 2023
eb9c334
remove unnecessary code
muhammadn Feb 20, 2023
93e0732
Almost there...
muhammadn Feb 21, 2023
a8edef7
Almost there...
muhammadn Feb 21, 2023
6b7b75a
remove old file
muhammadn Feb 21, 2023
2cb7f30
add more improvements
muhammadn Feb 21, 2023
fa56cee
update working code
muhammadn Feb 21, 2023
fea000d
add jwt validation
muhammadn Feb 22, 2023
8a778a8
fix a bug
muhammadn Feb 22, 2023
f48001b
fix tests
muhammadn Feb 22, 2023
3c7cf65
include jwt library
muhammadn Feb 24, 2023
3ff37b3
merge files
muhammadn Feb 24, 2023
616f0bc
update code
muhammadn Feb 24, 2023
89b7c08
cleanup
muhammadn Feb 24, 2023
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
14 changes: 10 additions & 4 deletions lib/openapi_parser/request_operation.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,11 @@ class << self
# @param [OpenAPIParser::Config] config
# @param [OpenAPIParser::PathItemFinder] path_item_finder
# @return [OpenAPIParser::RequestOperation, nil]
def create(http_method, request_path, path_item_finder, config)
def create(http_method, request_path, path_item_finder, config, security_schemes = {})
result = path_item_finder.operation_object(http_method, request_path)
return nil unless result

self.new(http_method, result, config)
self.new(http_method, result, config, security_schemes)
end
end

Expand All @@ -25,18 +25,23 @@ def create(http_method, request_path, path_item_finder, config)
# @return [String]
# @!attribute [r] path_item
# @return [OpenAPIParser::Schemas::PathItem]
attr_reader :operation_object, :path_params, :config, :http_method, :original_path, :path_item
attr_reader :operation_object, :path_params, :config, :http_method, :original_path, :path_item, :security_schemes

# @param [String] http_method
# @param [OpenAPIParser::PathItemFinder::Result] result
# @param [OpenAPIParser::Config] config
def initialize(http_method, result, config)
def initialize(http_method, result, config, security_schemes)
@http_method = http_method.to_s
@original_path = result.original_path
@operation_object = result.operation_object
@path_params = result.path_params || {}
@path_item = result.path_item_object
@config = config
@security_schemes = security_schemes
end

def validate_security(security_schemes, headers)
operation_object.validate_security_schemes(security_schemes, headers)

@ota42y ota42y Feb 23, 2023

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The OpenAPI Specification define that it is OK if either one of OpenAPI Object or Operation Object's security requirement is satisfied, so need to check both security fields.
https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.0.3.md#securityRequirementObject

When a list of Security Requirement Objects is defined on the OpenAPI Object or Operation Object, only one of the Security Requirement Objects in the list needs to be satisfied to authorize the request.

Now RequestOperation Object doesn't have OpenAPI's security field so we need pass it too.

@ota42y ota42y Feb 23, 2023

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think it is good to create validator which require Security Scheme Object and Security Requriement Object and validate seurity schemes specified by requirement objects for validating OpenAPI Object and Operation Object.

@muhammadn muhammadn Feb 27, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ota42y I don't understand this part:

I think it is good to create validator which require Security Scheme Object and Security Requriement Object

@ota42y ota42y Mar 2, 2023

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

If you are adding the validation method to the Security Requriement Object, that's fine. I was thinking of using Security Scheme Object for validation and thought that would be a problem. Ignore it.

end

def validate_path_params(options = nil)
Expand Down Expand Up @@ -65,6 +70,7 @@ def validate_response_body(response_body, response_validate_options = nil)
# @param [OpenAPIParser::SchemaValidator::Options] options request validator options
def validate_request_parameter(params, headers, options = nil)
options ||= config.request_validator_options
validate_security(security_schemes, headers)

@ota42y ota42y Feb 23, 2023

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Now users are using code under the assumption that this validation will not take place, and it may suddenly break at update time.
Therefore, it would be better to control this optionally or make it a separate method and have it called separately.

operation_object&.validate_request_parameter(params, headers, options)
end

Expand Down
1 change: 1 addition & 0 deletions lib/openapi_parser/schemas.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,4 @@
require_relative 'schemas/media_type'
require_relative 'schemas/schema'
require_relative 'schemas/header'
require_relative 'schemas/security_schemes'
1 change: 1 addition & 0 deletions lib/openapi_parser/schemas/classes.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,5 @@ class MediaType < Base; end
class Schema < Base; end
class Components < Base; end
class Header < Base; end
class SecuritySchemes < Base; end
end
4 changes: 4 additions & 0 deletions lib/openapi_parser/schemas/components.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,5 +24,9 @@ class Components < Base
# @!attribute [r] headers
# @return [Hash{String => Header}, nil] header objects
openapi_attr_hash_object :headers, Header, reference: true

# @!attribute [r] security_schemes
# @return [Hash{String => Header}, nil]
openapi_attr_hash_object :security_schemes, SecuritySchemes, reference: true, schema_key: :securitySchemes
end
end
2 changes: 1 addition & 1 deletion lib/openapi_parser/schemas/openapi.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ def initialize(raw_schema, config, uri: nil, schema_registry: {})

# @return [OpenAPIParser::RequestOperation, nil]
def request_operation(http_method, request_path)
OpenAPIParser::RequestOperation.create(http_method, request_path, @path_item_finder, @config)
OpenAPIParser::RequestOperation.create(http_method, request_path, @path_item_finder, @config, components&.security_schemes)
end

# load another schema with shared config and schema_registry
Expand Down
20 changes: 18 additions & 2 deletions lib/openapi_parser/schemas/operation.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@ module OpenAPIParser::Schemas
class Operation < Base
include OpenAPIParser::ParameterValidatable

openapi_attr_values :tags, :summary, :description, :deprecated
openapi_attr_values :tags, :summary, :description, :deprecated, :security

openapi_attr_value :operation_id, schema_key: :operationId

openapi_attr_list_object :parameters, Parameter, reference: true

# @!attribute [r] request_body
Expand All @@ -30,5 +30,21 @@ def validate_request_body(content_type, params, options)
def validate_response(response_body, response_validate_options)
responses&.validate(response_body, response_validate_options)
end

def validate_security_schemes(securitySchemes, headers)
securitySchemes&.each do |securityScheme|

@ota42y ota42y Feb 23, 2023

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

We must checks for a Security Scheme Object with a corresponding name if there is a SecurityRequriment Object so we don't need check all securitySchemes.

def validate_security_schemes(securitySchemes, headers)
  validate_results = security.map do |s| # s is security requirement object
    # check all security
    s.validate_security_schemes(securitySchemes, headers)
  end
  if validate_results.count(true) == 1
    return true # accept 
  end
  return ValidateSecurityError()
end

I have determined that the validate passes if only one security scheme is valid, and if two or more are valid, the validate does not pass.
( We need check validate_results.count(true) == 1, not validate_results.count(true) >= 1 )
Is this OK? My English is not very good, so I doubt if I am reading it correctly.

Only one of the security requirement objects need to be satisfied to authorize a request.

@muhammadn muhammadn Feb 24, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ota42y
your english is ok...! I have to understand, security requirement object does not have schema information about type, scheme and bearerFormat is only in SecuritySchema object.

So this code below cannot be used in security.rb which is security requirements object (the latest code that i just pushed)

   # this code cannot be used in security.rb (security requirement object)
    def validate_security_schemes(securityScheme, headers)
      if self.type == "http" && self.scheme == "bearer" && self.bearer_format == "JWT"
      end
    end

@muhammadn muhammadn Feb 28, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ota42y Hi. any updates? I'm thinking of doing my own fork because i cannot find the solution with:

def validate_security_schemes(securitySchemes, headers)
  validate_results = security.map do |s| # s is security requirement object
    # check all security
    s.validate_security_schemes(securitySchemes, headers) <-- this needs to be object, cannot use "self" with `securitySchemes
  end
  if validate_results.count(true) == 1
    return true # accept 
  end
  return ValidateSecurityError()
end

@ota42y ota42y Mar 2, 2023

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

security requirement object does not have schema information about type, scheme and bearerFormat is only in SecuritySchema object.

You are right. I confused security scheme object and secuirty requirement object 🙇

# check if the endpoint has security in schema
if security
# if security exists, check what securitySchemas used for enforcing
security.each do |s|
# securityScheme[0] is the securitySchema name
if s == securityScheme[0]
# securitySchema[1] is the values like "type", "scheme" and bearerFormat
securityScheme[1].validate_security_schemes(securityScheme[1], headers)
end
end
end
end
end
end
end
20 changes: 20 additions & 0 deletions lib/openapi_parser/schemas/security_schemes.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
require 'jwt'
Comment thread
muhammadn marked this conversation as resolved.
module OpenAPIParser::Schemas
class SecuritySchemes < Base

openapi_attr_values :type, :description, :scheme
openapi_attr_value :bearer_format, schema_key: :bearerFormat

def validate_security_schemes(securityScheme, headers)
if self.type == "http" && self.scheme == "bearer" && self.bearer_format == "JWT"
raise "need authorization" unless headers["AUTHORIZATION"]
raise "not bearer" unless headers["AUTHORIZATION"].split[0] == "Bearer"

@ota42y ota42y Feb 23, 2023

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Please define new error class.
https://github.com/ota42y/openapi_parser/blob/master/lib/openapi_parser/errors.rb


# check if the JWT token is being sent and try to decode.
# if JWT token does not exist or token cannot decode, then deny access
token = headers["AUTHORIZATION"].split[1]
value = JWT.decode token, nil, false

@ota42y ota42y Feb 23, 2023

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

How much detail to verify here is a difficult question.
When verifying a signed JWT, the user expects the signature to be correct.
But we must passed key and various options to the library.
The options are numerous and require the same amount of effort as creating a wrapper for the jwt library.
Also, the OpenAPI definition does not expect JWT to have detailed information about signature verification.

One solution would be to have the user pass in a callback what validation is to be performed, and this library would only call the callback.
This would allow users to, for example, check for certain values in the JWT or force disabling of the JWT for some users only.

In this case, instead of limiting the bearerFormat=JWT, I think it would be possible to pass a Security Scheme Object to the callback so that users can validate non-JWT formats as well, thus allowing for a wider range of applications.

end
end
end
end
5 changes: 5 additions & 0 deletions spec/data/petstore-expanded.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,11 @@ paths:
$ref: '#/components/schemas/Pet'

components:
securitySchemes:
jwt:
type: "http"
scheme: "bearer"
bearerFormat: "JWT"
parameters:
test:
name: limit
Expand Down
2 changes: 2 additions & 0 deletions spec/openapi_parser/schemas/components_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
expect(subject.request_bodies['test_body'].class).to eq OpenAPIParser::Schemas::RequestBody
expect(subject.request_bodies['test_body'].object_reference).to eq '#/components/requestBodies/test_body'

expect(subject.security_schemes['jwt'].class).to eq OpenAPIParser::Schemas::SecuritySchemes

expect(subject.headers['X-Rate-Limit-Limit'].class).to eq OpenAPIParser::Schemas::Header
end
end
Expand Down