Authx to genrate url#1131
Conversation
WalkthroughThis pull request updates the UrbanPlannedVisitService by modifying the email-related functions. In the service, the database update marking feedback emails as sent is commented out, and the feedback form URL is now secured using a JWT with a three-day expiry. Additionally, the theme’s functions enqueue a new JavaScript dependency, Changes
Sequence Diagram(s)sequenceDiagram
participant B as Browser
participant JS as main.js
participant JWT as jwt_decode
B->>JS: Page load with URL hash (_authx token)
JS->>JWT: Decode _authx token
JWT-->>JS: Return decoded token (visit_id, exp)
JS->>B: Store visit_id in sessionStorage
JS->>B: Update URL hash to include visit_id
JS->>B: Restore _authx token after delay
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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: 1
🧹 Nitpick comments (2)
wp-content/themes/goonj-crm/main.js (1)
165-202: New token handling implementation looks good, but consider improving error logging.The self-invoking function for JWT token processing adds secure handling of visit_id via URL hash parameters. The implementation properly handles token decoding, session storage persistence, and URL manipulation.
The error handling at line 181 could be enhanced by logging additional details about the token that caused the error (without exposing sensitive data). This would help with debugging:
- console.error("JWT decode error:", err); + console.error("JWT decode error:", err.message, "Token format invalid");wp-content/civi-extensions/goonjcustom/Civi/UrbanPlannedVisitService.php (1)
921-926: Secure URL generation with JWT token - good security practice!The implementation of JWT token generation for the feedback form URL is a good security practice. The token includes a 3-day expiration time, which balances security with usability.
Consider extracting the JWT generation into a reusable helper method in case it needs to be used elsewhere:
private static function generateVisitToken($visitId, $expiryDays = 3) { $jwt = \Civi::service('crypto.jwt'); return $jwt->encode([ 'exp' => time() + (86400 * $expiryDays), 'visit_id' => $visitId, ]); }Then call it as:
$token = self::generateVisitToken($visitId);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
wp-content/civi-extensions/goonjcustom/Civi/UrbanPlannedVisitService.php(1 hunks)wp-content/themes/goonj-crm/functions.php(1 hunks)wp-content/themes/goonj-crm/main.js(1 hunks)
🔇 Additional comments (2)
wp-content/themes/goonj-crm/functions.php (2)
26-26: LGTM! Dependency declaration ensures proper script loading order.The addition of 'jwt-decode' as a dependency for 'goonj-script' ensures that the JWT library is loaded before the main script attempts to use it, preventing potential "undefined function" errors.
14-21:Details
❓ Verification inconclusive
External dependency introduced - ensure version pinning and security monitoring.
Adding jwt-decode from a CDN is appropriate for this use case. The script is properly added with null versioning, which is consistent with CDN usage.
Verify that the jwt-decode library version (3.1.2) is the latest stable version and doesn't have known vulnerabilities:
🏁 Script executed:
#!/bin/bash # Check for the latest version of jwt-decode curl -s https://registry.npmjs.org/jwt-decode | jq '.["dist-tags"].latest' # Check for known vulnerabilities curl -s https://registry.npmjs.org/jwt-decode | jq '.versions["3.1.2"].deprecated'Length of output: 174
Upgrade Reminder for the jwt-decode Dependency
It looks like the enqueueing of the jwt-decode script is implemented correctly, and using a CDN with a null version is consistent with common practices. However, note that while the code pins the version to 3.1.2, our verification shows the latest stable version is now 4.0.0. Although 3.1.2 isn’t deprecated or known to have vulnerabilities, please review the changelog and compatibility implications to determine if an upgrade to 4.0.0 would be beneficial.
- File: wp-content/themes/goonj-crm/functions.php (lines 14–21)
- Action: Verify if migrating from 3.1.2 to 4.0.0 aligns with your project’s compatibility requirements and risk profile, and ensure ongoing security monitoring for external dependencies.
| // Only for testing need to remove the comment code later. | ||
| // If ($emailSendResultToExternalPoc) { | ||
| // EckEntity::update('Institution_Visit', FALSE) | ||
| // ->addValue('Visit_Feedback.Feedback_Email_Sent', 1) | ||
| // ->addWhere('id', '=', $visit['id']) | ||
| // ->execute(); | ||
| // } |
There was a problem hiding this comment.
Remove commented-out code intended for temporary testing.
The code that updates the feedback email status has been commented out with a note indicating it's "Only for testing". Commented-out code reduces maintainability and creates confusion about whether this functionality should be enabled in production.
Either remove the commented code entirely if the functionality is no longer needed, or reinstate it if the functionality should be active in production. If this is a temporary state for debugging, add a TODO comment with a specific timeline:
- // Only for testing need to remove the comment code later.
- // If ($emailSendResultToExternalPoc) {
- // EckEntity::update('Institution_Visit', FALSE)
- // ->addValue('Visit_Feedback.Feedback_Email_Sent', 1)
- // ->addWhere('id', '=', $visit['id'])
- // ->execute();
- // }
+ // Track email sent status in database
+ if ($emailSendResultToExternalPoc) {
+ EckEntity::update('Institution_Visit', FALSE)
+ ->addValue('Visit_Feedback.Feedback_Email_Sent', 1)
+ ->addWhere('id', '=', $visit['id'])
+ ->execute();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Only for testing need to remove the comment code later. | |
| // If ($emailSendResultToExternalPoc) { | |
| // EckEntity::update('Institution_Visit', FALSE) | |
| // ->addValue('Visit_Feedback.Feedback_Email_Sent', 1) | |
| // ->addWhere('id', '=', $visit['id']) | |
| // ->execute(); | |
| // } | |
| // Track email sent status in database | |
| if ($emailSendResultToExternalPoc) { | |
| EckEntity::update('Institution_Visit', FALSE) | |
| ->addValue('Visit_Feedback.Feedback_Email_Sent', 1) | |
| ->addWhere('id', '=', $visit['id']) | |
| ->execute(); | |
| } |
1 participant
Authx to genrate url
Summary by CodeRabbit
New Features
Chores