Feature/react UI #47
Feature/react UI #47
Conversation
WalkthroughThe pull request includes several modifications across the project. The Dockerfile has its multi-stage alias casing updated. Logging is enabled within the HAProxy configuration along with a new HAProxy service added in docker-compose. Additional VPN server entries were appended to the JSON mapping. Cypress tests have been refactored to use cross-origin commands with updated endpoints and a new "Who Am I" test has been introduced. The API endpoints in the HTML template and Flask app have been updated, and a new API endpoint for retrieving user information has been added. Finally, the Cypress dependency was reclassified and updated in package.json. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Flask as Flask App
participant Redis as Redis Cache
Client->>Flask: GET /api/whoami
Flask->>Redis: Retrieve user session info
Redis-->>Flask: Return session data / null
alt Session is active
Flask->>Client: 200 OK with user info
else Session is inactive
Flask->>Client: 401 Unauthorized
end
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
cypress/e2e/vsm.cy.js (2)
13-15: Consider making the timeout configurableThe 2-second timeout seems arbitrary. Consider making it a constant or configuration value for better maintainability.
- setTimeout(function () { - doc.location.reload() - }, 2000) +const RELOAD_TIMEOUT = 2000; + setTimeout(function () { + doc.location.reload() + }, RELOAD_TIMEOUT)
71-82: Enhance whoami test coverageThe whoami test looks good but could use additional edge cases:
- Test with expired session
- Test with invalid user
- Test with user having no VPN servers
Would you like me to generate the additional test cases?
vsm.py (1)
95-95: Enhance error responsesThe 401 responses could be more informative by including error messages.
- return Response(status=401) + return Response(json.dumps({'error': 'Session not found'}), status=401, content_type='application/json') - return Response(status=401) + return Response(json.dumps({'error': 'Session expired'}), status=401, content_type='application/json')Also applies to: 98-98
config/vsm/vpn_group_mapping.json (1)
11-28: Hey there! Let's talk about these copy-pasted server configs 👀I notice we're using identical configuration values for all three servers. While this might be fine for testing, in production we should:
- Consider using unique JWT roles per server for better security isolation
- Look into templating or configuration reuse to avoid duplication
Here's a suggestion - we could restructure this to have common configs as defaults:
[ + { + "defaults": { + "idp_group": "/example", + "vault_metadata_mountpoint": "vpnmeta", + "vault_metadata_path": "develmeta", + "vault_pki_mountpoint": "vpn/devel", + "vault_jwt_role": "test-role", + "vault_jwt_path": "jwt" + } + }, { "vpn_server": "myserver", - "idp_group": "/example", - "vault_metadata_mountpoint": "vpnmeta", - "vault_metadata_path": "develmeta", - "vault_pki_mountpoint": "vpn/devel", - "vault_jwt_role": "test-role", - "vault_jwt_path": "jwt" + "vault_jwt_role": "myserver-role" }, { "vpn_server": "myserver2", - "idp_group": "/example", - "vault_metadata_mountpoint": "vpnmeta", - "vault_metadata_path": "develmeta", - "vault_pki_mountpoint": "vpn/devel", - "vault_jwt_role": "test-role", - "vault_jwt_path": "jwt" + "vault_jwt_role": "myserver2-role" }, { "vpn_server": "myserver3", - "idp_group": "/example", - "vault_metadata_mountpoint": "vpnmeta", - "vault_metadata_path": "develmeta", - "vault_pki_mountpoint": "vpn/devel", - "vault_jwt_role": "test-role", - "vault_jwt_path": "jwt" + "vault_jwt_role": "myserver3-role" } ]docker-compose.yml (1)
44-44: Quick cleanup needed! 🧹There's a trailing space on line 44 that we should remove.
- - ./config/vsm:/app/config:ro - + - ./config/vsm:/app/config:ro +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 44-44: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
Dockerfile(1 hunks)config/haproxy/haproxy.cfg(1 hunks)config/vsm/vpn_group_mapping.json(1 hunks)cypress/e2e/vsm.cy.js(2 hunks)docker-compose.yml(1 hunks)package.json(1 hunks)templates/index.html(2 hunks)vsm.py(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- Dockerfile
🧰 Additional context used
🪛 YAMLlint (1.35.1)
docker-compose.yml
[error] 44-44: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Building container
🔇 Additional comments (4)
config/haproxy/haproxy.cfg (1)
3-3: Nice logging setup! 👍The addition of logging to stdout in raw format is perfect for containerized environments. The configuration in both global and defaults sections is well-structured.
Also applies to: 8-8
package.json (1)
2-3: Verify Cypress version compatibilityMoving Cypress to devDependencies is correct! However, let's check if version 14.0.0 has any breaking changes that might affect our tests.
✅ Verification successful
🌐 Web query:
What are the breaking changes in Cypress 14.0.0 compared to 13.15.2?💡 Result:
Cypress 14.0.0 introduced several breaking changes compared to version 13.15.2, primarily focused on modernizing dependencies and improving security/maintenance efficiency:
Core System Requirements
- Node.js support dropped: Removed compatibility with Node.js 16 and 21[1][3]
- Bundled Node.js upgraded: From v18.17 → v20.18.1[1][3]
- Linux compatibility reduced: Prebuilt binaries no longer support glibc <2.28 (affects Ubuntu 14-18/RHEL 7/CentOS 7/Amazon Linux 2)[1][3]
Browser & Testing Changes
- Strict
cy.origin()requirements: Now mandatory for cross-subdomain navigation due to Chrome'sdocument.domaindeprecation[1][3][5]- Browser support narrowed: Only latest 3 major Chrome/Firefox/Edge versions officially supported[1]
- Test isolation behavior: Fixed navigation issues when test isolation is disabled[1][3]
Removed Features
- Deprecated CLI commands:
cypress open-ct/run-ctreplaced with--componentflag[1]- Undocumented
Cypress.backend()methods removed[1]- Configuration options:
nodeVersionconfiguration removed[3]videoUploadOnPassesoption eliminated[3]Component Testing
- Just-in-Time compilation became default for Webpack (Vite excluded)[5]
- Experimental flag renamed:
experimentalJustInTimeCompile→justInTimeCompileconfig option[5]Dependency Updates
- Electron upgraded to Chromium 130[2]
- Critical security patches applied for:
express(CVE-2024-43796/45590/43800)[3]send(CVE-2024-43799)[3]loader-utils(CVE-2022-37601)[3]For migration assistance, Cypress provides an official guide addressing these changes[5]. Most users report minimal disruption when upgrading, though cross-subdomain tests require explicit
cy.origin()usage[2][3].Citations:
- 1: https://github.com/cypress-io/cypress/blob/develop/cli/CHANGELOG.md
- 2: https://www.cypress.io/blog/cypress-14-is-here-see-whats-new
- 3: https://docs.cypress.io/app/references/changelog
- 4: https://www.npmjs.com/package/cypress?activeTab=versions
- 5: https://docs.cypress.io/app/references/migration-guide
- 6: https://www.npmjs.com/package/cypress/v/0.7.0?activeTab=versions
- 7: Cypress 14: Breaking changes cypress-io/cypress#29925
- 8: https://community.chocolatey.org/packages/cypress
- 9: https://fossies.org/linux/www/cypress-14.0.0.tar.gz/cypress-14.0.0/npm/vue/CHANGELOG.md
- 10: https://tuleap.net/plugins/tracker/?aid=41797
Cypress 14.0.0 Upgrade – Breaking Changes Verification
Based on the migration details for Cypress 14.0.0, the update from 13.15.2 introduces several breaking changes that you should be aware of:
Node.js & System Requirements:
- Dropped support for older Node.js versions (e.g., Node 16) and requires using a supported version (Node ≥18).
- Linux prebuilt binaries now require glibc 2.28 or higher.
Testing Behavior Changes:
- Mandatory usage of
cy.origin()for cross-subdomain tests due to changes around Chrome’sdocument.domain.- Test isolation improvements and navigation changes that might affect tests if isolation is disabled.
CLI & Configuration Adjustments:
- Some deprecated CLI commands have been removed or replaced (e.g.,
cypress open-ct/run-ctnow use the--componentflag).- Certain configuration options like
nodeVersionandvideoUploadOnPasseshave been eliminated.While moving Cypress to devDependencies is indeed the right approach for a testing tool, please review your test suites and CI environments to ensure that the changes above do not conflict with your current setups. It may be a good idea to run your tests in a controlled staging environment after the upgrade and consult Cypress’s official migration guide for any necessary adjustments.
templates/index.html (1)
16-16: Looking good! The API paths are all lined up nicely 👍The updates to both the logout path and VPN server endpoints are consistent with the API restructuring. Nice job keeping things organized!
Also applies to: 29-29
docker-compose.yml (1)
45-62: The HAProxy setup looks solid! 🚀Good practices I'm seeing:
- Read-only volume mounts for configs and certs
- Proper service dependencies
- Standard web ports exposed
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 48-48: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
vsm.py (2)
96-128: Let's make the error responses more informative 🔍While the session validation is solid, we could make the API more user-friendly by providing specific error messages.
Consider this improvement:
if uuid is not None and rcon.get(uuid) is not None: user = json.loads(rcon.get(uuid)) else: - return Response(status=401) + return Response( + json.dumps({'error': 'No active session found'}), + status=401, + content_type='application/json' + ) if user['expires_at'] <= time.time(): - return Response(status=401) + return Response( + json.dumps({'error': 'Session has expired'}), + status=401, + content_type='application/json' + )
184-189: Consider a more thorough session cleanup 🧹While the basic session cleanup works, we could make it more robust.
Here's a suggestion:
def logout(): """UI - Logout""" rcon.delete(session['user']) session.pop('user', None) + session.clear() # Clear all session data return redirect(INDEX_PAGE, 302)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
vsm.py(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Building container
🔇 Additional comments (3)
vsm.py (3)
46-50: Nice work on the route structure! 👍The updated routes follow a clear and consistent pattern:
- Auth-related routes under
/auth/*- API endpoints under
/api/*This separation makes the API more intuitive and maintainable.
87-94: Thanks for extracting the common logic! 🎯Great job implementing the suggested refactor to eliminate code duplication. The helper function makes the code more maintainable.
153-153: Clean and simple! ✨Great use of the helper function here. It makes the code more readable and maintainable.
Make the API compatible with a React UI and keep the deprecated classic UI for testing purposes.
Summary by CodeRabbit
New Features
Refactor
Style