Added E2E test for SSO via generic OIDC provider

[dd-georgiev]

Given that a lot of the issues in GitHub are related to SSO, I decided to add e2e test for the login flow using mock OIDC server.

1. Changed postgres in e2e scritps to supported postgres version(list with supported versions)
2. Build OIDC mock server based on oauth2 mock server
3. Added the mock OIDC server to docker-compose and attached it to the kviklet network, so that both kviklet and the client browser can connect to it using localhost. This is done via network_mode
4. Configure Kviklet container to work with the OIDC mock server
5. Added data-testid to the "Login with SSO" buttons
6. Added test for login with SSO
7. Increased the test timeout to 20 seconds, since the Create connection with required review, approve, and execute query test sometimes takes extra second when executed locally on my machine

---

Important

Add E2E test for SSO login using a mock OIDC server and update Docker configuration.

• E2E Test:
  • Added test for SSO login via generic OIDC provider in e2e.spec.ts.
  • Increased test timeout to 20 seconds in playwright.config.ts.
• Mock OIDC Server:
  • Added oidc_mock directory with Dockerfile, index.js, and package.json to set up a mock OIDC server using oauth2-mock-server.
  • Integrated mock server into e2e-compose.yml with network_mode set to service:kviklet.
• Docker and Configuration:
  • Updated e2e-compose.yml to use Postgres 13.
  • Configured kviklet service to connect to the mock OIDC server.
• Frontend:
  • Added data-testid to SSO login buttons in Login.tsx.

^{This description was created by for 199dc68. It will automatically update as commits are pushed.}

[ellipsis-dev]

❌ Changes requested. Reviewed everything up to 199dc68 in 1 minute and 42 seconds

More details

• Looked at 184 lines of code in 8 files
• Skipped 2 files when reviewing.
• Skipped posting 13 drafted comments based on config settings.

1. e2e-compose.yml:49

• Draft comment:
  Clarify purpose of mapping both ports 4444 and 80 to container's 8080. Is both needed?
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  Multiple port mappings to the same container port is a common pattern in Docker for providing different access points (e.g., one for development, one for production-like testing). The addition of port 80 alongside 4444 seems intentional given the new OIDC integration being added. The comment is asking for clarification rather than pointing out a clear issue.
  The port mappings could potentially indicate a configuration issue or unnecessary complexity. Having multiple ports mapped to the same target could be confusing for other developers.
  Multiple port mappings are a valid and common pattern in Docker compose files, especially in testing environments. The addition appears intentional given the broader changes around OIDC integration.
  Delete the comment as it merely asks for clarification without identifying a clear issue, violating our rule about not asking authors to explain their intentions.

2. e2e-compose.yml:64

• Draft comment:
  Using network_mode: service:kviklet for oidc-server-mock is interesting; ensure it doesn’t create unexpected network coupling.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

3. e2e/oidc_mock/index.js:20

• Draft comment:
  Consider handling errors in the main function (e.g., try/catch) to prevent silent failures.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
  Since this is a test mock server, error handling is less critical than in production code. If the server fails to start, the e2e tests would fail anyway. The suggestion would add complexity to test code where simplicity is often preferred. The current code will throw errors naturally if something goes wrong, which is fine for test code.
  The comment does have merit - unhandled errors could make debugging test failures more difficult. Also, proper error handling is generally a good practice regardless of context.
  However, for test code, explicit error handling is often unnecessary overhead. Test failures should be obvious and the natural error propagation is sufficient.
  The comment should be removed as it suggests adding unnecessary complexity to test code where simple, straightforward implementation is preferred.

4. e2e/tests/e2e.spec.ts:276

• Draft comment:
  The new SSO test seems to assume successful login always redirects to /requests; ensure the environment and mock properly simulate all flows.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

5. frontend/src/routes/Login.tsx:70

• Draft comment:
  Ensure data-testid uniqueness for SSO buttons; multiple providers use same 'sso-login-button' which may confuse tests.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
  Having duplicate test IDs could potentially cause issues in tests that try to interact with a specific SSO button. However, since only one SSO provider will be active at a time (based on config.oauthProvider), there will never actually be multiple buttons with the same ID rendered simultaneously. The comment may be overly cautious.
  I could be wrong about the config - maybe multiple providers can be enabled simultaneously? Also, even if only one is shown, having distinct IDs might make tests more readable and maintainable.
  Looking at the code structure and conditionals, it's clear that only one provider can be active at a time. While distinct IDs might be nice, it's not a critical issue since there's no actual conflict at runtime.
  The comment should be deleted because it raises a theoretical issue that isn't actually a problem in practice, since only one SSO button can exist at a time.

