Web of Trust Setup #305
Web of Trust Setup #305
Conversation
WalkthroughThe pull request introduces a new Web of Trust configuration feature for the application. The changes span multiple files in the frontend and backend, including the addition of a The Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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 (2)
frontend/src/common/backend.ts (1)
370-372: Consider adding specific error handling for settings updates.The
putmethod could benefit from explicit error handling to catch and convert specific HTTP status codes, similar to other methods in the codebase that userethrowAndConvertIfExpected.public async put(settings: SettingsDto): Promise<void> { - return axiosAuth.put('/settings', settings); + return axiosAuth.put('/settings', settings) + .catch((error) => rethrowAndConvertIfExpected(error, 400, 403)); }frontend/src/components/AdminSettings.vue (1)
373-396: Improve the save operation implementation.Several improvements can be made to the save operation:
- Remove the artificial delay (
setTimeout)- Add success feedback
- Consider disabling inputs during processing
async function saveWebOfTrust() { onSaveError.value = null; trustLevelError.value = trustLevel.value < 0 || trustLevel.value > 10; maxChainLengthError.value = maxChainLength.value < 0; if (trustLevelError.value || maxChainLengthError.value) { return; } try { processing.value = true; const settings = { wotMaxDepth: trustLevel.value, wotIdVerifyLen: maxChainLength.value, hubId: admin.value?.hubId ?? '' }; await backend.settings.put(settings); - await new Promise(resolve => setTimeout(resolve, 500)); + // Show success message + const toast = useToast(); + toast.success(t('admin.webOfTrust.saveSuccess')); } catch (error) { console.error('Failed to save settings:', error); onSaveError.value = error instanceof Error ? error : new Error('Unknown reason'); } finally { processing.value = false; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/src/common/backend.ts(1 hunks)frontend/src/components/AdminSettings.vue(5 hunks)frontend/src/i18n/de-DE.json(1 hunks)frontend/src/i18n/en-US.json(1 hunks)
|
|
||
| <div class="col-span-6 sm:col-span-3"> | ||
| <label for="trustLevel" class="block text-sm font-medium text-gray-700">{{ t('admin.webOfTrust.trustLevel.title') }}</label> | ||
| <input id="trustLevel" v-model="trustLevel" type="number" min="0" max="10" step="1" class="mt-1 block w-full shadow-sm sm:text-sm rounded-md border-gray-300 focus:ring-primary focus:border-primary [appearance:textfield] [&::-webkit-outer-spin-button]:appearance-none [&::-webkit-inner-spin-button]:appearance-none" :class="{ 'invalid:border-red-300 invalid:text-red-900 focus:invalid:ring-red-500 focus:invalid:border-red-500': trustLevelError }" /> | ||
| <p v-if="trustLevelError" class="mt-1 text-sm text-red-900"> | ||
| {{ t('admin.webOfTrust.trustLevel.error') }} | ||
| </p> | ||
| </div> | ||
|
|
||
| <div class="col-span-6 sm:col-span-3"> | ||
| <label for="maxChainLength" class="block text-sm font-medium text-gray-700">{{ t('admin.webOfTrust.maxChainLength.title') }}</label> | ||
| <input id="maxChainLength" v-model="maxChainLength" type="number" min="0" step="1" class="mt-1 block w-full shadow-sm sm:text-sm rounded-md border-gray-300 focus:ring-primary focus:border-primary [appearance:textfield] [&::-webkit-outer-spin-button]:appearance-none [&::-webkit-inner-spin-button]:appearance-none" :class="{ 'invalid:border-red-300 invalid:text-red-900 focus:invalid:ring-red-500 focus:invalid:border-red-500': maxChainLengthError }" /> | ||
| <p v-if="maxChainLengthError" class="mt-1 text-sm text-red-900"> | ||
| {{ t('admin.webOfTrust.maxChainLength.error') }} | ||
| </p> | ||
| </div> | ||
|
|
||
| <div class="col-span-6 flex justify-end space-x-3"> | ||
| <button type="button" class="inline-flex justify-center py-2 px-4 border border-transparent shadow-sm text-sm font-medium rounded-md text-white bg-primary hover:bg-primary-d1 focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-primary" @click="saveWebOfTrust"> | ||
| {{ t('admin.webOfTrust.save') }} | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
Remove duplicate Web of Trust section.
This section is duplicated. The same content appears again at lines 200-242. Remove this instance and keep the one with better error handling and processing state management.
| "admin.webOfTrust.description": "Configure the Web of Trust settings to manage how trust is established between users in the system. These settings affect the verification of user identities and key signatures.", | ||
| "admin.webOfTrust.trustLevel.error": "Trust level must be between 0 and 10.", | ||
| "admin.webOfTrust.trustLevel.title": "Trust Level", | ||
| "admin.webOfTrust.maxChainLength.error": "Maximum chain length must be greater than 0.", |
There was a problem hiding this comment.
Fix inconsistency in error message.
The error message states "must be greater than 0" but the validation in AdminSettings.vue checks for "greater than or equal to 0" (value < 0). Either update the message or the validation to match.
Choose one of these approaches:
- If zero is allowed:
- "admin.webOfTrust.maxChainLength.error": "Maximum chain length must be greater than 0.",
+ "admin.webOfTrust.maxChainLength.error": "Maximum chain length must not be negative.",- If zero is not allowed (recommended for chain length):
// In AdminSettings.vue
-maxChainLengthError.value = maxChainLength.value < 0;
+maxChainLengthError.value = maxChainLength.value <= 0;📝 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.
| "admin.webOfTrust.maxChainLength.error": "Maximum chain length must be greater than 0.", | |
| "admin.webOfTrust.maxChainLength.error": "Maximum chain length must not be negative.", |
| "admin.webOfTrust.description": "Konfigurieren Sie die Web of Trust-Einstellungen, um zu verwalten, wie Vertrauen zwischen Benutzern im System aufgebaut wird. Diese Einstellungen beeinflussen die Überprüfung von Benutzeridentitäten und Schlüssel-Signaturen.", | ||
| "admin.webOfTrust.trustLevel.error": "Das Vertrauensniveau muss zwischen 0 und 10 liegen.", | ||
| "admin.webOfTrust.trustLevel.title": "Vertrauensniveau", | ||
| "admin.webOfTrust.maxChainLength.error": "Die maximale Kettenlänge muss größer als 0 sein.", |
There was a problem hiding this comment.
Fix inconsistency in German error message.
The same inconsistency exists in the German translation. Update it to match the English version once the validation logic is decided.
Choose one of these approaches based on the decision made for the English translation:
- If zero is allowed:
- "admin.webOfTrust.maxChainLength.error": "Die maximale Kettenlänge muss größer als 0 sein.",
+ "admin.webOfTrust.maxChainLength.error": "Die maximale Kettenlänge darf nicht negativ sein.",- If zero is not allowed (recommended for chain length):
// Keep the current message as it's already correct for this caseCommittable suggestion skipped: line range outside the PR's diff.
| // Add validation functions | ||
| function validateTrustLevel(value: number) { | ||
| return value < 0 || value > 10; | ||
| } | ||
|
|
||
| function validateMaxChainLength(value: number) { | ||
| return value < 0; | ||
| } | ||
|
|
||
| class FormValidationFailedError extends Error { | ||
| constructor() { | ||
| super('The form is invalid.'); | ||
| } | ||
| } |
There was a problem hiding this comment.
form validation isn't yet hooked up to the HTML, is it?
|
|
||
| <div v-if="admin.hasLicense" class="flex justify-end items-center"> | ||
| <button type="button" class="flex-none inline-flex justify-center py-2 px-4 border border-transparent shadow-sm text-sm font-medium rounded-md text-white bg-primary hover:bg-primary-d1 focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-primary disabled:opacity-50 disabled:hover:bg-primary disabled:cursor-not-allowed" @click="manageSubscription()"> | ||
| <ArrowTopRightOnSquareIcon class="-ml-1 mr-2 h-5 w-5" aria-hidden="true" /> | ||
| {{ t('admin.licenseInfo.manageSubscription') }} | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
probably deleted accidentally?
| {{ t('admin.webOfTrust.description') }} | ||
| </p> | ||
| </div> | ||
| <div class="mt-5 md:mt-0 md:col-span-2"> |
There was a problem hiding this comment.
div can be a form, so the contained input fields and buttons are semantically grouped. Furthermore form validation might be easier this way (triggered by submit event instead of click event - so pressing enter works as well)
| const trustLevel = ref<number>(0); | ||
| const maxChainLength = ref<number>(0); |
There was a problem hiding this comment.
use the same names as in the database and backend (i.e. wotMaxDepth and wotIdVerifyLen) to avoid confusion. Especially since we have a trust level in other places, which has a completely different meaning.
| "admin.webOfTrust.trustLevel.error": "Trust level must be between 0 and 10.", | ||
| "admin.webOfTrust.trustLevel.title": "Trust Level", | ||
| "admin.webOfTrust.maxChainLength.error": "Maximum chain length must be greater than 0.", | ||
| "admin.webOfTrust.maxChainLength.title": "Maximum Chain Length", |
There was a problem hiding this comment.
Please adjust the names. Furthermore, we probably need to explain the meaning of both values, so you should add some description field.
| "admin.webOfTrust.trustLevel.error": "Trust level must be between 0 and 10.", | |
| "admin.webOfTrust.trustLevel.title": "Trust Level", | |
| "admin.webOfTrust.maxChainLength.error": "Maximum chain length must be greater than 0.", | |
| "admin.webOfTrust.maxChainLength.title": "Maximum Chain Length", | |
| "admin.webOfTrust.wotMaxDepth.error": "Maximum WoT Depth must be between 0 and 10.", | |
| "admin.webOfTrust.wotMaxDepth.title": "Maximum WoT Depth", | |
| "admin.webOfTrust.wotMaxDepth.description": "Maximum distance between two users, so an indirect acquaintanceship is still considered trustworthy (assuming intermediate identities have been verified).", | |
| "admin.webOfTrust.wotIdVerifyLen.error": "Fingerprint Verification Preciseness must be positive.", | |
| "admin.webOfTrust.wotIdVerifyLen.title": "Fingerprint Verification Preciseness", | |
| "admin.webOfTrust.wotIdVerifyLen.description": "How many digits of another users fingerprint one needs to enter to verify their identity.", |
Co-authored-by: iammajid <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/src/main/java/org/cryptomator/hub/entities/events/SettingWotUpdateEvent.java (1)
21-25: Add validation constraints for numeric fields.Consider adding validation annotations to ensure the numeric fields have appropriate bounds:
@Column(name = "wot_max_depth") +@Min(0) @Max(9) private int wotMaxDepth; @Column(name = "wot_id_verify_len") +@Min(0) private int wotIdVerifyLen;backend/src/main/resources/org/cryptomator/hub/flyway/V17__Create_Audit_Event_Setting_WoT.sql (1)
1-6: Consider adding CHECK constraints for the numeric columns.The table structure looks good with appropriate column types and NOT NULL constraints. However, consider adding CHECK constraints to ensure
wot_max_depthandwot_id_verify_lenare positive integers, as negative values wouldn't make sense for these settings.CREATE TABLE "audit_event_setting_wot_update" ( "id" BIGINT NOT NULL, "updated_by" VARCHAR(255) COLLATE "C" NOT NULL, - "wot_max_depth" INTEGER NOT NULL, - "wot_id_verify_len" INTEGER NOT NULL, + "wot_max_depth" INTEGER NOT NULL CHECK ("wot_max_depth" > 0), + "wot_id_verify_len" INTEGER NOT NULL CHECK ("wot_id_verify_len" > 0),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/src/main/java/org/cryptomator/hub/api/AuditLogResource.java(4 hunks)backend/src/main/java/org/cryptomator/hub/api/SettingsResource.java(2 hunks)backend/src/main/java/org/cryptomator/hub/entities/events/EventLogger.java(1 hunks)backend/src/main/java/org/cryptomator/hub/entities/events/SettingWotUpdateEvent.java(1 hunks)backend/src/main/resources/org/cryptomator/hub/flyway/V17__Create_Audit_Event_Setting_WoT.sql(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test
🔇 Additional comments (5)
backend/src/main/java/org/cryptomator/hub/entities/events/SettingWotUpdateEvent.java (1)
1-63: LGTM! Well-structured event entity.The implementation follows JPA entity best practices with proper inheritance, field mapping, and equals/hashCode implementation.
backend/src/main/java/org/cryptomator/hub/api/SettingsResource.java (1)
56-63: LGTM! Proper change detection and event logging.The implementation correctly tracks and logs WoT setting changes only when values actually change.
backend/src/main/java/org/cryptomator/hub/entities/events/EventLogger.java (1)
103-110: LGTM! Consistent logging implementation.The new logging method follows the established pattern and properly sets all required fields.
backend/src/main/java/org/cryptomator/hub/api/AuditLogResource.java (1)
85-85: LGTM! Proper audit event integration.The implementation correctly integrates the new WoT settings update event into the audit system:
- Proper JsonSubTypes registration
- Well-structured DTO record
- Correct conversion logic in fromEntity
Also applies to: 109-109, 133-134
backend/src/main/resources/org/cryptomator/hub/flyway/V17__Create_Audit_Event_Setting_WoT.sql (1)
7-9: LGTM! Well-structured constraints.The constraints are properly defined:
- Primary key ensures unique audit events
- Foreign key with CASCADE delete ensures referential integrity and automatic cleanup of audit records when the parent event is deleted
Added a Web of Trust section in the Admin Settings to allow administrators to set values for Trust Level (wot_max_depth) and Maximum Chain Length (wot_id_verify_len). This addresses Issue #297.