Skip to content

Commit

Permalink
Fix safe RuboCop offenses in production code
Browse files Browse the repository at this point in the history
  • Loading branch information
tagliala committed Jan 9, 2024
1 parent baf07d9 commit 2808a29
Show file tree
Hide file tree
Showing 5 changed files with 40 additions and 37 deletions.
2 changes: 0 additions & 2 deletions .rubocop.yml
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
inherit_from: .rubocop_todo.yml

require:
- rubocop-packaging
- rubocop-performance
Expand Down
2 changes: 1 addition & 1 deletion lib/omniauth/cas.rb
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
require 'omniauth/cas/version'
require 'omniauth/strategies/cas'
require 'omniauth/strategies/cas'
50 changes: 26 additions & 24 deletions lib/omniauth/strategies/cas.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ class InvalidCASTicket < StandardError; end
autoload :LogoutRequest, 'omniauth/strategies/cas/logout_request'

attr_accessor :raw_info
alias_method :user_info, :raw_info
alias user_info raw_info

option :name, :cas # Required property by OmniAuth::Strategy

Expand All @@ -26,7 +26,7 @@ class InvalidCASTicket < StandardError; end
option :service_validate_url, '/serviceValidate'
option :login_url, '/login'
option :logout_url, '/logout'
option :on_single_sign_out, Proc.new {}
option :on_single_sign_out, proc {}
# A Proc or lambda that returns a Hash of additional user info to be
# merged with the info returned by the CAS server.
#
Expand All @@ -35,7 +35,7 @@ class InvalidCASTicket < StandardError; end
# @param [Hash] The user info for the Service Ticket returned by the CAS server
#
# @return [Hash] Extra user info
option :fetch_raw_info, Proc.new { Hash.new }
option :fetch_raw_info, proc { {} }
# Make all the keys configurable with some defaults set here
option :uid_field, 'user'
option :name_key, 'name'
Expand All @@ -48,23 +48,23 @@ class InvalidCASTicket < StandardError; end
option :phone_key, 'phone'

# As required by https://github.com/intridea/omniauth/wiki/Auth-Hash-Schema
AuthHashSchemaKeys = %w{name email nickname first_name last_name location image phone}
AuthHashSchemaKeys = %w[name email nickname first_name last_name location image phone]
info do
prune!({
name: raw_info[options[:name_key].to_s],
email: raw_info[options[:email_key].to_s],
nickname: raw_info[options[:nickname_key].to_s],
first_name: raw_info[options[:first_name_key].to_s],
last_name: raw_info[options[:last_name_key].to_s],
location: raw_info[options[:location_key].to_s],
image: raw_info[options[:image_key].to_s],
phone: raw_info[options[:phone_key].to_s]
})
name: raw_info[options[:name_key].to_s],
email: raw_info[options[:email_key].to_s],
nickname: raw_info[options[:nickname_key].to_s],
first_name: raw_info[options[:first_name_key].to_s],
last_name: raw_info[options[:last_name_key].to_s],
location: raw_info[options[:location_key].to_s],
image: raw_info[options[:image_key].to_s],
phone: raw_info[options[:phone_key].to_s]
})
end

extra do
prune!(
raw_info.delete_if{ |k,v| AuthHashSchemaKeys.include?(k) }
raw_info.delete_if { |k, _v| AuthHashSchemaKeys.include?(k) }
)
end

Expand All @@ -82,8 +82,10 @@ def callback_phase
else
@ticket = request.params['ticket']
return fail!(:no_ticket, MissingCASTicket.new('No CAS Ticket')) unless @ticket

fetch_raw_info(@ticket)
return fail!(:invalid_ticket, InvalidCASTicket.new('Invalid CAS Ticket')) if raw_info.empty?

super
end
end
Expand All @@ -97,7 +99,7 @@ def request_phase
'Location' => login_url(service_url),
'Content-Type' => 'text/plain'
},
["You are being redirected to CAS for sign-in."]
['You are being redirected to CAS for sign-in.']
]
end

Expand Down Expand Up @@ -136,9 +138,9 @@ def extract_url
end

def validate_cas_setup
if options.host.nil? || options.login_url.nil?
raise ArgumentError.new(":host and :login_url MUST be provided")
end
return unless options.host.nil? || options.login_url.nil?

raise ArgumentError, ':host and :login_url MUST be provided'
end

