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

Development: Add tool token support #9408

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

Development: Add tool token support #9408

wants to merge 57 commits into from

Conversation

janthoXO
Copy link
Contributor

@janthoXO janthoXO commented Oct 1, 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.

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).
  • I tested all changes and their related features with all corresponding user types on a test server configured with Gitlab and Jenkins.

Motivation and Context

For the online IDE to guarantee a seamless login progress without prompting the user with a login dialog again, the JWT token has to be passed from the Artemis Angular Client to the Theia Online IDE in a redirect. Therefore the token can not be in an HTTP Cookie.
To work around this but still guarantee security, we generate a new token with a max lifespan of 1 day and add a particular Tool Claim to restrict its access to certain endpoints.

Description

This PR adds functionality for tool specific tokens. These tokens are restricted to accessing only certain endpoints to guarantee the principle of least privilege.
The PR show cases this functionality for the Scorpio VSCode extension and annotates the needed endpoints with the @AllowedTools(ToolTokenType.SCORPIO)
In addition this claim can also be used when authenticating the user with username and password to restrict the token to these endpoints there as well.

Affected Endpoints which should be allowed for the Scorpio Extension:

  • GET participations/{participationId}/results/{resultId}/details fetch Result Feedback to display TestCase Feedback, and which tests failed for the submission
  • GET account/participation-vcs-access-token to get the user VCS token to then generate a Clone URL with VCS Token, so that the user does not need to have SSH configured nor needs to input his/her credentials
  • POST account/participation-vcs-access-token to generate the VCS Token if this user does not have one already
  • GET courses/for-dashboard to display a course selection view in the Extension similar to the angular client
  • GET courses/{courseId}/for-dashboard to display a exercise selection view in the Extension similar to the angular client
  • POST exercises/{exerciseId}/participations to start a programming exercise from within the extension
  • GET exercises/{exerciseId}/participation to get the current participation for the displayed exercise
  • GET /plantuml/svg to generate the plantuml which should be displayed in the problem statement

Steps for Testing

Postman testing or code review:
Postman Testing:
ArtemisTokens.postman_collection.json

  1. Send a request to POST {{base_url}}/api/public/authenticate?tool=SCORPIO

  2. Send a request to GET {{base_url}}/api/courses/for-dashboard

  3. Verify that you get a response 200

  4. Send a request to an GET {{base_url}}/api/ide-settings

  5. verify that you get an 403 Forbidden

2nd Part:
6. Send a request to POST {{base_url}}/api/public/authenticate !!! without the tool Parameter
7. Send a request to GET {{base_url}}/api/courses/for-dashboard
8. Verify a 200 response
9. Send a request to an GET {{base_url}}/api/ide-settings
10. Verify a 200 response

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

Class/File Line Coverage Confirmation (assert/expect)
ToolsInterceptor 86%
JWTCookieService 98%
TokenProvider 99%
TokenResource 82%

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Introduced AllowedTools annotation for enhanced access control.
    • Added ToolTokenType enumeration for tool-specific token management.
    • New TokenResource for managing tool tokens via REST API.
    • Enhanced JWT cookie creation with tool-specific claims.
    • Added ToolsInterceptor for access control based on JWT claims.
  • Improvements

    • Updated multiple resource methods to enforce tool-based access restrictions.
    • Enhanced logging for performance tracking in course-related endpoints.
  • Bug Fixes

    • Adjusted error handling for unauthorized access in token management.
  • Chores

    • Refactored existing methods to integrate new security features without altering core functionalities.

@janthoXO janthoXO self-assigned this Oct 1, 2024
@janthoXO janthoXO requested a review from iyannsch October 1, 2024 20:22
@github-actions github-actions bot added server Pull requests that update Java code. (Added Automatically!) core Pull requests that affect the corresponding module labels Oct 1, 2024
@janthoXO janthoXO removed the request for review from iyannsch October 1, 2024 20:22
@krusche
Copy link
Member

krusche commented Oct 2, 2024

As discussed before, I do not think that it is a good idea to offer a re-key functionality from a security point of view. Therefore, I would like to decline this PR and close it.

@janthoXO
Copy link
Contributor Author

janthoXO commented Oct 2, 2024

As discussed before, I do not think that it is a good idea to offer a re-key functionality from a security point of view. Therefore, I would like to decline this PR and close it.

its still a draft, so I would like to first have a discussion about it, before declining this PR.

@krusche
Copy link
Member

krusche commented Oct 2, 2024

As discussed before, I do not think that it is a good idea to offer a re-key functionality from a security point of view. Therefore, I would like to decline this PR and close it.

its still a draft, so I would like to first have a discussion about it, before declining this PR.

We discussed this already several times. I'm not sure if additional discussions help, when my arguments against it are ignored :-(

@janthoXO
Copy link
Contributor Author

janthoXO commented Oct 2, 2024

We discussed this already several times. I'm not sure if additional discussions help, when my arguments against it are ignored :-(

we agreed on a meeting in the afternoon, so please let us have this meeting first

@janthoXO
Copy link
Contributor Author

janthoXO commented Oct 2, 2024

After the meeting the proposed solution looks as follows:

  • re-key is a suboptimal name since it is only used for theia. therefore the endpoint should be renamed
  • the token lifetime should not be extended - best case the token should be valid at max for 1 day
  • the token should contain a theia specific flag
  • the whole functionality depends on the theia profile being active
  • the token is a standard artemis token inheriting all of the original tokens capabilities

@krusche will provide further feedback on that

@janthoXO janthoXO changed the title Add re-key endpoint Programming Exercise Add re-key endpoint Oct 2, 2024
@janthoXO janthoXO changed the title Programming Exercise Add re-key endpoint Programming Exercise: Add re-key endpoint Oct 2, 2024
@janthoXO janthoXO changed the title Programming Exercise: Add re-key endpoint Programming exercises: Add re-key endpoint Oct 2, 2024
@janthoXO janthoXO changed the title Programming exercises: Add re-key endpoint Programming exercises: Add theia token for redirect Oct 2, 2024
@janthoXO janthoXO added the stacked-pr PR that depends on another PR label Oct 4, 2024
@janthoXO janthoXO changed the base branch from develop to feature/bearer-support October 4, 2024 09:48
@janthoXO
Copy link
Contributor Author

janthoXO commented Oct 4, 2024

TODO, make endpoint only available if theia profile is active

coderabbitai[bot]
coderabbitai bot previously approved these changes Oct 8, 2024
coderabbitai[bot]
coderabbitai bot previously approved these changes Jan 16, 2025
@janthoXO janthoXO removed ready to merge maintainer-approved The feature maintainer has approved the PR stacked-pr PR that depends on another PR labels Jan 16, 2025
Copy link
Contributor

@muradium muradium 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

@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.

Code reapprove 👍

Copy link

@SimonKaran13 SimonKaran13 left a comment

Choose a reason for hiding this comment

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

Reapprove

@janthoXO janthoXO added ready to merge maintainer-approved The feature maintainer has approved the PR labels Jan 17, 2025
Copy link
Member

@Mtze Mtze left a comment

Choose a reason for hiding this comment

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

Reapprove

@janthoXO janthoXO changed the title Programming exercises: Add tool token support Development: Add tool token support Jan 17, 2025
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assessment Pull requests that affect the corresponding module core Pull requests that affect the corresponding module exercise Pull requests that affect the corresponding module maintainer-approved The feature maintainer has approved the PR programming Pull requests that affect the corresponding module ready for review ready to merge 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.