6. e2e-compose.yml:63

• Draft comment:
  Ensure the network_mode usage for oidc-server-mock is intentional; confirm that binding to service:kviklet and using localhost (ISSUERURI) works as expected in your test environment.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the author to confirm the intention behind using network_mode and binding to a specific service. It also asks to confirm that a specific configuration works in the test environment. This is not a direct code suggestion or a request for a specific test to be written, but rather a request for confirmation and testing, which violates the rules.

7. e2e/oidc_mock/Dockerfile:2

• Draft comment:
  Consider using an LTS Node version (e.g. node:18) instead of node:23 for improved stability in tests.
• Reason this comment was not posted:
  Marked as duplicate.

8. e2e/oidc_mock/index.js:20

• Draft comment:
  Add error handling in the async main function to catch initialization errors.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
  This is a test/mock server file in an e2e directory, which suggests it's only used for testing. While error handling is generally good practice, for test utilities the priority is usually simplicity and clarity over robustness. The lack of error handling means uncaught errors will crash the process, which is actually desirable behavior for a test utility - you want to know immediately if something goes wrong.
  Error handling could help diagnose issues if the mock server fails to start. Without it, stack traces might be less clear about what failed.
  For a test utility, crashing on error is acceptable and even preferable. Adding error handling would add complexity without providing significant value in this context.
  The comment should be deleted as it suggests adding complexity to test code where it's not needed - crashing on error is acceptable behavior here.

9. e2e/tests/e2e.spec.ts:276

• Draft comment:
  Ensure the 'sso-login-button' test ID is unique if multiple SSO providers are enabled to avoid ambiguous selectors.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  The comment is speculative - it's warning about a potential future issue with multiple SSO providers, but there's no evidence in the code that this is currently an issue. The test ID mentioned isn't even visible in the code. The comment is asking the author to "ensure" something, which violates our rules about verification requests.
  Maybe the test ID is defined in the LoginPage class which we can't see. Maybe multiple SSO providers are a known upcoming feature.
  Even if the test ID exists in LoginPage class, the comment is still speculative about multiple SSO providers and asks for verification. We should only comment on actual current issues, not potential future ones.
  Delete this comment as it's speculative, asks for verification, and addresses a potential future issue rather than a current problem.

10. frontend/src/routes/Login.tsx:71

• Draft comment:
  Good addition of data-testid for SSO buttons; consider using provider-specific IDs if supporting multiple options in the future.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50%
  None

11. e2e/oidc_mock/Dockerfile:9

• Draft comment:
  The Dockerfile is missing a newline at the end. Adding a newline would adhere to common style guidelines.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  While having a newline at the end of files is a common convention, this is a very minor stylistic issue. It doesn't affect functionality, and many editors automatically handle this. Most CI systems or linters would catch this if it was important to the project. This feels like unnecessary noise in the PR review.
  The trailing newline convention is well-established in Unix systems and could potentially cause issues with some tools. Maybe this is more important than I'm giving it credit for?
  While the convention exists, this is still a minor stylistic issue that doesn't warrant a PR comment. If it's important to the project, it should be caught by automated tooling rather than manual review comments.
  Delete this comment as it's too minor and stylistic to warrant manual intervention in a PR review. If this is important to the project, it should be handled by automated tooling.

12. e2e/tests/e2e.spec.ts:67

• Draft comment:
  Typo: The property 'dditionalOptions' appears to be misspelled. It should be 'additionalOptions' for consistency with the other database configurations.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

13. e2e/tests/pages.ts:14

• Draft comment:
  Typographical issue: It looks like the call to 'click()' in the ssoLogin() method on line 14 is missing a semicolon at the end. Kindly add a semicolon to keep the coding style consistent.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  The comment is technically correct - there is an inconsistency in semicolon usage. However, this is a very minor style issue that would likely be caught by a linter or formatter. Modern TypeScript/JavaScript development typically relies on automated tools for enforcing such consistency. The comment doesn't point out a functional issue or suggest an important improvement.
  The semicolon inconsistency could indicate that automated formatting is not properly set up in the project, which might be worth addressing. Also, consistent style does help with code readability.
  While code style consistency is important, it should be handled by automated tools rather than manual code review comments. This is too minor an issue to warrant a human review comment.
  Delete this comment as it points out a trivial style issue that should be handled by automated tooling rather than manual review.

Workflow ID: wflow_p1uU6fbEc7Lody9H

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

Consider pinning Node version (e.g., node:18 LTS) instead of using node:23 to ensure stability.

Suggested change

	FROM node:23
	FROM node:18

[dd-georgiev]

@Askir can you take a look at this & merge it eventually?