Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrated code lifecycle: Support multiple SSH keys per user #9478

Open
wants to merge 59 commits into
base: develop
Choose a base branch
from

Conversation

SimonEntholzer
Copy link
Contributor

@SimonEntholzer SimonEntholzer commented Oct 14, 2024

Checklist

General

Server

  • Important: I implemented the changes with a very good performance and prevented too many (unnecessary) and too complex database calls.
  • I strictly followed the principle of data economy for all database calls.
  • I strictly followed the server coding and design guidelines.
  • I added multiple integration tests (Spring) related to the features (with a high test coverage).
  • I added pre-authorization annotations according to the guidelines and checked the course groups for all new REST Calls (security).
  • I documented the Java code using JavaDoc style.

Client

  • Important: I implemented the changes with a very good performance, prevented too many (unnecessary) REST calls and made sure the UI is responsive, even with large data (e.g. using paging).
  • I strictly followed the principle of data economy for all client-server REST calls.
  • I strictly followed the client coding and design guidelines.
  • Following the theming guidelines, I specified colors only in the theming variable files and checked that the changes look consistent in both the light and the dark theme.
  • I added multiple integration tests (Jest) related to the features (with a high test coverage), while following the test guidelines.
  • I added authorities to all new routes and checked the course groups for displaying navigation elements (links, buttons).
  • I documented the TypeScript code using JSDoc style.
  • I added multiple screenshots/screencasts of my UI changes.
  • I translated all newly inserted strings into English and German.

Changes affecting Programming Exercises

  • High priority: I tested all changes and their related features with all corresponding user types on a test server configured with the integrated lifecycle setup (LocalVC and LocalCI).

Motivation and Context

Adds the feature requested in #9166

Description

Support multiple SSH keys per user.

This PR:

  • adds a new table to support multiple SSH keys
  • extends the frontend to let users create multiple keys
  • enables git operations with the new keys

Steps for Testing

Add ssh keys to your account, and use them to clone different repositories in Artemis.

Testserver States

Note

These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.







Review Progress

Performance Review

  • I (as a reviewer) confirm that the client changes (in particular related to REST calls and UI responsiveness) are implemented with a very good performance even for very large courses with more than 2000 students.
  • I (as a reviewer) confirm that the server changes (in particular related to database calls) are implemented with a very good performance even for very large courses with more than 2000 students.

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Test Coverage

Client

Class/File Line Coverage Confirmation (assert/expect)
account.service.ts 87.71% ✅ ❌
user-ssh-public-key.model.ts 100%
code-button.component.ts 93.37% ✅ ❌
ssh-user-settings-key-details.component.ts 98.61% ✅ ❌
ssh-user-settings.component.ts 94.73% ✅ ❌

Server

Class/File Line Coverage Confirmation (assert/expect)
UserRepository.java 89% ✅ ❌
AccountResource.java 93% ✅ ❌
UserSshPublicKey.java 93% ✅ ❌
UserSshPublicKeyDTO.java 100%
UserSshPublicKeyService.java 97% ✅ ❌
GitPublickeyAuthenticatorService.java 83% ✅ ❌

Screenshots

The initial view, when the user does not have any SSH keys:
image
The SSH key creation view:
image
The view after the key has been created:
image
The keys overview page when the user has multiple keys:
image
Th user can delete keys and view its details:
image
View details:
image
Delete the key:
image

Summary by CodeRabbit

Release Notes

  • New Features

    • Enhanced SSH key management with new endpoints for adding, retrieving, and deleting SSH keys.
    • Introduction of a new UserSshPublicKey model to encapsulate SSH key details.
    • New component for SSH key details management, supporting both creation and viewing modes.
    • Improved user feedback with detailed error messages for SSH key operations.
    • Added new methods to manage and validate SSH keys, including checks for existing keys and invalid formats.
    • New UserSshPublicKeyDTO for data transfer related to SSH keys.
    • Asynchronous loading and management of SSH keys in user settings components.
    • New routes for SSH user settings, facilitating navigation between adding and viewing SSH keys.
  • Bug Fixes

    • Enhanced error handling for invalid SSH key formats and duplication scenarios.
  • Documentation

    • Updated user settings translations to reflect new SSH key management features.
  • Style

    • Improved UI layout and responsiveness for SSH settings components.
    • Updated styling for dropdowns and buttons in SSH settings to enhance user interaction.
  • Tests

    • Expanded test coverage for SSH key management functionalities, including new scenarios for retrieval and validation.
    • Comprehensive unit tests for the new SSH key details component, ensuring robust functionality and error handling.

@github-actions github-actions bot added tests server Pull requests that update Java code. (Added Automatically!) client Pull requests that update TypeScript code. (Added Automatically!) database Pull requests that update the database. (Added Automatically!). Require a CRITICAL deployment. core Pull requests that affect the corresponding module programming Pull requests that affect the corresponding module labels Oct 14, 2024
@b-fein b-fein changed the title Integrated code Lifecycle: Support multiple SSH keys per useradded Integrated code Lifecycle: Support multiple SSH keys per user Oct 14, 2024
@github-actions github-actions bot added the buildagent Pull requests that affect the corresponding module label Oct 17, 2024
@github-actions github-actions bot removed the buildagent Pull requests that affect the corresponding module label Oct 17, 2024
Copy link
Member

@BBesrour BBesrour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on TS3, from the user-view, works as expected
But the build fails
image

Feras797
Feras797 previously approved these changes Oct 29, 2024
Copy link

@Feras797 Feras797 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on TS3, everything works as expected.
Only one thing I noticed is that it is possible to assign the same label to multiple keys. Not sure if this is supposed to be possible.
image

SindiBuklaji
SindiBuklaji previously approved these changes Oct 29, 2024
Copy link

@SindiBuklaji SindiBuklaji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on TS5. Works as expected 👍

coderabbitai[bot]
coderabbitai bot previously approved these changes Oct 30, 2024
Copy link
Member

@BBesrour BBesrour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-approve after merge conflicts

Copy link
Contributor

@JohannesStoehr JohannesStoehr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reapprove after merge

Copy link
Contributor

@az108 az108 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reapprove after merge

Copy link

@HawKhiem HawKhiem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reapprove

Copy link
Contributor

@pzdr7 pzdr7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client Pull requests that update TypeScript code. (Added Automatically!) core Pull requests that affect the corresponding module database Pull requests that update the database. (Added Automatically!). Require a CRITICAL deployment. programming Pull requests that affect the corresponding module ready for review server Pull requests that update Java code. (Added Automatically!) tests
Projects
Status: Ready For Review
Development

Successfully merging this pull request may close these issues.