Skip to content

Conversation

@sriramkanakam87
Copy link
Collaborator

No description provided.

Copy link
Contributor

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 introduces token-based authentication for accessing the ChemInformatics Microservice (CMS) API and refactors the codebase to use centralized configuration and service patterns. It replaces direct env() calls with config() helpers throughout the application and introduces a new CmsClient service to handle authenticated API communication with the CMS.

Key Changes:

  • Introduced CmsClient service with Bearer token authentication for CMS API requests
  • Added CmsProxyController with two endpoints: a generic CSRF-protected proxy and a rate-limited depiction endpoint
  • Refactored all CMS API calls to use the new CmsClient service instead of direct Http facade calls
  • Replaced env() calls with config() helpers for better configuration caching and Laravel best practices
  • Added getDepictUrl() helper function to centralize depiction URL generation through the proxy

Reviewed changes

Copilot reviewed 32 out of 33 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
app/Services/CmsClient.php New service for authenticated CMS API communication with Bearer token support
app/Http/Controllers/CmsProxyController.php New controller providing generic proxy and depiction endpoints for CMS API
routes/web.php Added two new routes: generic CSRF-protected proxy and rate-limited depiction endpoint
config/services.php Added configuration for CMS API URLs, authentication token, rate limiting, citation services, and Tawk chat
config/filesystems.php Replaced env() with config() for public disk URL
app/Helper.php Added getDepictUrl() helper and updated citation fetching to use config()
app/Jobs/ProcessEntryBatch.php Refactored to use CmsClient service for pre-processing API calls
app/Jobs/ProcessEntry.php Refactored to use CmsClient service for pre-processing API calls
app/Jobs/ImportEntry.php Updated citation API URLs to use config() instead of env()
app/Jobs/ImportEntryAuto.php Updated citation API URLs to use config() instead of env()
app/Console/Commands/GenerateCoordinates.php Refactored to use CmsClient service for coordinate generation API calls
app/Console/Commands/SubmissionsAutoProcess/FetchCitationMetadataAuto.php Updated citation API URLs to use config() instead of env()
app/Notifications/*.php Updated APP_URL references to use config() instead of env()
app/Models/Molecule.php Updated schema URL generation to use config() instead of env()
app/Livewire/MoleculeDepict2d.php Replaced direct API URL construction with getDepictUrl() helper
resources/views/molecule.blade.php Updated Open Graph and Twitter meta image URLs to use getDepictUrl() helper
resources/views/components/tawk-chat.blade.php Updated Tawk URL to use config() instead of env()
resources/views/forms/components/organisms-table.blade.php Updated dashboard URL to use config() instead of env()
app/Filament/Dashboard/Resources/*.php Updated all depiction URLs to use getDepictUrl() helper and dashboard URLs to use config()
.env.example Added documentation for CMS configuration variables
composer.lock Updated Symfony dependencies to newer versions

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

Comment on lines +128 to +132
$response = $cmsClient->get('chem/coconut/pre-processing', [
'smiles' => $canonical_smiles,
'_3d_mol' => 'false',
'descriptors' => 'false',
], false);
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

The CmsClient::get() method is being called with 3 arguments, but the method signature only accepts 2 parameters ($endpoint and $params). The third argument false will be ignored. This appears to be a bug that needs to be fixed by updating the CmsClient::get() method signature to accept this third parameter.

Copilot uses AI. Check for mistakes.
Comment on lines +62 to +66
$response = $cmsClient->get('chem/coconut/pre-processing', [
'smiles' => $canonical_smiles,
'_3d_mol' => 'false',
'descriptors' => 'false',
], false);
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

The CmsClient::get() method is being called with 3 arguments, but the method signature only accepts 2 parameters ($endpoint and $params). The third argument false will be ignored. This appears to be a bug that needs to be fixed by updating the CmsClient::get() method signature to accept this third parameter.

Copilot uses AI. Check for mistakes.
/**
* Make the actual HTTP request
*
* @return \Illuminate\Http\Client\Response
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

The documentation states the parameter type @return \Illuminate\Http\Client\Response, but the method can also throw an \InvalidArgumentException for unsupported HTTP methods. The documentation should include a @throws tag to document this possibility.

Suggested change
* @return \Illuminate\Http\Client\Response
* @return \Illuminate\Http\Client\Response
* @throws \InvalidArgumentException

Copilot uses AI. Check for mistakes.
Comment on lines +27 to +30
public function get(string $endpoint, array $params = [])
{
return $this->makeRequest('GET', $endpoint, $params);
}
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

The get method signature only accepts 2 parameters ($endpoint and $params), but it's being called with 3 arguments in multiple places (e.g., ProcessEntryBatch.php:128, ProcessEntry.php:62, GenerateCoordinates.php:113). The third parameter appears to be a boolean flag, but it's not defined in the method signature and will be ignored.

If the third parameter is intended to control some behavior (like whether to use authentication), it should be added to the method signature with a proper parameter name and type.

Copilot uses AI. Check for mistakes.
$response = $cmsClient->get($endpoint, [
'smiles' => $smiles,
'toolkit' => 'rdkit',
], false);
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

The CmsClient::get() method is being called with 3 arguments, but the method signature only accepts 2 parameters ($endpoint and $params). The third argument false will be ignored. This appears to be a bug that needs to be fixed by updating the CmsClient::get() method signature to accept this third parameter.

Suggested change
], false);
]);

Copilot uses AI. Check for mistakes.
Route::get('/collections', CollectionList::class)->name('collections.index');

// CMS API Proxy - CSRF protected for generic proxy
Route::match(['GET', 'POST'], '/cms-proxy', [CmsProxyController::class, 'proxy'])->name('cms.proxy');
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

The /cms-proxy endpoint is publicly accessible without authentication middleware. This allows any user to proxy requests to the internal CMS API. Consider adding authentication middleware if this endpoint should be restricted to authenticated users only, or at minimum add rate limiting to prevent abuse.

Copilot uses AI. Check for mistakes.
Comment on lines +52 to +56
$response = $this->cmsClient->get('depict/2D', $params);

return response($response->body())
->header('Content-Type', $response->header('Content-Type') ?? 'image/svg+xml')
->header('Cache-Control', 'public, max-age=86400');
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

The response status code is not preserved when the API request fails. The proxy always returns the CMS API's status code, but if the request throws an exception (timeout, network error), it will result in a 500 error without a proper error response. Consider wrapping the API call in a try-catch and returning an appropriate error status (e.g., 502 Bad Gateway) when the upstream service is unavailable.

Copilot uses AI. Check for mistakes.
Comment on lines +24 to +25
{
$endpoint = $request->input('endpoint');
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

Missing input validation for the endpoint parameter. A malicious user could potentially pass arbitrary endpoints, which could lead to unintended API calls. Consider validating or sanitizing the endpoint input, or maintaining a whitelist of allowed endpoints.

Suggested change
{
$endpoint = $request->input('endpoint');
{
// Define a whitelist of allowed endpoints
$allowedEndpoints = [
'articles',
'users',
'depict/2D',
// Add other allowed endpoints here
];
$endpoint = $request->input('endpoint');
if (!in_array($endpoint, $allowedEndpoints, true)) {
return response('Invalid endpoint', 400);
}

Copilot uses AI. Check for mistakes.
Comment on lines +29 to +38
$response = match ($method) {
'GET' => $this->cmsClient->get($endpoint, $params),
'POST' => $this->cmsClient->post($endpoint, $params),
default => abort(405, 'Method not allowed'),
};

return response($response->body())
->header('Content-Type', $response->header('Content-Type'))
->header('Cache-Control', 'public, max-age=86400')
->setStatusCode($response->status());
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

Missing error handling for failed API requests. If the CMS API returns an error or times out, the proxy will forward that error response to the client without any logging or fallback behavior. Consider adding try-catch blocks and logging failures for monitoring and debugging purposes.

Copilot uses AI. Check for mistakes.
Comment on lines +52 to +56
$response = $this->cmsClient->get('depict/2D', $params);

return response($response->body())
->header('Content-Type', $response->header('Content-Type') ?? 'image/svg+xml')
->header('Cache-Control', 'public, max-age=86400');
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

Missing error handling for failed API requests in the depict2D method. If the CMS API is unavailable or returns an error, users will see broken images without any fallback. Consider adding try-catch blocks and returning a placeholder image or appropriate error response on failure.

Suggested change
$response = $this->cmsClient->get('depict/2D', $params);
return response($response->body())
->header('Content-Type', $response->header('Content-Type') ?? 'image/svg+xml')
->header('Cache-Control', 'public, max-age=86400');
try {
$response = $this->cmsClient->get('depict/2D', $params);
// If the response is not successful, return a placeholder image
if ($response->status() !== 200) {
return $this->placeholderImage();
}
return response($response->body())
->header('Content-Type', $response->header('Content-Type') ?? 'image/svg+xml')
->header('Cache-Control', 'public, max-age=86400');
} catch (\Exception $e) {
// On exception, return a placeholder image
return $this->placeholderImage();
}
}
/**
* Returns a simple SVG placeholder image indicating an error.
*
* @return \Illuminate\Http\Response
*/
private function placeholderImage()
{
$svg = <<<SVG
<svg xmlns="http://www.w3.org/2000/svg" width="120" height="40">
<rect width="120" height="40" fill="#eee"/>
<text x="60" y="25" font-size="14" text-anchor="middle" fill="#888">Image Error</text>
</svg>
SVG;
return response($svg)
->header('Content-Type', 'image/svg+xml')
->header('Cache-Control', 'public, max-age=300');

Copilot uses AI. Check for mistakes.
@vcnainala vcnainala added the wip label Nov 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants