General: Disable edits of users if saml2 is active#12542
General: Disable edits of users if saml2 is active#12542
General: Disable edits of users if saml2 is active#12542Conversation
github-actions
Bot
added
tests
server
There was a problem hiding this comment.
Pull request overview
Disables editing of user name/email in the account settings when the saml2 profile is active, while still allowing users to change their language preference.
Changes:
- Server: Short-circuit
/api/core/accountupdates under SAML2 to only persistlangKey. - Client: Detect active SAML2 profile to disable name/email inputs and only send
langKeyupdates. - Tests/i18n: Add integration/unit tests and user-facing info message translations (EN/DE).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/de/tum/cit/aet/artemis/core/web/AccountResource.java | Enforces SAML2 behavior on the server by limiting updates to langKey. |
| src/main/java/de/tum/cit/aet/artemis/core/service/ProfileService.java | Adds a helper to check whether the saml2 Spring profile is active. |
| src/main/webapp/app/core/account/settings/settings.component.ts | Detects SAML2 profile, disables name/email fields, and avoids updating them in the payload. |
| src/main/webapp/app/core/account/settings/settings.component.html | Shows an informational banner when SAML2 syncing is active. |
| src/main/webapp/app/core/account/settings/settings.component.spec.ts | Adds a unit test ensuring name/email aren’t updated when SAML2 is active. |
| src/main/webapp/i18n/en/settings.json | Adds SAML2 syncing info text (EN). |
| src/main/webapp/i18n/de/settings.json | Adds SAML2 syncing info text (DE). |
| src/test/java/de/tum/cit/aet/artemis/core/user/AccountResourceIntegrationTest.java | Adds integration tests verifying server-side behavior under SAML2. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@dfuchss Test coverage could not be fully measured because some tests failed. Please check the workflow logs for details. |
End-to-End Test Results
Test Strategy: Two-phase execution
❌ Failed Tests (Phase 2)
Flakiness Scores for Failed Tests
Overall: ❌ Phase 2 (remaining tests) failed 🔗 Workflow Run · 📊 Test Report Phase 1 · 📊 Test Report Phase 2 |
dfuchss
force-pushed
the
feature/disable-account-profile-change-for-saml2
branch
from
e0524d1 to
2d73e4a
Compare
dfuchss
force-pushed
the
feature/disable-account-profile-change-for-saml2
branch
from
2d73e4a to
b5c2c8f
Compare
|
@dfuchss Test coverage could not be fully measured because some tests failed. Please check the workflow logs for details. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@dfuchss Test coverage has been automatically updated in the PR description. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@dfuchss Test coverage has been automatically updated in the PR description. |
…java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@dfuchss Test coverage has been automatically updated in the PR description. |
github-project-automation
Bot
moved this from Work In Progress
to Ready For Review
in Artemis Development
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/webapp/app/core/account/settings/settings.component.ts`:
- Around line 138-143: The payload currently only blocks name/email updates for
SAML2, but the form disables those controls when !isInternal || isSaml2Active;
change the update guard to match that rule so non-internal users can't submit
tampered name/email. In settings.component.ts replace the condition around
setting userToUpdate.firstName/lastName/email (currently checking
this.isSaml2Active) with the inverse of the disable predicate (i.e. only allow
assignment when this.isInternal && !this.isSaml2Active) so the submitted payload
aligns with the disabled-control logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e5037d88-e563-410a-b695-1e9d094bfa83
📒 Files selected for processing (2)
src/main/webapp/app/core/account/settings/settings.component.spec.tssrc/main/webapp/app/core/account/settings/settings.component.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/webapp/app/core/account/settings/settings.component.spec.ts
|
@dfuchss Test coverage has been automatically updated in the PR description. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/webapp/app/core/account/settings/settings.component.html (1)
34-34: Remove redundant[disabled]bindings on reactive form controls.The component already calls
disable()/enable()on these controls viaupdateNameAndEmailControlState()(lines 111–117), which manages the disabled state through the reactive form API. The[disabled]bindings in the template are redundant—when aFormControlis disabled, Angular automatically renders thedisabledattribute in the DOM. Using both triggers an Angular dev-mode warning: "It looks like you're using the disabled attribute with a reactive form directive."Remove the
[disabled]bindings to rely solely on the FormControl state as the single source of truth:Changes (lines 34, 59, 90)
- formControlName="firstName" - [disabled]="!isInternalUser() || isSaml2Active" + formControlName="firstName"Also applies to: 59, 90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/app/core/account/settings/settings.component.html` at line 34, Remove the redundant [disabled] bindings from the template controls that are already managed by the reactive form API; rely on the FormControl disabled state set by updateNameAndEmailControlState() instead of using [disabled]="!isInternalUser() || isSaml2Active". Specifically, delete the [disabled] attributes referencing isInternalUser() and isSaml2Active on the form inputs (the ones controlled by updateNameAndEmailControlState()) so Angular's FormControl.disable()/enable() is the single source of truth and avoids the dev-mode warning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/webapp/app/core/account/settings/settings.component.html`:
- Line 34: Remove the redundant [disabled] bindings from the template controls
that are already managed by the reactive form API; rely on the FormControl
disabled state set by updateNameAndEmailControlState() instead of using
[disabled]="!isInternalUser() || isSaml2Active". Specifically, delete the
[disabled] attributes referencing isInternalUser() and isSaml2Active on the form
inputs (the ones controlled by updateNameAndEmailControlState()) so Angular's
FormControl.disable()/enable() is the single source of truth and avoids the
dev-mode warning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3492cc8e-23e5-4fe9-8fdc-6c29e1e1aadc
📒 Files selected for processing (2)
src/main/webapp/app/core/account/settings/settings.component.htmlsrc/main/webapp/app/core/account/settings/settings.component.ts
|
@dfuchss Test coverage has been automatically updated in the PR description. |
Claudia-Anthropica
left a comment
Claudia-Anthropica
left a comment
There was a problem hiding this comment.
@dfuchss Nice clean feature — the server/client split and tests all look solid. One issue though: the SAML2 guard sits after the registration-disabled check, which means SAML2 users can't even change their language when registration is disabled. See inline comment.
b-fein
left a comment
b-fein
left a comment
There was a problem hiding this comment.
Tested on a Uni Passau test system with SAML2. Works as intended.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Claudia-Anthropica
left a comment
Claudia-Anthropica
left a comment
There was a problem hiding this comment.
@dfuchss All feedback addressed — SAML2 users are indeed internal so the ordering concern doesn't apply. Nice clean implementation, approving.
|
@dfuchss Test coverage has been automatically updated in the PR description. |
|
@dfuchss Test coverage has been automatically updated in the PR description. |
|
@dfuchss Test coverage has been automatically updated in the PR description. |
|
@dfuchss Test coverage has been automatically updated in the PR description. |
Summary
With SAML2 enabled, users shall not be able to change Name or Mail.
See #12528
Checklist
General
Server
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Motivation and Context
Resolve #12528 SAML2 users shall not be able to change their names or mail
Description
Disables (FE/BE) that users can change their Name or Mail if SAML2 is enabled.
Steps for Testing
Prerequisites:
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
Exam Mode Test
Performance Tests
Test Coverage
Client
Server
Last updated: 2026-05-07 20:30:19 UTC
Screenshots
Summary by CodeRabbit
New Features
Documentation
Tests