Skip to content

Use client connection for GET requests#21

Merged
tagliala merged 1 commit intomasterfrom
bugfix/20-honor-client-options
Nov 10, 2025
Merged

Use client connection for GET requests#21
tagliala merged 1 commit intomasterfrom
bugfix/20-honor-client-options

Conversation

@tagliala
Copy link
Copy Markdown
Member

@tagliala tagliala commented Nov 10, 2025

Ensure the configured client options (SSL, proxy, timeouts) are honored
by performing GETs via the client connection when fetching the OpenID
configuration and the JWKS certificates. Previously these requests used
Faraday.get, bypassing the client’s settings.

Additionally, add byebug for debugging, bump RuboCop and related cops,
update the RuboCop todo, and ignore .byebug_history.

Close ifad/omniauth-client#20

@tagliala tagliala requested a review from Copilot November 10, 2025 09:53
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the HTTP client implementation in the Keycloak strategy and adds development tooling. The main functional change replaces direct Faraday.get calls with client.connection.get, which properly utilizes the OAuth2 client's connection object for HTTP requests.

  • Replace direct Faraday calls with client.connection calls in the Keycloak strategy
  • Add byebug debugger to development dependencies
  • Update rubocop and related gems to newer versions

Reviewed Changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
lib/omniauth/strategies/keycloak.rb Replace Faraday.get with client.connection.get for configuration and certificate fetching to properly use the OAuth2 client's connection
spec/spec_helper.rb Add byebug require for debugging support
Gemfile Add byebug gem and update rubocop-related gems to newer versions (1.81.7, 1.26.1, 3.7.0)
.rubocop_todo.yml Increase AbcSize metric threshold from 57 to 61
.gitignore Add .byebug_history to ignore byebug session files

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +11 to 12
gem 'byebug'
gem 'rake'
Copy link

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

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

The byebug gem should be placed in a development/test group rather than at the top level. This ensures it's only loaded in development and test environments, not in production. Consider wrapping it in a group block:

group :development, :test do
  gem 'byebug'
end
Suggested change
gem 'byebug'
gem 'rake'
gem 'rake'
group :development, :test do
gem 'byebug'
end

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@copilot in this case, when there is a .gemspec, the Gemfile is for development and test

@tagliala tagliala force-pushed the bugfix/20-honor-client-options branch from bc593b9 to 267df65 Compare November 10, 2025 09:59
Ensure the configured client options (SSL, proxy, timeouts) are honored
by performing GETs via the client connection when fetching the OpenID
configuration and the JWKS certificates. Previously these requests used
Faraday.get, bypassing the client’s settings.

Additionally, add byebug for debugging, bump RuboCop and related cops,
update the RuboCop todo, and ignore .byebug_history.

Close ifad/omniauth-client#20
@tagliala tagliala force-pushed the bugfix/20-honor-client-options branch from 267df65 to aaad8cd Compare November 10, 2025 10:00
@tagliala tagliala requested a review from Copilot November 10, 2025 10:01
@tagliala tagliala changed the title Use client connection to make GET requests Use client connection for GET requests Nov 10, 2025
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown

Copilot AI commented Nov 10, 2025

@tagliala I've opened a new pull request, #22, to work on those changes. Once the pull request is ready, I'll request review from you.

@tagliala tagliala merged commit d1220d8 into master Nov 10, 2025
15 checks passed
@tagliala tagliala deleted the bugfix/20-honor-client-options branch November 10, 2025 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants