Skip to content

Conversation

@Gared
Copy link
Owner

@Gared Gared commented Jul 14, 2025

No description provided.

@Gared Gared marked this pull request as ready for review July 14, 2025 21:10
@Gared Gared requested a review from Copilot July 14, 2025 21:11
Copy link

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

Adds a test case and implementation updates to verify admin login functionality during scanning.

  • Updated getAdmin in ScannerService to POST to admin-auth/ and fall back to GET admin/
  • Added a new GitHub Actions job EtherpadAdminLogin to run an end-to-end admin login check

Reviewed Changes

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

File Description
src/Service/ScannerService.php Replace GET with POST admin-auth, handle auth responses and fallback GET request
.github/workflows/e2e-test.yml Add new EtherpadAdminLogin job to test admin login in CI
Comments suppressed due to low confidence (1)

.github/workflows/e2e-test.yml:395

  • E2E tests cover successful admin login but lack a test for failed login scenarios. Consider adding a case with incorrect credentials to verify the false path.
          command_line: bin/console.php ether:scan http://localhost:9001

$callback->onScanAdminResult($user, $password, $response->getStatusCode() === 200);
return;
} catch (GuzzleException) {
$callback->onScanAdminResult($user, $password, false);
Copy link

Copilot AI Jul 14, 2025

Choose a reason for hiding this comment

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

Missing return after callback in the second catch block. Without an explicit return, code after this block could execute, causing multiple callbacks or unintended behavior. Add return; after invoking the callback.

Suggested change
$callback->onScanAdminResult($user, $password, false);
$callback->onScanAdminResult($user, $password, false);
return;

Copilot uses AI. Check for mistakes.
]);
$callback->onScanAdminResult($user, $password, $response->getStatusCode() === 200);
return;
} catch (GuzzleException $e) {
Copy link

Copilot AI Jul 14, 2025

Choose a reason for hiding this comment

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

[nitpick] Catching all GuzzleExceptions without logging or rethrowing hides potential issues beyond authentication failures. Consider logging the exception or rethrowing unexpected errors to improve debuggability.

Copilot uses AI. Check for mistakes.
Comment on lines +231 to +235
$callback->onScanAdminResult($user, $password, $response->getStatusCode() === 200);
return;
} catch (GuzzleException $e) {
if ($e->getCode() === 401) {
$callback->onScanAdminResult($user, $password, false);
Copy link

Copilot AI Jul 14, 2025

Choose a reason for hiding this comment

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

[nitpick] The callback invocation is duplicated across multiple branches. Consider refactoring to extract a helper or iterate endpoints to reduce duplication.

Suggested change
$callback->onScanAdminResult($user, $password, $response->getStatusCode() === 200);
return;
} catch (GuzzleException $e) {
if ($e->getCode() === 401) {
$callback->onScanAdminResult($user, $password, false);
$this->invokeScanAdminResult($callback, $user, $password, $response->getStatusCode() === 200);
return;
} catch (GuzzleException $e) {
if ($e->getCode() === 401) {
$this->invokeScanAdminResult($callback, $user, $password, false);

Copilot uses AI. Check for mistakes.
@Gared Gared merged commit e19887e into main Jul 14, 2025
51 checks passed
@Gared Gared deleted the fix_admin_login_detection branch July 14, 2025 21:13
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.

2 participants