Artemis icon indicating copy to clipboard operation
Artemis copied to clipboard

`General`: Personal Access Tokens (PAT)

Open LDAP opened this issue 2 years ago • 15 comments

Checklist

General

Server

  • [x] I followed the coding and design guidelines.
  • [x] I added multiple integration tests (Spring) related to the features (with a high test coverage).
  • [x] I added @PreAuthorize and checked the course groups for all new REST Calls (security).
  • [x] I implemented the changes with a good performance and prevented too many database calls.
  • [x] I documented the Java code using JavaDoc style.

Client

  • [x] I followed the coding and design guidelines and ensured that the layout is responsive.
  • [x] 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.
  • [x] I added authorities to all new routes and checked the course groups for displaying navigation elements (links, buttons).
  • [x] I documented the TypeScript code using JSDoc style.
  • [x] I added multiple screenshots/screencasts of my UI changes.
  • [x] I translated all newly inserted strings into English and German.

Motivation and Context

Since we are developing applications for Artemis like eclipse-artemis, we would like to have an authentication workflow that is independent from the used login mechanism. E.g., we at KIT are currently using the SAML2/Shibboleth workflow for registering users & the internal user management of Artemis.

Since our Eclipse Plugin directly talks to the REST API of Artemis, we currently rely on the ability to login via username and password to get an JWT Token. Nevertheless, we want to disable the ability to change the password of users in Artemis and want to enable https://github.com/ls1intum/Artemis/pull/4749 to clone repositories. This would make our Grading Plugin not usable, since it could not communicate with the API anymore.

@dfuchss

Implementation using JWT Tokens for #5075. Closes #5075

Description

This PR adds a new REST endpoint that allows a logged-in user to generate new JWT Tokens that can be used as personal access tokens. The feature can be disabled using feature toggles.

Steps for Testing

Prerequisites: None

  1. Activate "pat" profile. You can use prebuild docker images from https://github.com/kit-sdq/Artemis4Docker/pkgs/container/artemis/27516500?tag=pr-5216
  2. Log in to Artemis
  3. Navigate to Settings ⇾ Personal Access Tokens
  4. Press Generate
  5. Check that the JWT Token works for the API (e.g., using Postman)

Review Progress

Code Review

  • [x] Review 1
  • [x] Review 2

Manual Tests

  • [x] Test 1
  • [x] Test 2

Test Coverage

Class/File Branch Line

Screenshots

image image

LDAP avatar Jun 08 '22 12:06 LDAP

I'll add it to the current milestone :) Let's see whether we manage to merge this in this or the next milestones :)

dfuchss avatar Jun 14 '22 08:06 dfuchss

Additionally, there is a related client test failure due to the additional feature toggle in AdminFeatureToggleComponentTest › onInit test if features mapped successfully.

b-fein avatar Jun 14 '22 08:06 b-fein

Additionally, there is a related client test failure due to the additional feature toggle in AdminFeatureToggleComponentTest › onInit test if features mapped successfully.

I've fixed the test. I simply increased the expected amount of feature toggles.

dfuchss avatar Jun 14 '22 11:06 dfuchss

As a follow-up PR, we can think about changing the mechanisms of FeatureToggles in a way that you can specify whether Features are enabled or disabled by default. For now, I would expect that to be out of scope of this PR.

Nevertheless, I think the patch would be ..

diff --git a/src/main/java/de/tum/in/www1/artemis/service/feature/Feature.java b/src/main/java/de/tum/in/www1/artemis/service/feature/Feature.java
index e20625d2821fdd68d07175813444a2db3e554528..6ac5ae61815cf175f217fa40bda99ecdfdbda9a9 100644
--- a/src/main/java/de/tum/in/www1/artemis/service/feature/Feature.java
+++ b/src/main/java/de/tum/in/www1/artemis/service/feature/Feature.java
@@ -1,5 +1,15 @@
 package de.tum.in.www1.artemis.service.feature;
 
 public enum Feature {
-    ProgrammingExercises, PlagiarismChecks, PersonalAccessTokens
+    ProgrammingExercises(true), PlagiarismChecks(true), PersonalAccessTokens(false);
+
+    private final boolean enabledByDefault;
+
+    Feature(boolean enabledByDefault){
+        this.enabledByDefault = enabledByDefault;
+    }
+
+    public boolean isEnabledByDefault(){
+        return this.enabledByDefault;
+    }
 }
diff --git a/src/main/java/de/tum/in/www1/artemis/service/feature/FeatureToggleService.java b/src/main/java/de/tum/in/www1/artemis/service/feature/FeatureToggleService.java
index 544fe7bcac8eeadf2fed1ede9147ec5a4a949296..401a4737891f4e333622128ff13ab51d5c01180c 100644
--- a/src/main/java/de/tum/in/www1/artemis/service/feature/FeatureToggleService.java
+++ b/src/main/java/de/tum/in/www1/artemis/service/feature/FeatureToggleService.java
@@ -28,7 +28,7 @@ public class FeatureToggleService {
         // This ensures that all features are enabled once the system starts up
         for (Feature feature : Feature.values()) {
             if (!features.containsKey(feature)) {
-                features.put(feature, true);
+                features.put(feature, feature.isEnabledByDefault());
             }
         }
     }

