feat(TV-57): implement altcha captcha #931
Conversation
…ub.com/it-at-m/eappointment into feature-tv-57-implement-altcha-captcha
WalkthroughThe changes refactor the Altcha CAPTCHA configuration across backend and frontend modules. Updates include renaming and replacing environment variables and class properties, transitioning from a secret key to a site secret along with new endpoints for challenge and verification. The AltchaCaptcha class and corresponding tests have been updated accordingly, and the CaptchaService now uses the new model. On the frontend, a new dependency is added; an API function, a Vue component, and relevant TypeScript interfaces/types are introduced. Additionally, the Vite configuration and JSON schema are modified to support custom elements and updated property names. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as User Interface
participant API as ZMSAppointmentAPI
participant Server as Captcha Server
UI->>API: Call fetchCaptchaDetails(baseUrl?)
API->>Server: HTTP GET (constructed URL using baseUrl + endpoint)
Server-->>API: Return JSON (CaptchaDetails | ErrorDTO)
API-->>UI: Return captcha details/error response
sequenceDiagram
participant User as End User
participant AltchaComp as Altcha.vue
participant Widget as <altcha-widget>
User->>AltchaComp: Interact with CAPTCHA widget
AltchaComp->>Widget: Listen for 'statechange' event
Widget-->>AltchaComp: Emit statechange (e.g., "verified")
AltchaComp->>AltchaComp: Update internalValue & emit update:payload
AltchaComp-->>User: Reflect updated payload via v-model binding
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. 🪧 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
Documentation and Community
|
![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 (4)
zmscitizenview/src/api/models/CaptchaDetails.ts (1)
1-41: Enhance JSDoc documentationThe interface itself is well-structured with proper typing. However, the JSDoc comments could be more descriptive by explaining the purpose of each property instead of just repeating the type information.
/** - * * @export * @interface CaptchaDetails + * Interface representing the data structure for Altcha CAPTCHA challenges */ export interface CaptchaDetails { /** - * + * The hashing algorithm used for the CAPTCHA challenge * @type {string} * @memberof CaptchaDetails */ algorithm: string; /** - * + * The CAPTCHA challenge string to be solved * @type {string} * @memberof CaptchaDetails */ challenge: string; /** - * + * The maximum number value used in the challenge (if applicable) * @type {number} * @memberof CaptchaDetails */ maxnumber?: number; /** - * + * Salt value used in the challenge generation * @type {string} * @memberof CaptchaDetails */ salt: string; /** - * + * Signature to verify the authenticity of the challenge * @type {string} * @memberof CaptchaDetails */ signature: string; }zmscitizenapi/README.md (1)
71-73: New environment variables correctly documented.The documentation has been properly updated to include the new environment variables for Altcha captcha. The variables are clearly described with their default values.
However, the bare URLs in the description should be formatted according to Markdown best practices.
Consider formatting the URLs using Markdown syntax:
-| ALTCHA_CAPTCHA_SITE_SECRET | Altcha site secret | "" | -| ALTCHA_CAPTCHA_ENDPOINT_CHALLENGE | Altcha challenge endpoint | https://captcha-k.muenchen.de/api/v1/captcha/challenge | -| ALTCHA_CAPTCHA_ENDPOINT_VERIFY | Altcha verification endpoint | https://captcha-k.muenchen.de/api/v1/captcha/verify | +| ALTCHA_CAPTCHA_SITE_SECRET | Altcha site secret | "" | +| ALTCHA_CAPTCHA_ENDPOINT_CHALLENGE | Altcha challenge endpoint | `https://captcha-k.muenchen.de/api/v1/captcha/challenge` | +| ALTCHA_CAPTCHA_ENDPOINT_VERIFY | Altcha verification endpoint | `https://captcha-k.muenchen.de/api/v1/captcha/verify` |🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
72-72: Bare URL used
null(MD034, no-bare-urls)
73-73: Bare URL used
null(MD034, no-bare-urls)
zmscitizenapi/src/Zmscitizenapi/Models/Captcha/AltchaCaptcha.php (2)
59-81: Consider implementing or removing commented codeThere's a large block of commented code for a potential requestCaptcha method. Consider either implementing this functionality if it's needed or removing the commented code to keep the codebase clean.
If this is intended for future implementation, consider adding a TODO comment explaining when this will be implemented, or move it to a separate development branch until ready.
86-97: verifyCaptcha method updated for new API contractThe verifyCaptcha method has been updated to use the new property names and API contract, changing from solution to payload parameter and including siteKey and siteSecret in the request.
Consider adding proper error logging when a RequestException occurs to help with troubleshooting:
} catch (RequestException $e) { + error_log('AltchaCaptcha verification failed: ' . $e->getMessage()); return false; }However, note that according to coding guidelines, error_log() should be replaced with a proper logging mechanism:
} catch (RequestException $e) { + // Using PSR-3 logger that should be injected + $this->logger->error('AltchaCaptcha verification failed', [ + 'error' => $e->getMessage(), + 'code' => $e->getCode() + ]); return false; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
zmscitizenview/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
zmscitizenapi/README.md(1 hunks)zmscitizenapi/src/Zmscitizenapi/Application.php(2 hunks)zmscitizenapi/src/Zmscitizenapi/Models/Captcha/AltchaCaptcha.php(2 hunks)zmscitizenapi/src/Zmscitizenapi/Services/Security/CaptchaService.php(2 hunks)zmscitizenapi/tests/Zmscitizenapi/ApplicationTest.php(3 hunks)zmscitizenapi/tests/Zmscitizenapi/Services/Security/CaptchaServiceTest.php(2 hunks)zmscitizenview/package.json(1 hunks)zmscitizenview/src/api/ZMSAppointmentAPI.ts(2 hunks)zmscitizenview/src/api/models/CaptchaDetails.ts(1 hunks)zmscitizenview/src/components/Appointment/Altcha.vue(1 hunks)zmscitizenview/src/components/Appointment/ServiceFinder.vue(3 hunks)zmscitizenview/src/types/AltchaTypes.ts(1 hunks)zmscitizenview/vite.config.ts(1 hunks)zmsentities/schema/citizenapi/captcha/altchaCaptcha.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.{js,jsx,ts,tsx}`: Flag any usage of console.log() as i...
**/*.{js,jsx,ts,tsx}: Flag any usage of console.log() as it should be replaced with proper logging:
- For development: Use proper debug tools or logging libraries
- For production: Remove console.log() statements or use structured logging
- For errors: Use error tracking services (e.g., Sentry)
- For debugging: Consider using debug libraries that can be enabled/disabled
Example replacement:
// Instead of: console.log('User data:', userData); // Use: logger.debug('Processing user data', { userData }); // or for development only: if (process.env.NODE_ENV === 'development') { debug('User data:', userData); }
zmscitizenview/vite.config.tszmscitizenview/src/api/models/CaptchaDetails.tszmscitizenview/src/api/ZMSAppointmentAPI.tszmscitizenview/src/types/AltchaTypes.ts
`**/*.php`: Flag any usage of error_log() as it should be re...
**/*.php: Flag any usage of error_log() as it should be replaced with proper logging mechanisms:
- For error handling: Use a proper logging framework with error levels (PSR-3 LoggerInterface)
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- For application events: Use structured logging with proper log levels
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $logger->error("Import failed", ['error' => $e->getMessage()]);
zmscitizenapi/tests/Zmscitizenapi/Services/Security/CaptchaServiceTest.phpzmscitizenapi/src/Zmscitizenapi/Services/Security/CaptchaService.phpzmscitizenapi/src/Zmscitizenapi/Application.phpzmscitizenapi/tests/Zmscitizenapi/ApplicationTest.phpzmscitizenapi/src/Zmscitizenapi/Models/Captcha/AltchaCaptcha.php
🧠 Learnings (1)
zmscitizenapi/src/Zmscitizenapi/Application.php (1)
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Application.php:57-77
Timestamp: 2025-01-08T09:53:23.015Z
Learning: CAPTCHA configuration must be validated when enabled to ensure all required environment variables are set and valid, including secret keys, site keys and endpoint URLs for both FriendlyCaptcha and AltchaCaptcha.
🪛 markdownlint-cli2 (0.17.2)
zmscitizenapi/README.md
72-72: Bare URL used
null
(MD034, no-bare-urls)
73-73: Bare URL used
null
(MD034, no-bare-urls)
🔇 Additional comments (27)
zmscitizenview/package.json (1)
17-17: Added new Altcha dependencyThe altcha package has been successfully added to the dependencies section which is needed for the CAPTCHA implementation.
zmscitizenview/vite.config.ts (1)
14-19: Vue configuration updated to support Altcha custom elementsAppropriate configuration has been added to recognize custom elements with the 'altcha-' prefix, which is required for the Altcha CAPTCHA component to function properly.
zmscitizenview/src/api/ZMSAppointmentAPI.ts (1)
242-250: Implementation of fetchCaptchaDetails looks goodThe new function follows the same pattern as other API functions in this file and properly handles the response.
zmscitizenview/src/components/Appointment/ServiceFinder.vue (3)
91-93: Good integration of the Altcha component.The Altcha component is properly integrated with appropriate margins and using v-model for two-way data binding.
114-114: LGTM: Component import is correctly placed.The Altcha component import is properly added to the imports section.
126-127: LGTM: Payload state initialization.The altchaPayload ref is correctly initialized with an empty string.
zmscitizenview/src/types/AltchaTypes.ts (3)
1-4: Well-structured interface for the Altcha state event detail.The AltchaStateEventDetail interface is well-typed with an optional payload and appropriate state literals.
6-6: LGTM: Proper extension of CustomEvent.The AltchaStateEvent interface correctly extends CustomEvent with the appropriate generic parameter.
8-17: Well-defined type for the Altcha widget.The AltchaWidget type properly extends HTMLElement with the necessary event listener methods, providing strong typing for the component interactions.
zmscitizenview/src/components/Appointment/Altcha.vue (4)
4-5: LGTM: Correct dependency import.The altcha package is properly imported, which makes the custom element available in the template.
7-21: LGTM: Well-implemented two-way binding.The component correctly sets up a two-way binding pattern using props, emits, and a watcher for the internal value.
23-32: Robust event handling for state changes.The onStateChange function correctly processes the event details and updates the internal value based on the verification state.
34-44: LGTM: Proper lifecycle management.The component correctly adds event listeners on mount and removes them on unmount to prevent memory leaks.
zmscitizenapi/tests/Zmscitizenapi/Services/Security/CaptchaServiceTest.php (2)
31-31: Test assertion updated correctly for the new property name.The assertion has been properly updated to check for the 'captchaVerify' key instead of 'captchaEndpoint', aligning with the changes made to the AltchaCaptcha model.
49-49: Assertion updated correctly to validate the new property.This assertion has been correctly modified to verify that the expected endpoint matches the 'captchaVerify' property value, which is consistent with the changes to the captcha implementation.
zmscitizenapi/src/Zmscitizenapi/Services/Security/CaptchaService.php (2)
7-7: Import statement updated correctly for the new captcha implementation.The import statement has been properly updated to use the AltchaCaptcha class instead of FriendlyCaptcha, reflecting the transition to the new captcha model.
16-18: Method signature and implementation properly updated.Both the return type and implementation of the getCaptchaDetails method have been updated to use AltchaCaptcha, ensuring consistency with the new captcha implementation.
zmsentities/schema/citizenapi/captcha/altchaCaptcha.json (3)
9-12: Property renamed and description updated properly.The property has been correctly renamed from 'captchaEndpoint' to 'captchaVerify' and its description has been updated to "Endpoint for captcha verification".
13-16: Property renamed and description updated properly.The property has been correctly renamed from 'puzzle' to 'captchaChallenge' and its description has been updated to "Challenge endpoint URL for AltchaCaptcha".
22-22: Required fields list updated correctly.The required fields list has been properly updated to reflect the renamed properties, ensuring the schema validation will work correctly with the new property names.
zmscitizenapi/tests/Zmscitizenapi/ApplicationTest.php (3)
102-104: Environment variables updated to match new Altcha CAPTCHA configurationThe test now sets up the new environment variables for Altcha CAPTCHA, replacing the old
ALTCHA_CAPTCHA_SECRET_KEYwithALTCHA_CAPTCHA_SITE_SECRETand adding specific endpoint variables for challenge and verification. This aligns with the updated implementation in the Application class.
114-116: Test assertions updated to verify new Altcha propertiesThe assertions correctly verify that the new Application properties are properly initialized from the corresponding environment variables, ensuring the config is correctly loaded.
232-234: Test teardown updated to clean environment variablesThe tearDown method now properly cleans up the new environment variables, which is important for test isolation.
zmscitizenapi/src/Zmscitizenapi/Application.php (1)
45-47: New Altcha CAPTCHA configuration properties addedThe static properties for Altcha CAPTCHA have been updated to align with the new implementation requirements, replacing the old secret key with site secret and adding specific endpoint properties for challenge and verification.
zmscitizenapi/src/Zmscitizenapi/Models/Captcha/AltchaCaptcha.php (3)
19-23: Properties updated to match new Altcha CAPTCHA implementationThe properties have been updated from apiUrl, secretKey, and puzzle to siteSecret, challengeUrl, and verifyUrl to align with the new Altcha CAPTCHA implementation.
31-33: Constructor updated to use new Application propertiesThe constructor now correctly initializes the class properties from the updated Application static properties.
53-54: Updated getCaptchaDetails with new property namesThe getCaptchaDetails method now returns the challenge and verify URLs with updated property names that match the schema changes.
Pull Request Checklist (Feature Branch to
next):nextBranch in meinen Feature-Branch gemergt.Summary by CodeRabbit
New Features
Documentation