fix(astrolabe): Address reviewer feedback for hybrid mode
Addresses code review feedback: Personal.php: - Consolidate template variables to use camelCase consistently - Remove duplicate snake_case variables (auth_mode, supports_app_passwords) - Add oauthUrl to standard OAuth mode parameters (fixes fallback issue) - Add requesttoken for CSRF protection personal.php (template): - Use null coalescing for safe variable access - Reuse computed $isHybridMode variable instead of duplicate check - Remove complex fallback URL logic (oauthUrl now always provided) IdpTokenRefresher.php: - Use Nextcloud's overwrite.cli.url config when available - Fall back to http://localhost for container deployments - Better supports non-containerized environments Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
+12
-8
@@ -38,19 +38,23 @@ class IdpTokenRefresher {
|
||||
/**
|
||||
* Get Nextcloud base URL for constructing internal OIDC endpoint URLs.
|
||||
*
|
||||
* IMPORTANT: This method returns the INTERNAL URL for server-to-server
|
||||
* requests within the container. External URLs (like localhost:8080) won't
|
||||
* work from inside the container since localhost refers to the container itself.
|
||||
* Uses Nextcloud's CLI URL config if set (for non-containerized deployments),
|
||||
* otherwise defaults to http://localhost for container environments.
|
||||
*
|
||||
* For internal requests, we always use http://localhost (port 80) since
|
||||
* Nextcloud's web server is accessible at that address inside the container.
|
||||
* Configuration priority:
|
||||
* 1. overwrite.cli.url - Official Nextcloud system config for CLI operations
|
||||
* 2. http://localhost - Default for Docker containers (web server on port 80)
|
||||
*
|
||||
* @return string Base URL for internal requests (e.g., "http://localhost")
|
||||
*/
|
||||
private function getNextcloudBaseUrl(): string {
|
||||
// For internal requests within the container, always use http://localhost
|
||||
// The web server is accessible at port 80 inside the container.
|
||||
// External URLs like http://localhost:8080 won't work from inside the container.
|
||||
// Check for overwrite.cli.url (used in non-containerized deployments)
|
||||
$cliUrl = $this->config->getSystemValue('overwrite.cli.url', '');
|
||||
if (!empty($cliUrl)) {
|
||||
return rtrim($cliUrl, '/');
|
||||
}
|
||||
|
||||
// Default: container environment with web server on localhost:80
|
||||
return 'http://localhost';
|
||||
}
|
||||
|
||||
|
||||
+13
-7
@@ -93,23 +93,21 @@ class Personal implements ISettings {
|
||||
// OAuth URL for Astrolabe's own OAuth controller (NOT MCP server's browser OAuth)
|
||||
$oauthUrl = $this->urlGenerator->linkToRoute('astrolabe.oauth.initiateOAuth');
|
||||
|
||||
// Consolidated template parameters (camelCase convention)
|
||||
$parameters = [
|
||||
'userId' => $userId,
|
||||
'serverUrl' => $this->client->getPublicServerUrl(),
|
||||
'serverStatus' => $serverStatus,
|
||||
'auth_mode' => $authMode,
|
||||
'authMode' => $authMode,
|
||||
'supports_app_passwords' => $supportsAppPasswords,
|
||||
'supportsAppPasswords' => $supportsAppPasswords,
|
||||
'session' => null, // No session in hybrid mode
|
||||
'vectorSyncEnabled' => $serverStatus['vector_sync_enabled'] ?? false,
|
||||
// OAuth token status (for Astrolabe→MCP API calls)
|
||||
'hasToken' => $hasOAuthToken,
|
||||
'hasOAuthToken' => $hasOAuthToken,
|
||||
'oauthUrl' => $oauthUrl,
|
||||
// App password status (for MCP→Nextcloud background sync)
|
||||
'hasBackgroundAccess' => $hasAppPassword,
|
||||
'backgroundAccessGranted' => $hasAppPassword,
|
||||
'backgroundAccessGranted' => $hasAppPassword, // Legacy alias
|
||||
'backgroundSyncType' => $backgroundSyncType,
|
||||
'backgroundSyncProvisionedAt' => $backgroundSyncProvisionedAt,
|
||||
'requesttoken' => \OCP\Util::callRegister(),
|
||||
@@ -186,6 +184,9 @@ class Personal implements ISettings {
|
||||
$backgroundSyncType = $this->tokenStorage->getBackgroundSyncType($userId);
|
||||
$backgroundSyncProvisionedAt = $this->tokenStorage->getBackgroundSyncProvisionedAt($userId);
|
||||
|
||||
// OAuth URL for standard OAuth mode (in case user needs to re-authorize)
|
||||
$oauthUrl = $this->urlGenerator->linkToRoute('astrolabe.oauth.initiateOAuth');
|
||||
|
||||
// Provide initial state for Vue.js frontend (if needed)
|
||||
$this->initialState->provideInitialState('user-data', [
|
||||
'userId' => $userId,
|
||||
@@ -193,17 +194,22 @@ class Personal implements ISettings {
|
||||
'session' => $userSession,
|
||||
]);
|
||||
|
||||
// Consolidated template parameters (camelCase convention)
|
||||
$parameters = [
|
||||
'userId' => $userId,
|
||||
'serverUrl' => $this->client->getPublicServerUrl(),
|
||||
'serverStatus' => $serverStatus,
|
||||
'session' => $userSession,
|
||||
'vectorSyncEnabled' => $serverStatus['vector_sync_enabled'] ?? false,
|
||||
'backgroundAccessGranted' => $userSession['background_access_granted'] ?? false,
|
||||
'serverUrl' => $this->client->getPublicServerUrl(),
|
||||
'hasToken' => true,
|
||||
// OAuth status
|
||||
'hasOAuthToken' => true,
|
||||
'oauthUrl' => $oauthUrl,
|
||||
// Background sync status
|
||||
'hasBackgroundAccess' => $hasBackgroundAccess,
|
||||
'backgroundAccessGranted' => $userSession['background_access_granted'] ?? false, // Legacy
|
||||
'backgroundSyncType' => $backgroundSyncType,
|
||||
'backgroundSyncProvisionedAt' => $backgroundSyncProvisionedAt,
|
||||
'requesttoken' => \OCP\Util::callRegister(),
|
||||
];
|
||||
|
||||
return new TemplateResponse(
|
||||
|
||||
+7
-7
@@ -44,13 +44,13 @@ style('astrolabe', 'astrolabe-main'); // All CSS bundled into main
|
||||
<h2><?php p($l->t('Background Sync Access')); ?></h2>
|
||||
|
||||
<?php
|
||||
// In hybrid mode (multi_user_basic + app passwords), user needs BOTH OAuth AND app password
|
||||
// to be "fully configured". Check both credentials in hybrid mode.
|
||||
$isHybridMode = isset($_['authMode']) && $_['authMode'] === 'multi_user_basic' && !empty($_['supportsAppPasswords']);
|
||||
// Determine if hybrid mode (multi_user_basic + app passwords)
|
||||
// In hybrid mode, user needs BOTH OAuth AND app password to be "fully configured"
|
||||
$isHybridMode = ($_['authMode'] ?? '') === 'multi_user_basic' && !empty($_['supportsAppPasswords']);
|
||||
$hasOAuthToken = !empty($_['hasOAuthToken']);
|
||||
$hasBackgroundAccess = $_['hasBackgroundAccess'] || $_['backgroundAccessGranted'];
|
||||
$hasBackgroundAccess = !empty($_['hasBackgroundAccess']) || !empty($_['backgroundAccessGranted']);
|
||||
|
||||
// In hybrid mode, both credentials required; otherwise just background access
|
||||
// In hybrid mode: both credentials required; otherwise just background access
|
||||
$isFullyConfigured = $isHybridMode ? ($hasOAuthToken && $hasBackgroundAccess) : $hasBackgroundAccess;
|
||||
?>
|
||||
<?php if ($isFullyConfigured): ?>
|
||||
@@ -120,7 +120,7 @@ style('astrolabe', 'astrolabe-main'); // All CSS bundled into main
|
||||
</div>
|
||||
<?php else: ?>
|
||||
<!-- Not configured - show provisioning options -->
|
||||
<?php if (isset($_['authMode']) && $_['authMode'] === 'multi_user_basic' && !empty($_['supportsAppPasswords'])): ?>
|
||||
<?php if ($isHybridMode): ?>
|
||||
<!-- Hybrid mode: User needs BOTH OAuth AND app password -->
|
||||
<p class="mcp-help-text">
|
||||
<?php p($l->t('To use semantic search, you need to complete two setup steps:')); ?>
|
||||
@@ -203,7 +203,7 @@ style('astrolabe', 'astrolabe-main'); // All CSS bundled into main
|
||||
<p class="mcp-help-text">
|
||||
<?php p($l->t('When Nextcloud fully supports OAuth for app APIs. Currently waiting for upstream PR to merge.')); ?>
|
||||
</p>
|
||||
<a href="<?php p($_['oauthUrl'] ?? ($_['serverUrl'] . '/oauth/login?next=' . urlencode($urlGenerator->getAbsoluteURL($urlGenerator->linkToRoute('settings.PersonalSettings.index', ['section' => 'astrolabe']))))); ?>" class="button">
|
||||
<a href="<?php p($_['oauthUrl']); ?>" class="button">
|
||||
<span class="icon icon-confirm"></span>
|
||||
<?php p($l->t('Authorize via OAuth')); ?>
|
||||
</a>
|
||||
|
||||
Reference in New Issue
Block a user