dfuchss avatar Jun 14 '22 11:06 dfuchss

A feature toggle (runtime switch) does not make sense in this case in my opinion. They are active by default and decisions are reset after restarting the servers.

Can we instead activate / deactivate this feature using a profile (startup time switch)? Alternatively, an entry in the yml file could also be used.

In case the feature is not activated, the UI should not be shown.

And can we move the client code into a new lazy loaded module? Then, it will not affect instances that are not using the feature.

One additional question: is the max time of a token limited? Otherwise users could create a 50 years long token. And how do you handle the case if a user was deactivated or even deleted but the token is still valid?

krusche avatar Jun 27 '22 05:06 krusche

A feature toggle (runtime switch) does not make sense in this case in my opinion. They are active by default and decisions are reset after restarting the servers.

Can we instead activate / deactivate this feature using a profile (startup time switch)? Alternatively, an entry in the yml file could also be used.

In case the feature is not activated, the UI should not be shown.

And can we move the client code into a new lazy loaded module? Then, it will not affect instances that are not using the feature.

One additional question: is the max time of a token limited? Otherwise users could create a 50 years long token. And how do you handle the case if a user was deactivated or even deleted but the token is still valid?

Sure. We will take care about the suggestions.

Regarding "And can we move the client code into a new lazy loaded module? Then, it will not affect instances that are not using the feature.": For more difficult changes in the client we may need some help :)

dfuchss avatar Jun 27 '22 07:06 dfuchss

@krusche

  • I added a new profile pat which enables/disables the feature. The UI is now completely hidden when disabled.
  • Admins can set a maximum lifetime in the configuration files.
  • Disabled users cannot request new tokens, they can however still login with an existing token (this is the same behavior as the normal Artemis JWT token authentication).
  • The module should now be lazy loaded

LDAP avatar Jun 30 '22 09:06 LDAP

⚠️ The PR seems not to update to the current commits :( ⚠️ Please simply look at https://github.com/kit-sdq/Artemis/tree/feature/personal-access-token for now. We have to wait until github updates its UI

dfuchss avatar Jun 30 '22 09:06 dfuchss

@LDAP if the GitHub UI does not update, I would simply suggest to open a new PR and close this one :)

dfuchss avatar Jun 30 '22 10:06 dfuchss

@dfuchss The GitHub issue should be fixed now. A workaround was to temporarily change the base branch.

LDAP avatar Jun 30 '22 10:06 LDAP

Thanks for the detailled protocol and suggestions :)

  • Test that Artemis sets correct headers, so that a malicious application can not generate a token when the user is already signed-in in another tab or similar attack vectors

Agree, what do you think are these headers. From my point of view esp. CORS.

  • Maybe add a text notice that the token grants any application it is shared with full-access to your Artemis account

I would address this in a Follow-up PR

  • Use a sensible default for the lifetime input field (e.g., half of maximum?)

Heavily depends on use cases. I would not put too much effort here.

  • From an admin user experience, is there any benefit in setting the token length in milliseconds over minutes/days?

I don't think so :)

  • Document that we urgently need a way to invalidate tokens (#3139)

Where ? Internally only ?

  • Some client tests would be beneficial

Of course, but as I said we are more experts in backend development than in frontend dev :)

dfuchss avatar Jul 07 '22 12:07 dfuchss

I'll push the PR through the pipeline real quick in #5345. If it succeeds we can merge this one

bassner avatar Jul 07 '22 12:07 bassner

From an admin user experience, is there any benefit in setting the token length in milliseconds over minutes/days?

I don't think so :)

Then I'd be great if you could change the configuration setting to use “minutes” or “days”. 🙂 Makes more sense than milliseconds (why would you need a token < 1 second anyway?) in my opinion.

ge65cer avatar Jul 07 '22 13:07 ge65cer

From an admin user experience, is there any benefit in setting the token length in milliseconds over minutes/days?

I don't think so :)

Then I'd be great if you could change the configuration setting to use “minutes” or “days”. 🙂 Makes more sense than milliseconds (why would you need a token < 1 second anyway?) in my opinion.

I'll use seconds since other tokens using seconds as well (consistency :) )

dfuchss avatar Jul 08 '22 07:07 dfuchss

I've converted the lifetime to seconds (in the config). The other config (e.g., some lines above for athene uses seconds as well)

dfuchss avatar Jul 08 '22 07:07 dfuchss

On hold for now.

dfuchss avatar Sep 06 '22 17:09 dfuchss

Closing for now. Currently, we are working on a generic PAT-Workflow as service here. This service will be not directly integrated in Artemis but uses the password of the internal user management as "token" that can be easily generated.

dfuchss avatar Sep 12 '22 13:09 dfuchss