Add basic authentication support to i-doit integration #5633
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces basic authentication support to the i-doit integration by extending the API client, updating controller calls, and exposing new credential fields in the UI.
- Extend
Idoit.verify/Idoit.queryto acceptusernameandpasswordand inject a Basic Auth header. - Update Rails controllers and JavaScript clients to pass credentials with each request.
- Add username/password inputs in the integration settings UI.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/idoit.rb | Expanded verify, query, and _query to handle basic auth parameters and headers |
| app/controllers/integration/idoit_controller.rb | Updated verify and query calls to include username and password |
| app/assets/javascripts/app/views/integration/idoit.jst.eco | Added input fields for username and password |
| app/assets/javascripts/app/controllers/ticket_zoom/sidebar_idoit.coffee | Passed credentials in AJAX payload for object fetch |
| app/assets/javascripts/app/controllers/idoit_object_selector.coffee | Passed credentials in AJAX payload for object type and search |
| app/assets/javascripts/app/controllers/_integration/idoit.coffee | Included credentials in the form submission payload |
Comments suppressed due to low confidence (3)
lib/idoit.rb:17
- [nitpick] The parameter name
methodshadows Ruby's built-inKernel#method. Consider renaming it toapi_methodor similar to avoid confusion.
def self.verify(method, api_token, endpoint, username, password, client_id = nil, verify_ssl: false)
lib/idoit.rb:104
- The new basic auth logic in
_queryisn’t covered by existing tests. Add unit tests to verify header generation when credentials are provided and when they are omitted.
def self._query(method, username, password, params, url, verify_ssl: false)
app/assets/javascripts/app/controllers/ticket_zoom/sidebar_idoit.coffee:12
- [nitpick] Ensure that an icon named
'idoit'exists in your icon set. If not, choose an existing identifier or add the custom asset to prevent missing icon errors.
badgeIcon: 'idoit'
lib/idoit.rb
Outdated
| def self.verify(api_token, endpoint, _client_id = nil, verify_ssl: false) | ||
| raise __('Invalid i-doit configuration (missing endpoint or api_token).') if api_token.blank? || endpoint.blank? | ||
| def self.verify(method, api_token, endpoint, username, password, client_id = nil, verify_ssl: false) | ||
| raise __("Invalid i-doit configuration (missing endpoint or api_token). username: #{username}, password: #{password}") if api_token.blank? || endpoint.blank? |
There was a problem hiding this comment.
Remove username and password from this error message to avoid logging sensitive credentials. Consider raising a generic configuration error without exposing secrets.
| raise __("Invalid i-doit configuration (missing endpoint or api_token). username: #{username}, password: #{password}") if api_token.blank? || endpoint.blank? | |
| raise __("Invalid i-doit configuration (missing endpoint or api_token). Please check your configuration.") if api_token.blank? || endpoint.blank? |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
This removes it: 867bfee
It was only there for debugging purposes.
| @@ -4,7 +4,7 @@ class Integration::IdoitController < ApplicationController | |||
| prepend_before_action :authenticate_and_authorize! | |||
|
|
|||
| def verify | |||
| response = ::Idoit.verify(params[:api_token], params[:endpoint], params[:client_id], verify_ssl: params[:verify_ssl]) | |||
| response = ::Idoit.verify(params[:method], params[:api_token], params[:endpoint], params[:username], params[:password], params[:client_id], verify_ssl: params[:verify_ssl]) | |||
There was a problem hiding this comment.
The verify endpoint now expects a method name, but the UI does not supply params[:method]. Either default to a specific API method (e.g. 'cmdb.object_types') or update the settings form to include a method selector.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
From what I understood, the method name "cmdb.object_types" was hardcoded in both the frontend (idoit.coffee) and the backend (idoit.rb). This removes the previous hardcoding in "Idoit.verify" in the backend, keeping the method definition in one place.
|
@cloucs Thanks for your contribution :-) I did a first look inside the changes and it looks like it is not a state that we can easily merge. For example, the username and password should not be given from the frontend in the "real" usage in the ticket sidebar. They are also normally not available for "normal" agents. And in the end, this is not needed. Maybe you should take the It would also be nice to have some test cases for these new possibilities. Feel free to continue working on this feature idea, and we will see if we are reaching a state that could be merged in the core at some point. |
This PR adds basic authentication support to the i-doit integration. It updates the UI to allow credential input, adjusts integration logic to handle authentication and replaces the tab logo.