General: Display modal to setup passkeys after login
#10766
Conversation
github-actions
bot
added
server
WalkthroughThis update introduces a new mechanism to track whether a user has registered a passkey, extending both backend and frontend data models to include this information. The backend adds a boolean field to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HomeComponent
participant AccountService
participant NgbModal
participant SetupPasskeyModalComponent
User->>HomeComponent: Login
HomeComponent->>AccountService: Authenticate user
AccountService-->>HomeComponent: Return user info (hasRegisteredAPasskey)
HomeComponent->>HomeComponent: handleLoginSuccess()
HomeComponent->>HomeComponent: openSetupPasskeyModal()
alt Passkey feature enabled AND !hasRegisteredAPasskey AND no reminder
HomeComponent->>NgbModal: open(SetupPasskeyModalComponent)
NgbModal-->>SetupPasskeyModalComponent: Display modal
User->>SetupPasskeyModalComponent: Choose action (Setup/Remind Later/Close)
alt Setup
SetupPasskeyModalComponent->>NgbModal: close()
SetupPasskeyModalComponent->>Router: navigate(/user-settings/passkeys)
else Remind Me in 30 Days
SetupPasskeyModalComponent->>localStorage: set reminder date
SetupPasskeyModalComponent->>NgbModal: close()
else Set Up Later/Close
SetupPasskeyModalComponent->>NgbModal: close()
end
end
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 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 (9)
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 2
🧹 Nitpick comments (6)
src/main/webapp/app/core/course/overview/setup-passkey-modal/setup-passkey-modal.component.spec.ts (1)
1-22: Base unit test added; consider expanding coverage
Good start verifying the component instantiation. To improve test robustness and meet meaningful coverage guidelines, consider adding specs that simulate user interactions with the modal—e.g., opening/closing logic, button clicks, and reactive signal updates.src/main/webapp/app/core/user/user.model.ts (1)
42-42: Property naming inconsistency between parameter and class property.The parameter name
hasRegisteredPasskeys(plural) differs from the property namehasRegisteredAPasskey(singular), which could lead to confusion.Consider making the naming consistent between parameter and class property:
- hasRegisteredPasskeys?: boolean, + hasRegisteredAPasskey?: boolean,And then:
- this.hasRegisteredAPasskey = hasRegisteredPasskeys; + this.hasRegisteredAPasskey = hasRegisteredAPasskey;Also applies to: 55-55
src/main/webapp/app/core/auth/account.service.ts (1)
53-53: Consider tracking this TODO in your issue system.While the TODO comment provides good documentation for future refactoring, it might be beneficial to create a formal issue or task to ensure this work isn't forgotten.
src/main/java/de/tum/cit/aet/artemis/core/dto/UserDTO.java (1)
243-249: Getter and setter methods implemented correctly.The methods follow Java conventions and naming standards. However, the setter parameter name
hasCreatedPasskeysdiffers from the field namehasRegisteredAPasskey, which could cause confusion.Consider renaming the parameter to match the field name for consistency:
-public void setHasRegisteredAPasskey(boolean hasCreatedPasskeys) { - this.hasRegisteredAPasskey = hasCreatedPasskeys; +public void setHasRegisteredAPasskey(boolean hasRegisteredAPasskey) { + this.hasRegisteredAPasskey = hasRegisteredAPasskey; }src/main/java/de/tum/cit/aet/artemis/core/web/open/PublicAccountResource.java (1)
169-170: Variable name could better match field name.The variable name
hasUserRegisteredAPasskeyis similar but not identical to the DTO field namehasRegisteredAPasskey, which could potentially cause confusion.Consider renaming for consistency:
-boolean hasUserRegisteredAPasskey = this.passkeyCredentialsRepository.existsByUserId(user.getId()); +boolean hasRegisteredAPasskey = this.passkeyCredentialsRepository.existsByUserId(user.getId());src/main/webapp/app/app.component.ts (1)
79-82: Consider a more explicit approach to detect login screenThe current approach infers the user is on the login screen when they're not authenticated, which works but could be more explicit.
- const isUserOnLoginScreen = !this.accountService.isAuthenticatedSignal(); + // Check if user is authenticated (not on login screen) + const isAuthenticated = this.accountService.isAuthenticatedSignal(); + if (!isAuthenticated) { - if (isUserOnLoginScreen) { return; }This makes the intention clearer - we're checking if the user is authenticated rather than inferring they're on the login screen.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
src/main/java/de/tum/cit/aet/artemis/core/dto/UserDTO.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/core/dto/passkey/ArtemisPublicKeyCredentialParametersDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/repository/PasskeyCredentialsRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/repository/passkey/ArtemisPublicKeyCredentialUserEntityRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/repository/passkey/ArtemisUserCredentialRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/repository/passkey/COSEAlgorithm.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/repository/passkey/HazelcastHttpSessionPublicKeyCredentialCreationOptionsRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/repository/passkey/HazelcastPublicKeyCredentialRequestOptionsRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/security/passkey/ArtemisPublicKeyCredentialCreationOptionsFilter.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/security/passkey/ArtemisWebAuthnConfigurer.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/PasskeyResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/open/PublicAccountResource.java(3 hunks)src/main/webapp/app/app.component.ts(5 hunks)src/main/webapp/app/core/auth/account.service.ts(2 hunks)src/main/webapp/app/core/course/overview/courses/courses.component.ts(1 hunks)src/main/webapp/app/core/course/overview/setup-passkey-modal/setup-passkey-modal.component.html(1 hunks)src/main/webapp/app/core/course/overview/setup-passkey-modal/setup-passkey-modal.component.spec.ts(1 hunks)src/main/webapp/app/core/course/overview/setup-passkey-modal/setup-passkey-modal.component.ts(1 hunks)src/main/webapp/app/core/navbar/navbar.component.ts(1 hunks)src/main/webapp/app/core/user/settings/passkey-settings/passkey-settings.component.html(0 hunks)src/main/webapp/app/core/user/user.model.ts(3 hunks)src/main/webapp/app/shared/delete-dialog/component/delete-dialog.component.ts(1 hunks)src/main/webapp/app/shared/exercise-filter/exercise-filter-modal.component.ts(2 hunks)src/main/webapp/i18n/de/userSettings.json(2 hunks)src/main/webapp/i18n/en/userSettings.json(2 hunks)
💤 Files with no reviewable changes (1)
- src/main/webapp/app/core/user/settings/passkey-settings/passkey-settings.component.html
🧰 Additional context used
📓 Path-based instructions (4)
`src/main/java/**/*.java`: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,de...
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/core/repository/passkey/ArtemisUserCredentialRepository.javasrc/main/java/de/tum/cit/aet/artemis/core/dto/passkey/ArtemisPublicKeyCredentialParametersDTO.javasrc/main/java/de/tum/cit/aet/artemis/core/repository/passkey/HazelcastHttpSessionPublicKeyCredentialCreationOptionsRepository.javasrc/main/java/de/tum/cit/aet/artemis/core/repository/passkey/HazelcastPublicKeyCredentialRequestOptionsRepository.javasrc/main/java/de/tum/cit/aet/artemis/core/security/passkey/ArtemisWebAuthnConfigurer.javasrc/main/java/de/tum/cit/aet/artemis/core/security/passkey/ArtemisPublicKeyCredentialCreationOptionsFilter.javasrc/main/java/de/tum/cit/aet/artemis/core/repository/passkey/ArtemisPublicKeyCredentialUserEntityRepository.javasrc/main/java/de/tum/cit/aet/artemis/core/repository/PasskeyCredentialsRepository.javasrc/main/java/de/tum/cit/aet/artemis/core/web/PasskeyResource.javasrc/main/java/de/tum/cit/aet/artemis/core/repository/passkey/COSEAlgorithm.javasrc/main/java/de/tum/cit/aet/artemis/core/dto/UserDTO.javasrc/main/java/de/tum/cit/aet/artemis/core/web/open/PublicAccountResource.java
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalC...
src/main/webapp/app/core/course/overview/setup-passkey-modal/setup-passkey-modal.component.spec.tssrc/main/webapp/app/core/course/overview/courses/courses.component.tssrc/main/webapp/app/core/user/user.model.tssrc/main/webapp/app/shared/exercise-filter/exercise-filter-modal.component.tssrc/main/webapp/app/shared/delete-dialog/component/delete-dialog.component.tssrc/main/webapp/app/app.component.tssrc/main/webapp/app/core/auth/account.service.tssrc/main/webapp/app/core/course/overview/setup-passkey-modal/setup-passkey-modal.component.tssrc/main/webapp/app/core/navbar/navbar.component.ts
`src/main/webapp/i18n/de/**/*.json`: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/...
src/main/webapp/i18n/de/**/*.json: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".
src/main/webapp/i18n/de/userSettings.json
`src/main/webapp/**/*.html`: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/core/course/overview/setup-passkey-modal/setup-passkey-modal.component.html
🧬 Code Graph Analysis (2)
src/main/webapp/app/app.component.ts (1)
src/main/webapp/app/app.constants.ts (1)
FEATURE_PASSKEY(40-40)
src/main/webapp/app/core/course/overview/setup-passkey-modal/setup-passkey-modal.component.ts (1)
src/main/webapp/app/app.component.ts (1)
Component(23-185)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: Analyse
- GitHub Check: client-style
- GitHub Check: client-tests
- GitHub Check: client-tests-selected
- GitHub Check: server-tests
- GitHub Check: server-style
🔇 Additional comments (42)
src/main/webapp/app/core/course/overview/courses/courses.component.ts (1)
42-45: Appropriate icon property declarationThe FontAwesome icon properties have been moved to appear before the injected services, improving the component structure by grouping similar declarations together. The use of
protected readonlyis appropriate for template-accessible properties that shouldn't be modified.src/main/java/de/tum/cit/aet/artemis/core/repository/PasskeyCredentialsRepository.java (1)
29-34: Well-optimized query for checking user passkey existenceThe added method efficiently checks for passkey existence using a direct boolean return value rather than fetching and counting all records. The query follows Java and SQL best practices with:
- Proper parameter annotation
- SQL keywords in uppercase
- Clear and concise method naming
- Efficient query design (using
COUNT > 0pattern)This approach is more performant than retrieving the full list of credentials and checking its size.
src/main/webapp/app/shared/exercise-filter/exercise-filter-modal.component.ts (3)
3-3: Icon import cleanupThe import statement has been simplified to only include the
faFiltericon, removing the unnecessaryfaBackwardicon import.
36-36: Appropriate icon accessibilityChanged the
faFiltericon from public toprotected readonly, which is the appropriate access level for template-only properties and prevents accidental mutation.
38-38: Proper service injectionThe
activeModalinjection has been repositioned within the class while maintaining its private access modifier, following Angular's best practices for dependency injection using theinject()function.src/main/webapp/app/shared/delete-dialog/component/delete-dialog.component.ts (1)
22-27: Improved icon property declarationsThe FontAwesome icon properties have been changed from public instance fields to
protected readonlyclass properties, which:
- Prevents accidental mutation of icon references
- Provides appropriate access level for template-only properties
- Maintains consistency with similar icon declarations across the codebase
- Follows Angular best practices for component properties
This change aligns with the pattern seen in other components in this PR.
src/main/java/de/tum/cit/aet/artemis/core/repository/passkey/COSEAlgorithm.java (1)
1-1: Package relocation updated for COSEAlgorithm
The package declaration has been moved tode.tum.cit.aet.artemis.core.repository.passkey. Ensure that all references and import statements in related classes and DTOs have been updated to this new path.src/main/java/de/tum/cit/aet/artemis/core/dto/passkey/ArtemisPublicKeyCredentialParametersDTO.java (1)
13-13: Import path updated to newpasskeypackage
TheCOSEAlgorithmimport now reflects its relocation underde.tum.cit.aet.artemis.core.repository.passkey. No further action is required here.src/main/java/de/tum/cit/aet/artemis/core/repository/passkey/ArtemisUserCredentialRepository.java (1)
1-1: Relocated repository topasskeypackage
The package declaration has been updated tode.tum.cit.aet.artemis.core.repository.passkey. Confirm that any dependent imports (e.g., in security configuration or resource classes) have been adjusted accordingly.src/main/java/de/tum/cit/aet/artemis/core/repository/passkey/ArtemisPublicKeyCredentialUserEntityRepository.java (1)
1-1: Repository package updated for passkey functionality
The package has been moved tode.tum.cit.aet.artemis.core.repository.passkey. Verify there are no leftover references to the oldwebauthnpackage in your codebase.src/main/java/de/tum/cit/aet/artemis/core/repository/passkey/HazelcastPublicKeyCredentialRequestOptionsRepository.java (1)
1-1:✅ Verification successful
Verify package rename consistency
The class has been moved to thepasskeypackage. Please ensure that there are no remaining imports or references to the oldwebauthnpackage for this class anywhere in the codebase.
🏁 Script executed:
#!/bin/bash # Verify no stale imports from old package rg -n 'de\.tum\.cit\.aet\.artemis\.core\.repository\.webauthn\.HazelcastPublicKeyCredentialRequestOptionsRepository'Length of output: 118
🏁 Script executed:
#!/bin/bash # Broad search for any remaining imports or references to the old `webauthn` package rg -n 'de\.tum\.cit\.aet\.artemis\.core\.repository\.webauthn\.' -t javaLength of output: 74
No stale
webauthnimports detected
A repository‐wide search forde.tum.cit.aet.artemis.core.repository.webauthn.returned no results, confirming the package rename is consistent.src/main/java/de/tum/cit/aet/artemis/core/repository/passkey/HazelcastHttpSessionPublicKeyCredentialCreationOptionsRepository.java (1)
1-1:✅ Verification successful
Verify package relocation for creation options repository
The package declaration was updated topasskey. Confirm that all Javadoc links and import statements referring to this repository class in other modules (e.g., security config, filters) have been updated accordingly.
🏁 Script executed:
#!/bin/bash # Search for references to the old package location rg -n 'de\.tum\.cit\.aet\.artemis\.core\.repository\.webauthn\.HazelcastHttpSessionPublicKeyCredentialCreationOptionsRepository'Length of output: 130
🏁 Script executed:
#!/bin/bash # Search for all occurrences of the relocated repository class (imports, Javadoc links, usage) rg -n 'HazelcastHttpSessionPublicKeyCredentialCreationOptionsRepository'Length of output: 1626
All references correctly updated to the new package location
The Javadoc links in both security classes now point to
de.tum.cit.aet.artemis.core.repository.passkey.HazelcastHttpSessionPublicKeyCredentialCreationOptionsRepository, so no further changes are needed:
- src/main/java/de/tum/cit/aet/artemis/core/security/passkey/ArtemisPublicKeyCredentialCreationOptionsFilter.java (Javadoc link at line 46)
- src/main/java/de/tum/cit/aet/artemis/core/security/passkey/ArtemisWebAuthnConfigurer.java (Javadoc link at line 42)
src/main/java/de/tum/cit/aet/artemis/core/web/PasskeyResource.java (1)
27-27: Import updated to new package
The import forArtemisUserCredentialRepositoryhas been correctly updated to thepasskeypackage. No further action needed here.src/main/java/de/tum/cit/aet/artemis/core/security/passkey/ArtemisWebAuthnConfigurer.java (1)
42-43: Javadoc links corrected for custom repositories
The Javadoc now references the relocated Hazelcast repositories in thepasskeypackage. This aligns with the broader refactoring and documentation is now consistent.src/main/java/de/tum/cit/aet/artemis/core/security/passkey/ArtemisPublicKeyCredentialCreationOptionsFilter.java (1)
46-46: Package path update correctly reflects refactoring.The updated Javadoc reference to the repository class aligns with the broader refactoring where passkey-related classes were moved from the
webauthnpackage to the more appropriately namedpasskeypackage.src/main/webapp/app/core/user/user.model.ts (1)
18-21: Well-documented property addition.The added property is properly documented with a clear description of its purpose.
src/main/webapp/i18n/de/userSettings.json (2)
88-88: Improved passkey description.The updated description is more concise and focuses on the security benefits of passkeys, which helps users better understand their value.
109-117: Well-structured translations for the new modal component.The added translations for the passkey setup modal properly use the informal "du" form as required in the German translation guidelines. All necessary keys for the modal component are present with clear and informative messaging.
src/main/webapp/app/core/course/overview/setup-passkey-modal/setup-passkey-modal.component.html (1)
1-29: Well-structured modal with proper accessibility and localization.The modal template is well-organized with clear sections and proper accessibility attributes. It correctly uses the jhiTranslate directive for all text content and includes appropriate icon indicators.
Two minor improvements to consider:
- Add
type="submit"to the primary button since the form has an ngSubmit handler- The form element isn't necessary since you're using click handlers rather than form submission
-<form name="exerciseFilterForm" (ngSubmit)="navigateToSetupPasskey()"> +<div> ... - <button type="button" class="btn btn-primary" (click)="navigateToSetupPasskey()"> + <button type="submit" class="btn btn-primary" (click)="navigateToSetupPasskey()"> ... -</form> +</div>src/main/webapp/app/core/auth/account.service.ts (3)
1-1: Good job updating imports to include signal.The import statement correctly includes the
signalfunction from@angular/core, which aligns with modern Angular practices.
51-51: Well-structured reactive signal for authentication state.The use of signals for tracking authentication state is a good approach, making this state reactive and readily consumable by components. This follows Angular's recommended patterns for state management.
60-64: Good implementation of reactive signal updates.The code properly updates both the traditional boolean flag and the new signal when authentication state changes. The variable naming is clear and descriptive.
src/main/java/de/tum/cit/aet/artemis/core/dto/UserDTO.java (1)
74-78: Good addition of passkey registration field with documentation.The new field is properly documented with a clear comment explaining its purpose. The default value of
falseis appropriate as the absence of passkeys is the default state.src/main/webapp/app/core/course/overview/setup-passkey-modal/setup-passkey-modal.component.ts (6)
1-8: Imports are clean and well-organized.The imports are appropriately organized and include only what's needed for the component functionality.
9-14: Component setup follows Angular best practices.The component uses the standalone component pattern with explicit imports, which is a modern Angular approach. The template and style file references are correct.
15-18: Icon declarations are properly structured.The FontAwesome icons are declared as protected readonly properties, making them accessible to the template while preventing modification.
19-20: Good use of modern dependency injection.Using
inject()for dependency injection follows current Angular best practices.
22-25: Navigation method is clear and focused.The method correctly closes the modal before navigation, preventing UI issues. The path to the passkey setup page is correctly specified.
27-29: Modal close method is simple and effective.Good separation of concerns with a dedicated method for closing the modal.
src/main/java/de/tum/cit/aet/artemis/core/web/open/PublicAccountResource.java (4)
40-40: Added import for PasskeyCredentialsRepository.Appropriate import for the new repository dependency.
68-69: Good use of constructor injection for new repository.Following the dependency injection principle by using constructor injection for the repository.
70-77: Constructor properly initializes all dependencies.The constructor has been updated to include and initialize the new repository dependency.
175-175: Properly sets passkey registration status on UserDTO.This correctly transfers the passkey registration status from the repository check to the UserDTO being returned to the client.
src/main/webapp/i18n/en/userSettings.json (2)
88-88: Good concise description of passkeysThe revised text focuses on the security benefits of passkeys without being overly technical, making it more accessible to users.
109-117: Well-structured modal content with clear user guidanceThe new passkey modal text is well-organized with:
- A clear title that emphasizes security
- An explanation of how passkeys work and their benefits
- A recommendation that encourages adoption without being too pushy
- Appropriately labeled buttons that give users a choice
This content aligns perfectly with the PR objective of prompting users to set up passkeys after login.
src/main/webapp/app/app.component.ts (7)
1-1: Appropriate import of Angular reactive primitivesThe addition of
effectandinjectis necessary for the reactive approach used to manage the passkey modal display.
18-21: Well-organized imports for the passkey featureAll necessary dependencies for the passkey modal functionality are properly imported.
42-43: Services properly injected using the recommended inject patternFollowing Angular's recommended dependency injection pattern with the
inject()function.
61-61: Feature flag initializationGood practice to initialize the feature flag as disabled by default.
94-96: Good use of Angular's effect() for reactive modal displayUsing Angular's reactive
effect()ensures the modal logic runs at the appropriate times - both initially and whenever the tracked dependencies change.
114-114: Proper initialization of feature flag in ngOnInitThe code correctly initializes the
isPasskeyEnabledflag by checking if the feature is active using the profile service.
63-89: 🛠️ Refactor suggestionWell-documented and implemented method for conditionally displaying the passkey modal
The
openSetupPasskeyModal()method is:
- Thoroughly documented with JSDoc comments explaining the purpose and conditions
- Properly checks all necessary conditions before showing the modal
- Uses clean, early-return patterns for improved readability
However, there's a potential null reference issue that should be addressed.
- if (!this.accountService.userIdentity || this.accountService.userIdentity.hasRegisteredAPasskey) { + const { userIdentity } = this.accountService; + if (!userIdentity || userIdentity.hasRegisteredAPasskey) { return; }This makes the code safer by avoiding a potential null reference when accessing the
hasRegisteredAPasskeyproperty.Likely an incorrect or invalid review comment.
github-project-automation
bot
moved this from Work In Progress
to Ready For Review
in Artemis Development
src/main/webapp/app/shared/exercise-filter/exercise-filter-modal.component.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
gradle/jacoco.gradle(1 hunks)src/main/webapp/app/app.component.spec.ts(2 hunks)src/main/webapp/app/app.component.ts(2 hunks)src/main/webapp/app/core/admin/system-notification-management/system-notification-detail.component.spec.ts(1 hunks)src/main/webapp/app/core/auth/account.service.ts(1 hunks)src/main/webapp/app/core/course/overview/setup-passkey-modal/setup-passkey-modal.component.html(1 hunks)src/main/webapp/app/core/course/overview/setup-passkey-modal/setup-passkey-modal.component.spec.ts(1 hunks)src/main/webapp/app/core/course/overview/setup-passkey-modal/setup-passkey-modal.component.ts(1 hunks)src/main/webapp/app/core/home/home.component.spec.ts(1 hunks)src/main/webapp/app/core/home/home.component.ts(4 hunks)src/main/webapp/i18n/de/userSettings.json(2 hunks)src/main/webapp/i18n/en/userSettings.json(2 hunks)
✅ Files skipped from review due to trivial changes (4)
- src/main/webapp/app/core/auth/account.service.ts
- src/main/webapp/app/core/admin/system-notification-management/system-notification-detail.component.spec.ts
- src/main/webapp/app/app.component.ts
- src/main/webapp/app/core/course/overview/setup-passkey-modal/setup-passkey-modal.component.html
🚧 Files skipped from review as they are similar to previous changes (6)
- src/main/webapp/app/app.component.spec.ts
- src/main/webapp/i18n/de/userSettings.json
- gradle/jacoco.gradle
- src/main/webapp/app/core/course/overview/setup-passkey-modal/setup-passkey-modal.component.spec.ts
- src/main/webapp/app/core/course/overview/setup-passkey-modal/setup-passkey-modal.component.ts
- src/main/webapp/i18n/en/userSettings.json
🧰 Additional context used
📓 Path-based instructions (1)
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalC...
src/main/webapp/app/core/home/home.component.tssrc/main/webapp/app/core/home/home.component.spec.ts
🧬 Code Graph Analysis (2)
src/main/webapp/app/core/home/home.component.ts (1)
src/main/webapp/app/core/course/overview/setup-passkey-modal/setup-passkey-modal.component.ts (1)
EARLIEST_SETUP_PASSKEY_REMINDER_DATE_LOCAL_STORAGE_KEY(9-9)
src/main/webapp/app/core/home/home.component.spec.ts (4)
src/test/javascript/spec/helpers/mocks/mock-router.ts (1)
MockRouter(5-30)src/test/javascript/spec/helpers/mocks/service/mock-account.service.ts (1)
MockAccountService(7-45)src/test/javascript/spec/helpers/mocks/service/mock-ngb-modal.service.ts (1)
MockNgbModalService(1-5)src/main/webapp/app/core/course/overview/setup-passkey-modal/setup-passkey-modal.component.ts (1)
EARLIEST_SETUP_PASSKEY_REMINDER_DATE_LOCAL_STORAGE_KEY(9-9)
🪛 GitHub Check: client-tests-selected
src/main/webapp/app/core/home/home.component.spec.ts
[failure] 99-99:
No overload matches this call.
🪛 GitHub Check: client-tests
src/main/webapp/app/core/home/home.component.spec.ts
[failure] 99-99:
No overload matches this call.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: server-tests
- GitHub Check: server-style
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: Analyse
🔇 Additional comments (8)
src/main/webapp/app/core/home/home.component.ts (4)
23-24: Appropriate imports added.The necessary imports have been added to support the new passkey setup modal functionality.
50-50: NgbModal service properly injected.The modalService is correctly injected using the recommended Angular inject function.
87-111: Well-structured implementation of passkey modal display logic.The method contains clear and comprehensive checks before displaying the modal:
- Feature flag check
- User preference check via localStorage
- User registration status check
The implementation aligns with the PR objective to encourage passkey adoption.
233-234: Appropriate placement of passkey modal trigger.Calling
openSetupPasskeyModal()in thehandleLoginSuccess()method ensures the modal appears right after login, which is the optimal time to prompt users about setting up passkeys.src/main/webapp/app/core/home/home.component.spec.ts (4)
1-75: Well-structured test setup with comprehensive mock providers.The test setup includes all necessary mocks and dependencies for thorough testing of the component, including AccountService, RouterLink, NgbModal, and other services.
Good practice to clear localStorage before each test to ensure test isolation.
77-95: Basic component functionality tests are appropriate.Tests verify:
- Component creation
- Initialization with profile info
- Prefilled username handling
- Form validation logic
These cover the fundamental component behavior effectively.
97-141: Login process tests are thorough and well-structured.Tests cover:
- Successful login flow
- Failed login handling
- Loading state management during login process
The async/await pattern is correctly used to handle asynchronous operations.
🧰 Tools
🪛 GitHub Check: client-tests-selected
[failure] 99-99:
No overload matches this call.🪛 GitHub Check: client-tests
[failure] 99-99:
No overload matches this call.
143-202: Comprehensive test coverage for passkey modal functionality.Tests verify all decision paths for the modal display logic:
- Feature flag check
- User registration status check
- Reminder date validation in localStorage
- Both past and future reminder dates
The tests align perfectly with the implementation logic in the component.
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
src/main/webapp/app/core/course/overview/setup-passkey-modal/setup-passkey-modal.component.html
Outdated
Show resolved
Hide resolved
src/main/webapp/app/core/user/settings/passkey-settings/passkey-settings.component.html
Show resolved
Hide resolved
| "title": "Bleib sicher mit Passkeys", | ||
| "description": { | ||
| "howItWorks": "Dein Passkey wird auf deinem Gerät gespeichert und ist durch biometrische Daten oder eine PIN gesichert, was das Einloggen schnell und sicher macht – ohne dass du dir ein Passwort merken musst.", | ||
| "recommendation": "Wir empfehlen, jetzt einen Passkey einzurichten, um die Sicherheit deines Kontos zu erhöhen und eine nahtlose und passwortfreie Anmeldung zu genießen." |
There was a problem hiding this comment.
"genießen" might sound a bit too enthusiastic, but I'm fine with it 😁
There was a problem hiding this comment.
I am open to suggestions 😂
There was a problem hiding this comment.
"erleben" or "profitieren" maybe?
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||||||||||||||
@ahmetsenturk what other way would you propose? I am not quite sure which change you are expecting. The 30 days button is only there because of the rare edge case that a user, for whichever reason, does not want to setup passkey and logs out and in to Artemis frequently (which also wont be the case for most users, who most likely activated the "remember me" functionality). |
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||||||||
florian-glombik
moved this from Ready For Review
to Work In Progress
in Artemis Development
Checklist
General
Server
Client
Motivation and Context
We recently introduced passkey support in Artemis in #10656.
We now want to advertise passkeys as the preferred authentication mechanism. Therefore users shall be informed via a modal if they have not yet registered a passkey.
It was an active decision to notify users each time until they have registered at least one passkey.
Description
UserDTO.javato contain whether the user has registered a passkey or notwebauthnpackage topasskeyPasskeyCredentialRepository#existsByUserIdto easily infer if a user has a passkey or notSteps for Testing
These are many steps, but it should only take around 5 minutes as it is mostly logging in and out of Artemis.
Set Up LaterSetup PasskeyRemind me in 30 days(You can clear/adjust
earliestSetupPasskeyReminderDatefrom your local storage if you want to check that after 30 days the modal will be displayed again)Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Performance Review
Code Review
Manual Tests
Test Coverage
Screenshots
Summary by CodeRabbit
New Features
Bug Fixes
Style
Documentation
Tests
Chores