# Build a service-validation URL from +service+ and +ticket+.
Expand All @@ -154,9 +156,9 @@ def service_validate_url(service_url, ticket)
service_url = Addressable::URI.parse(service_url)
service_url.query_values = service_url.query_values.tap { |qs| qs.delete('ticket') }
cas_url + append_params(options.service_validate_url, {
service: service_url.to_s,
ticket: ticket
})
service: service_url.to_s,
ticket: ticket
})
end

# Build a CAS login URL from +service+.
Expand All @@ -175,7 +177,7 @@ def login_url(service)
#
# @return [String] the new joined URL.
def append_params(base, params)
params = params.each { |k,v| v = Rack::Utils.escape(v) }
params = params.each { |_k, v| v = Rack::Utils.escape(v) }
Addressable::URI.parse(base).tap do |base_uri|
base_uri.query_values = (base_uri.query_values || {}).merge(params)
end.to_s
Expand All @@ -187,14 +189,14 @@ def validate_service_ticket(ticket)
ServiceTicketValidator.new(self, options, callback_url, ticket).call
end

private
private

def fetch_raw_info(ticket)
validator = validate_service_ticket(ticket)
ticket_user_info = validator.user_info
ticket_success_body = validator.success_body
custom_user_info = options.fetch_raw_info.call(self,
options, ticket, ticket_user_info, ticket_success_body)
options, ticket, ticket_user_info, ticket_success_body)
self.raw_info = ticket_user_info.merge(custom_user_info)
end

Expand Down
17 changes: 9 additions & 8 deletions lib/omniauth/strategies/cas/logout_request.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,37 +3,38 @@ module Strategies
class CAS
class LogoutRequest
def initialize(strategy, request)
@strategy, @request = strategy, request
@strategy = strategy
@request = request
end

def call(options = {})
@options = options

begin
result = single_sign_out_callback.call(*logout_request)
rescue StandardError => err
return @strategy.fail! :logout_request, err
rescue StandardError => e
@strategy.fail! :logout_request, e
else
result = [200,{},'OK'] if result == true || result.nil?
result = [200, {}, 'OK'] if result == true || result.nil?
ensure
return unless result

# TODO: Why does ActionPack::Response return [status,headers,body]
# when Rack::Response#new wants [body,status,headers]? Additionally,
# why does Rack::Response differ in argument order from the usual
# Rack-like [status,headers,body] array?
return Rack::Response.new(result[2],result[0],result[1]).finish
return Rack::Response.new(result[2], result[0], result[1]).finish
end
end

private
private

def logout_request
@logout_request ||= begin
saml = Nokogiri.parse(@request.params['logoutRequest'])
name_id = saml.xpath('//saml:NameID').text
sess_idx = saml.xpath('//samlp:SessionIndex').text
inject_params(name_id:name_id, session_index:sess_idx)
inject_params(name_id: name_id, session_index: sess_idx)
@request
end
end
Expand All @@ -42,7 +43,7 @@ def inject_params(new_params)
rack_input = @request.env['rack.input'].read
params = Rack::Utils.parse_query(rack_input, '&').merge new_params
@request.env['rack.input'] = StringIO.new(Rack::Utils.build_query(params))
rescue
rescue StandardError
# A no-op intended to ensure that the ensure block is run
raise
ensure
Expand Down
6 changes: 4 additions & 2 deletions lib/omniauth/strategies/cas/service_ticket_validator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def user_info
parse_user_info(@success_body)
end

private
private

# Merges attributes with multiple values into an array if support is
# enabled (disabled by default)
Expand All @@ -56,10 +56,11 @@ def attribute_value(user_info, attribute, value)
# returns nil if given nil
def parse_user_info(node)
return nil if node.nil?

{}.tap do |hash|
node.children.each do |e|
node_name = e.name.sub(/^cas:/, '')
unless e.kind_of?(Nokogiri::XML::Text) || node_name == 'proxies'
unless e.is_a?(Nokogiri::XML::Text) || node_name == 'proxies'
# There are no child elements
if e.element_children.count == 0
hash[node_name] = attribute_value(hash, node_name, e.content)
Expand All @@ -82,6 +83,7 @@ def parse_user_info(node)
# if the passed body is nil or if there is no such node.
def find_authentication_success(body)
return nil if body.nil? || body == ''

begin
doc = Nokogiri::XML(body)
begin
Expand Down

0 comments on commit 2808a29

Please sign in to comment.