Artemis
Artemis copied to clipboard
`General`: Personal Access Tokens (PAT)
Checklist
General
- [x] I tested all changes and their related features with all corresponding user types on a test server. (KIT Test System)
- [x] Language: I followed the guidelines for inclusive, diversity-sensitive, and appreciative language.
- [x] I chose a title conforming to the naming conventions for pull requests.
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
- Activate "pat" profile. You can use prebuild docker images from https://github.com/kit-sdq/Artemis4Docker/pkgs/container/artemis/27516500?tag=pr-5216
- Log in to Artemis
- Navigate to Settings ⇾ Personal Access Tokens
- Press Generate
- 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
I'll add it to the current milestone :) Let's see whether we manage to merge this in this or the next milestones :)
Additionally, there is a related client test failure due to the additional feature toggle in AdminFeatureToggleComponentTest › onInit test if features mapped successfully
.
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.
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());
}
}
}
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?
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 :)
@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
⚠️ 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
@LDAP if the GitHub UI does not update, I would simply suggest to open a new PR and close this one :)
@dfuchss The GitHub issue should be fixed now. A workaround was to temporarily change the base branch.
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 :)
I'll push the PR through the pipeline real quick in #5345. If it succeeds we can merge this one
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.
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 :) )
I've converted the lifetime to seconds (in the config). The other config (e.g., some lines above for athene uses seconds as well)
On hold for now.
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.