Conversation
olexii4
left a comment
There was a problem hiding this comment.
PR Review Summary
Verdict: 💬 Comment - No blocking issues. Design is sound, follows established project patterns consistently.
Criticality: 🟡 Important (DELETE endpoint on K8s Secrets)
Integration contracts verified against che-code extension source:
- ✅ Label
che.eclipse.org/device-authentication=truematches - ✅ Secret name pattern compatible
- ✅ No secret data exposed (only metadata returned)
Positive feedback:
- Clean layered architecture matching SSH keys / PATs / AI provider keys patterns
- 23 new frontend tests with good assertion rigor
- Minimal DTO - no secret data leakage
- Consistent error handling across all layers
5 inline comments posted below covering schema constraints, backend tests, reducer tests, label verification, and card distinguishability.
Review generated by ok-pr-review (standard + deep)
|
Hi! I'm che-ai-assistant — I help with your pull requests. Available commands:
|
|
Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1633 (linux/amd64, linux/arm64, linux/s390x) kubectl patch commandkubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/dashboard/deployment", "value": {containers: [{image: "quay.io/eclipse/che-dashboard:pr-1633", name: che-dashboard}]}}]" |
1 similar comment
|
Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1633 (linux/amd64, linux/arm64, linux/s390x) kubectl patch commandkubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/dashboard/deployment", "value": {containers: [{image: "quay.io/eclipse/che-dashboard:pr-1633", name: che-dashboard}]}}]" |
|
/che-ai-assistant ok-pr-review Task completed. |
tolusha
left a comment
There was a problem hiding this comment.
Well-structured PR with solid test coverage across all layers. The security model is sound with proper label validation on delete and correct use of user bearer tokens throughout. A few findings worth discussing before merging:
- Date type mismatch (I1): The
DeviceAuthTokenDTO declarescreationTimestamp?: Datebut Fastify serializes it to an ISO 8601 string. The frontend receives astringat runtime. See inline comment on the DTO. - Error conflation in deleteToken (D2): The catch block in
deleteTokenwraps the intentional label-mismatch throw alongside network errors, making all failure modes indistinguishable to the caller. - Bootstrap fetch for all users (W1/P1):
fetchDeviceAuthTokensadds a K8s API call to every dashboard load. Since the component already lazy-loads on tab visit, removing the bootstrap call would improve initial load time for users who never use this tab.
All other findings are minor (ordering, modal UX, extensibility). See inline comments for details.
|
Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1633 (linux/amd64, linux/arm64, linux/s390x) kubectl patch commandkubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/dashboard/deployment", "value": {containers: [{image: "quay.io/eclipse/che-dashboard:pr-1633", name: che-dashboard}]}}]" |
|
/che-ai-assistant ok-pr-review Task completed. |
|
/retest |
1 similar comment
|
/retest |
|
Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1633 (linux/amd64, linux/arm64, linux/s390x) kubectl patch commandkubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/dashboard/deployment", "value": {containers: [{image: "quay.io/eclipse/che-dashboard:pr-1633", name: che-dashboard}]}}]" |
olexii4
left a comment
There was a problem hiding this comment.
PR Review Summary
Verdict: 🔄 Request Changes — 2 critical issues, 4 suggestions
Criticality: 🟡 Important (K8s Secrets + GitHub OAuth tokens)
Integration contracts verified:
- ✅ Label
che.eclipse.org/device-authentication=truematches che-code - ✅ GitHub Device Flow API endpoints and grant type match docs
- ✅ GitHub revocation API matches docs
- ✅ No secret data exposed (only metadata returned)
Critical:
Math.random()for K8s secret names — collision risk in multi-user deployments (inline #1)response.oknot checked on GitHub API calls — silent failures on non-2xx (inline #2)
Warnings:
3. slow_down response not propagated — violates GitHub Device Flow spec (inline #3)
4. PF v5 CSS variable in PF 6 codebase (inline #4)
5. onSuccess callback drops token argument — stale UI on re-fetch failure (inline #5)
6. Missing maxLength on deviceCode schema field (inline #6)
Positive feedback:
- Clean layered architecture matching SSH keys / PATs patterns
- Comprehensive backend test suite (321 lines) with strong assertion rigor
resourceVersionprecondition on delete is a genuine hardening improvement- Delete modal includes helpful GitHub settings link
- Provider field + label is nice forward-looking extensibility
All prior review feedback (tolusha + self-review: 12 comments) has been addressed.
Review generated by ok-pr-review (standard + deep)
|
Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1633 (linux/amd64, linux/arm64, linux/s390x) kubectl patch commandkubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/dashboard/deployment", "value": {containers: [{image: "quay.io/eclipse/che-dashboard:pr-1633", name: che-dashboard}]}}]" |
1 similar comment
|
Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1633 (linux/amd64, linux/arm64, linux/s390x) kubectl patch commandkubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/dashboard/deployment", "value": {containers: [{image: "quay.io/eclipse/che-dashboard:pr-1633", name: che-dashboard}]}}]" |
|
/retest |
|
/che-ai-assistant ok-pr-review |
|
/retest |
Assisted-by: Claude Sonnet 4.6 Signed-off-by: Oleksii Orel <oorel@redhat.com>
…ferences tab Assisted-by: Claude Sonnet 4.6 Signed-off-by: Oleksii Orel <oorel@redhat.com>
…error handling Assisted-by: Claude Sonnet 4.6 Signed-off-by: Oleksii Orel <oorel@redhat.com>
…rovements Assisted-by: Claude Sonnet 4.6 Signed-off-by: Oleksii Orel <oorel@redhat.com>
Assisted-by: Claude Sonnet 4.6 Signed-off-by: Oleksii Orel <oorel@redhat.com>
…ap fallback Assisted-by: Claude Sonnet 4.6 Signed-off-by: Oleksii Orel <oorel@redhat.com>
Assisted-by: Claude Sonnet 4.6 Signed-off-by: Oleksii Orel <oorel@redhat.com>
Assisted-by: Claude Sonnet 4.6 Signed-off-by: Oleksii Orel <oorel@redhat.com>
|
Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1633 (linux/amd64, linux/arm64, linux/s390x) kubectl patch commandkubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/dashboard/deployment", "value": {containers: [{image: "quay.io/eclipse/che-dashboard:pr-1633", name: che-dashboard}]}}]" |
|
/retest |
Documents the Device Auth Tokens tab in User Preferences introduced in eclipse-che/che-dashboard#1633. New article (connecting-to-github-using-device-authorization.adoc): - Connect to GitHub using the device authorization flow (RFC 8628) - Reconnect to replace a revoked or expired token in-place - Delete a single token or multiple tokens in bulk - Troubleshooting for expired one-time codes Updated admin procedure (proc_setting-up-the-github-oauth-app.adoc): - Optional steps to enable Device Flow on the GitHub OAuth App - Step to create the device-auth-config ConfigMap that enables the Connect to GitHub button in the Dashboard Updated assembly (configuring-oauth-2-for-github.adoc): - NOTE for admins who already configured the OAuth App and want to enable Device Auth Tokens without repeating the full setup Navigation: added the new article under Using credentials and configurations in workspaces > Mounting secrets. Assisted-by: Claude Sonnet 4.6 Signed-off-by: Oleksii Orel <oorel@redhat.com>
svor
left a comment
There was a problem hiding this comment.
@olexii4 Great job!
Works for me
I'd like to ask @RomanNikitenko and @vinokurig for the review as well
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: olexii4, svor The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Documents the Device Auth Tokens tab in User Preferences introduced in eclipse-che/che-dashboard#1633. New article (connecting-to-github-using-device-authorization.adoc): - Connect to GitHub using the device authorization flow (RFC 8628) - Reconnect to replace a revoked or expired token in-place - Delete a single token or multiple tokens in bulk - Troubleshooting for expired one-time codes Updated admin procedure (proc_setting-up-the-github-oauth-app.adoc): - Optional steps to enable Device Flow on the GitHub OAuth App - Step to create the device-auth-config ConfigMap that enables the Connect to GitHub button in the Dashboard Updated assembly (configuring-oauth-2-for-github.adoc): - NOTE for admins who already configured the OAuth App and want to enable Device Auth Tokens without repeating the full setup Navigation: added the new article under Using credentials and configurations in workspaces > Mounting secrets. Assisted-by: Claude Sonnet 4.6 Signed-off-by: Oleksii Orel <oorel@redhat.com>
What does this PR do?
Adds a Device Auth Tokens tab to the User Preferences page with two capabilities:
Connect to GitHub — A Connect to GitHub button opens a modal displaying a one-time code with a copy button and a "Copy & Continue to Browser" primary button. The backend polls GitHub for authorization (RFC 8628) and on success writes a
che.eclipse.org/device-authentication=trueKubernetes Secret to the user's namespace. The button appears automatically when thedevice-auth-configConfigMap is present in the Che namespace — fully independent of Git Services OAuth configuration.View & delete (single and bulk) — A compact card list (matching SSH Keys UI) shows stored tokens. A per-row Actions ⋮ menu provides Delete and Reconnect actions. Deleting removes the K8s Secret and revokes the GitHub token via
POST /credentials/revoke(no app credentials required). Reconnecting replaces the existing token (single-active-token model matching che-code). Replacing an existing secret preserves itsresourceVersionand any extra labels applied by che-code or a mutating webhook.Device Authentication tokens are GitHub OAuth tokens generated by the device authorization flow (RFC 8628). They are stored as Kubernetes Secrets labeled
che.eclipse.org/device-authentication=true. Previously the only way to generate or remove a token was through the VS Code command palette inside a running workspace.Screenshot/screencast of this PR
What issues does this PR fix or reference?
fixes https://redhat.atlassian.net/browse/CRW-11582
Is it tested? How?
Unit / integration tests
getDeviceAuthClientId.spec.ts— 6 tests: ConfigMap read, TTL cache hit, env-var override, missing namespace, K8s 404deviceAuthToken.spec.ts— 4 route tests: 503 when ConfigMap absent, correct delegation when presentdeviceAuthTokenApi.spec.ts— 27 tests updated: clientId passed as parameter, revocation success path, createNamespacedSecret argument shapeDeploy and verify
device-auth-configConfigMap. Use VS Code's public OAuth App (01ab8ac9400c4e429b23) — it has Device Flow enabled by default and supports GitHub Copilot:github.com/login/device→ authorize.POST .../device-auth-token/initiatedirectly — verify HTTP 503 is returned.Release Notes
Added a Device Auth Tokens tab to User Preferences. Users can connect their GitHub account using device authorization directly from the Dashboard, view tokens with on-demand validity checks, and delete/revoke tokens without requiring app credentials. The feature is enabled by creating a
device-auth-configConfigMap in the Che namespace — fully independent of Git Services OAuth.Docs PR
eclipse-che/che-docs#3168