feat: add Slack integration features and settings #580
Conversation
- Introduced Slack OAuth callback handling in the SDK. - Added new API types for Slack connection management including disconnect, sync, and installation. - Implemented AdminSlackSettings component to manage Slack connection status. - Enhanced AdminSettingsPage to include Slack settings and notifications based on Slack connection status. - Created SlackConnectionSection for user settings, allowing users to link and unlink their Slack accounts. - Updated settings page to handle Slack connection state and provide feedback on connection status. - Added storybook stories for Slack connection components to demonstrate various states (connected, disconnected, loading, disabled). - Updated routing to handle Slack connection status in admin settings.
WalkthroughAdds comprehensive Slack integration spanning OAuth identity linking via Keycloak, workspace bot installation, user account management endpoints, React UI components, API contract updates, configuration across Docker and Spring Boot, database schema extension, and documentation guides with full test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/<br/>Browser
participant FE as Frontend
participant WC as WorkspaceSlack<br/>Controller
participant WIS as Workspace<br/>SlackIntegration<br/>Service
participant SlackAPI as Slack<br/>OAuth API
participant SC as SlackCallback<br/>Controller
participant DB as Database
User->>FE: Click "Connect Workspace"
FE->>WC: GET /slack/install
WC->>WIS: generateInstallUrl(workspace)
WIS->>WIS: generateSignedState(slug)
Note over WIS: HMAC-SHA256(slug+ts)
WIS-->>WC: install_url + signed_state
WC-->>FE: 302 Redirect
FE->>SlackAPI: Navigate to Slack OAuth
User->>SlackAPI: Authorize app
SlackAPI->>SC: Callback + code + state
SC->>WIS: completeInstallation(code, state)
WIS->>WIS: validateAndParseState()
Note over WIS: Verify signature & expiry
WIS->>SlackAPI: POST oauth.v2.access
SlackAPI-->>WIS: access_token
WIS->>DB: Update workspace.slack_token
DB-->>WIS: ✓ Persisted
WIS-->>SC: token_saved
SC-->>User: 302 → /admin/settings?slack=success
User->>FE: Settings page loads
FE-->>User: Show "Connected" state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
documentation
There was a problem hiding this comment.
Pull request overview
This PR introduces comprehensive Slack integration features including OAuth-based workspace bot installation, user account linking via Keycloak, and enhanced leaderboard notifications with @-mentions. The implementation follows security best practices with HMAC-signed state tokens, CSRF protection, and proper OAuth flows.
Key Changes:
- Added dual OAuth flows: workspace bot installation and user account linking
- Implemented HMAC-SHA256 signed state tokens with 10-minute expiry for CSRF protection
- Replaced fuzzy name matching with deterministic Slack user matching (linked ID → email)
- Added comprehensive test coverage for security-critical components
Reviewed changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
webapp/src/routes/_authenticated/settings.tsx |
Integrated Slack connection management into user settings with sync/disconnect mutations |
webapp/src/components/settings/SlackConnectionSection.tsx |
New component for user-level Slack account linking UI |
webapp/src/components/admin/AdminSlackSettings.tsx |
Admin component for workspace-level Slack bot installation |
server/.../WorkspaceSlackIntegrationService.java |
Core OAuth flow with HMAC-signed state tokens and constant-time signature validation |
server/.../SlackCallbackController.java |
Handles Slack OAuth callback with security validation |
server/.../SlackConnectionService.java |
Manages user account linking via Keycloak federated identities |
server/.../SlackWeeklyLeaderboardTask.java |
Replaced fuzzy name matching with deterministic ID/email matching |
server/.../WorkspaceSlackIntegrationServiceTest.java |
Comprehensive tests for OAuth flow, state validation, and security |
docs/admin/slack-integration.mdx |
Complete setup guide for production Slack integration |
pom.xml |
Updated Maven plugin wait time and JMX configuration (unrelated to Slack) |
| function RouteComponent() { | ||
| const navigate = useNavigate(); | ||
| const queryClient = useQueryClient(); | ||
| const client = useQueryClient(); |
There was a problem hiding this comment.
Variable renamed from queryClient to client. This inconsistency with common naming conventions makes the code less searchable. Consider keeping queryClient as it's the standard name used throughout the codebase and in TanStack Query documentation.
| const settingsKey = getUserSettingsQueryKey(); | ||
|
|
||
| // Query for user settings | ||
| const { data: settings, isLoading } = useQuery({ | ||
| ...getUserSettingsOptions({}), | ||
| retry: 1, | ||
| }); | ||
|
|
||
| // Mutation for updating user settings | ||
| const updateSettingsMutation = useMutation< | ||
| const updateMutation = useMutation< | ||
| UpdateUserSettingsResponse, | ||
| DefaultError, | ||
| Options<UpdateUserSettingsData>, | ||
| { previousSettings?: UserSettings } | ||
| { previous?: UserSettings } | ||
| >({ | ||
| ...updateUserSettingsMutation(), | ||
| onMutate: async (variables) => { | ||
| await queryClient.cancelQueries({ | ||
| queryKey: userSettingsQueryKey, | ||
| }); | ||
| const previousSettings = | ||
| queryClient.getQueryData<UserSettings>(userSettingsQueryKey); | ||
| await client.cancelQueries({ queryKey: settingsKey }); | ||
| const previous = client.getQueryData<UserSettings>(settingsKey); | ||
| if (variables.body) { | ||
| queryClient.setQueryData(userSettingsQueryKey, variables.body); | ||
| client.setQueryData(settingsKey, variables.body); | ||
| } | ||
| return { previousSettings }; | ||
| return { previous }; | ||
| }, | ||
| onError: (error, _variables, context) => { | ||
| if (context?.previousSettings) { | ||
| queryClient.setQueryData( | ||
| userSettingsQueryKey, | ||
| context.previousSettings, | ||
| ); | ||
| onError: (_error, _variables, context) => { | ||
| if (context?.previous) { | ||
| client.setQueryData(settingsKey, context.previous); | ||
| } | ||
| console.error("Failed to update user settings:", error); | ||
| toast.error("Failed to update settings. Please try again later."); | ||
| }, | ||
| onSuccess: (data) => { | ||
| queryClient.setQueryData(userSettingsQueryKey, data); | ||
| }, | ||
| onSettled: () => { | ||
| queryClient.invalidateQueries({ | ||
| queryKey: userSettingsQueryKey, | ||
| }); | ||
| toast.error("Failed to update settings."); | ||
| }, | ||
| onSuccess: (data) => client.setQueryData(settingsKey, data), | ||
| onSettled: () => client.invalidateQueries({ queryKey: settingsKey }), | ||
| }); | ||
|
|
||
| // Mutation for deleting account | ||
| const deleteAccountMutation = useMutation({ | ||
| const deleteMutation = useMutation({ |
There was a problem hiding this comment.
Variable naming is overly abbreviated. settingsKey, updateMutation, and deleteMutation should maintain their full descriptive names like userSettingsQueryKey, updateSettingsMutation, and deleteAccountMutation for better clarity and consistency with TanStack Query conventions.
| const handleConnect = () => { | ||
| if (!linkUrl) return; | ||
| window.open(linkUrl, "_blank", "noopener,noreferrer"); | ||
| }; |
There was a problem hiding this comment.
Missing null check for linkUrl. If linkUrl is null or undefined, the button will be disabled but window.open could still be called if this logic changes. Add an explicit null check before calling window.open.
| }: SlackConnectionSectionProps) { | ||
| if (!slackEnabled && !isLoading) return null; | ||
|
|
||
| const pending = Boolean(isLoading); |
There was a problem hiding this comment.
The pending variable is redundant. It's directly derived from isLoading with a Boolean cast that's unnecessary since isLoading is already a boolean. Remove this variable and use isLoading directly.
| export const Disconnected: Story = { | ||
| args: { | ||
| isConnected: false, | ||
| slackEnabled: true, | ||
| linkUrl: "https://keycloak.example.com/realms/hephaestus/account", | ||
| }, | ||
| }; | ||
|
|
||
| export const Connected: Story = { | ||
| args: { | ||
| isConnected: true, | ||
| slackUserId: "U12345ABCDE", | ||
| slackEnabled: true, | ||
| }, | ||
| }; | ||
|
|
||
| export const Loading: Story = { | ||
| args: { | ||
| isConnected: false, | ||
| slackEnabled: true, | ||
| isLoading: true, | ||
| }, | ||
| }; | ||
|
|
||
| export const Disabled: Story = { | ||
| args: { | ||
| isConnected: false, | ||
| slackEnabled: false, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Missing JSDoc comments for each story. According to the Storybook guidelines, add brief JSDoc comments explaining each story's specific use case with action-oriented descriptions.
| </exclude> | ||
| </excludes> | ||
| <wait>1000</wait> | ||
| <wait>30000</wait> |
There was a problem hiding this comment.
The wait time was increased from 1000ms to 30000ms (30 seconds). This is an extremely long wait time for the Spring Boot Maven plugin. This change appears unrelated to Slack integration and should be reverted or explained in a separate commit.
| <wait>30000</wait> | |
| <wait>1000</wait> |
| <!-- Use 'specs' profile for OpenAPI generation - disables WorkspaceStartupListener --> | ||
| <profiles>specs</profiles> | ||
| <jvmArguments>-Dspring.application.admin.enabled=true -XX:+EnableDynamicAgentLoading --enable-native-access=ALL-UNNAMED -Dspring.devtools.restart.enabled=false</jvmArguments> | ||
| <jvmArguments>-Dspring.application.admin.enabled=true -XX:+EnableDynamicAgentLoading --enable-native-access=ALL-UNNAMED -Dspring.devtools.restart.enabled=false -Djava.rmi.server.hostname=127.0.0.1 -Dspring.jmx.enabled=true -Dspring.application.admin.jmx-name=org.springframework.boot:type=Admin,name=SpringApplication</jvmArguments> |
There was a problem hiding this comment.
Additional JMX configuration appears unrelated to Slack integration. These changes should be in a separate commit or explained in the PR description.
| <jvmArguments>-Dspring.application.admin.enabled=true -XX:+EnableDynamicAgentLoading --enable-native-access=ALL-UNNAMED -Dspring.devtools.restart.enabled=false -Djava.rmi.server.hostname=127.0.0.1 -Dspring.jmx.enabled=true -Dspring.application.admin.jmx-name=org.springframework.boot:type=Admin,name=SpringApplication</jvmArguments> | |
| <jvmArguments>-Dspring.application.admin.enabled=true -XX:+EnableDynamicAgentLoading --enable-native-access=ALL-UNNAMED -Dspring.devtools.restart.enabled=false</jvmArguments> |
| isLoading?: boolean; | ||
| } | ||
|
|
||
| export function SlackConnectionSection({ |
There was a problem hiding this comment.
Missing JSDoc comment for the component. According to the Storybook guidelines (CodingGuidelineID: 1000004), add a concise JSDoc comment above the component explaining its purpose and when to use it.
| import { fn } from "@storybook/test"; | ||
| import { SlackConnectionSection } from "./SlackConnectionSection"; | ||
|
|
||
| const meta = { |
There was a problem hiding this comment.
Missing JSDoc comment for the Storybook meta object. According to the Storybook guidelines, add a brief comment explaining the component's purpose and key capabilities.
| /** | ||
| * Constant-time string comparison to prevent timing attacks. | ||
| * <p> | ||
| * Uses MessageDigest.isEqual which is specifically designed for cryptographic | ||
| * comparisons. | ||
| */ | ||
| private boolean constantTimeEquals(String a, String b) { | ||
| byte[] aBytes = a.getBytes(StandardCharsets.UTF_8); | ||
| byte[] bBytes = b.getBytes(StandardCharsets.UTF_8); | ||
| return MessageDigest.isEqual(aBytes, bBytes); | ||
| } |
There was a problem hiding this comment.
The constant-time comparison using MessageDigest.isEqual is good for security. However, consider documenting why this specific method is used instead of a simple string equality check to educate future maintainers about timing attack prevention.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| HttpServletResponse response) throws IOException { | ||
| // Handle user cancellation or Slack-side errors | ||
| if (error != null) { | ||
| logger.info("Slack OAuth was cancelled or errored: {}", error); |
Check failure
Code scanning / CodeQL
Log Injection High
| logger.info( | ||
| "Successfully connected Workspace '{}' to Slack Team '{}' ({})", | ||
| workspaceSlug, | ||
| response.getTeam().getName(), | ||
| response.getTeam().getId()); |
Check failure
Code scanning / CodeQL
Log Injection High
| String payload = encodedSlug + "|" + timestamp; | ||
| String expectedSignature = sign(payload); | ||
| if (!constantTimeEquals(expectedSignature, providedSignature)) { | ||
| logger.warn("State signature mismatch for workspace '{}' - possible CSRF attempt", workspaceSlug); |
Check failure
Code scanning / CodeQL
Log Injection High
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (14)
docker/.env.example (1)
182-188: Optional: Consider addressing static analysis hints.The static analysis tool suggests:
- Alphabetically ordering the KEYCLOAK_SLACK_* keys
- Removing the extra blank line at 188
These are style nitpicks and not blocking issues, but addressing them would improve consistency.
webapp/src/components/settings/SlackConnectionSection.tsx (1)
27-27: Remove unused parameter or use it in the UI.The
slackUserIdparameter is destructured with an underscore prefix (indicating intentional non-use) but never referenced in the component. If this is intended for future use or debugging, consider either:
- Removing it from the props interface if not needed
- Displaying it in the connected state UI for user confirmation
- Adding a comment explaining why it's present but unused
If you intend to display it:
<h3 className="text-base font-medium"> {isConnected ? "Connected" : "Not connected"} </h3> <p className="text-sm text-muted-foreground leading-relaxed"> {isConnected - ? "Your Slack and GitHub accounts are linked. You will be @-mentioned in leaderboard notifications." + ? `Your Slack account (${_slackUserId || 'ID unknown'}) is linked. You will be @-mentioned in leaderboard notifications.` : "Connect your Slack account to be @-mentioned in notifications."} </p>server/application-server/src/test/java/de/tum/in/www1/hephaestus/workspace/WorkspaceSlackIntegrationServiceTest.java (2)
29-31: Prefer extendingBaseUnitTestto avoid repeating Mockito/JUnit plumbing.You already have
de.tum.in.www1.hephaestus.testconfig.BaseUnitTestproviding@ExtendWith(MockitoExtension.class)+@Tag("unit"); inheriting it reduces duplication and keeps conventions consistent. Based on learnings, ...
117-125: Duplicate helperextractAndDecodeStateFromUrl—keep one shared implementation.Both nested classes define the same helper; move it to the outer test class (or a small private static utility) to prevent drift.
Also applies to: 281-288
webapp/src/components/admin/AdminSlackSettings.tsx (1)
36-41: Preferclsx/tailwind-merge+ design tokens over manual string building + raw hex.This aligns with the project’s TSX guidelines (class composition helpers + tokenized colors). If the Slack purple is intentional, consider centralizing it (e.g., a CSS var / theme token) rather than inline hex.
Also applies to: 61-68
server/application-server/src/test/java/de/tum/in/www1/hephaestus/account/SlackConnectionServiceTest.java (1)
22-24: AvoidReflectionTestUtilsby injecting config (constructor/setter) for testability.Setting
realm/slackEnabledvia reflection makes tests more fragile; prefer providing these via constructor or explicit package-private setters (or Spring config in an integration test). As per coding guidelines, ...Also applies to: 46-51
webapp/src/components/admin/AdminSettingsPage.tsx (1)
1-6: Move Slack toast side-effects to the route/container (keep this component presentational).
AdminSettingsPagenow runsuseEffect+toast(...), which conflicts with its “presentational component” role and the project’s container/presentation split. Prefer handlingslackStatusinwebapp/src/routes/_authenticated/w/$workspaceSlug/admin/_admin/settings.tsx(then pass only render-state props down). As per coding guidelines, ...Also applies to: 12-29, 50-81
server/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceSlackController.java (1)
67-76: Consider extracting the callback path to a shared constant.The path
/integrations/slack/callbackis duplicated between this controller andSlackCallbackController. While the comment helps maintain consistency, a shared constant would eliminate the risk of drift.// In a shared location (e.g., SlackConstants.java): public static final String SLACK_CALLBACK_PATH = "/integrations/slack/callback";webapp/src/routes/_authenticated/settings.tsx (1)
102-118: Consider invalidating Slack query after mutations for consistency.The settings mutations use
onSettledto invalidate queries, but Slack mutations only usesetQueryData. While this works since the server response is authoritative, adding invalidation would ensure consistency if cache state diverges.const syncMutation = useMutation({ ...syncSlackConnectionMutation(), onSuccess: (data) => { client.setQueryData(getSlackConnectionQueryKey(), data); toast.success("Slack synced"); }, onError: () => toast.error("Failed to sync Slack."), + onSettled: () => client.invalidateQueries({ queryKey: getSlackConnectionQueryKey() }), });server/application-server/src/main/java/de/tum/in/www1/hephaestus/account/SlackConnectionService.java (1)
161-167: Consider URL-encoding the realm name.The
realmvalue is concatenated directly into the URL without encoding. While realm names are typically alphanumeric, special characters could break the URL.private String getLinkUrl(String keycloakBaseUrl) { if (!slackEnabled || keycloakBaseUrl == null) { return null; } // Keycloak Account Console v3 - Identity Providers section - return keycloakBaseUrl + "/realms/" + realm + "/account/#/security/linked-accounts"; + return keycloakBaseUrl + "/realms/" + URLEncoder.encode(realm, StandardCharsets.UTF_8) + "/account/#/security/linked-accounts"; }Note: Requires importing
java.net.URLEncoderandjava.nio.charset.StandardCharsets.server/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceSlackIntegrationService.java (2)
176-224: Harden state parsing: preserve empty parts + reject far-future timestamps.
String.split()drops trailing empty parts; and a far-future timestamp (even though signed) makes debugging harder if clocks drift.@@ - String[] parts = decoded.split("\\|"); + String[] parts = decoded.split("\\|", -1); if (parts.length != 3) { throw new IllegalArgumentException("Malformed state parameter"); } @@ long now = Instant.now().getEpochSecond(); + // Allow small clock skew, but reject obviously broken timestamps + if (timestamp > now + 60) { + throw new IllegalArgumentException("State timestamp is in the future"); + } if (now - timestamp > STATE_EXPIRY_SECONDS) { throw new IllegalArgumentException("State token expired"); }
247-251: MakeconstantTimeEqualsnull-safe.
Prevents accidental NPEs if future refactors allow null inputs.@@ private boolean constantTimeEquals(String a, String b) { + if (a == null || b == null) { + return false; + } byte[] aBytes = a.getBytes(StandardCharsets.UTF_8); byte[] bBytes = b.getBytes(StandardCharsets.UTF_8); return MessageDigest.isEqual(aBytes, bBytes); }server/application-server/openapi.yaml (1)
150-191: Add auth error responses (or document global security) for Slack connection endpoints.
Right now only200is modeled; clients may assume these can’t return401/403. If security is global, fine—but then keep endpoint-level docs consistent across the spec.server/application-server/src/main/java/de/tum/in/www1/hephaestus/account/AccountController.java (1)
228-234: NormalizekeycloakIssuerUri(blank/null/trailing slash) before deriving base URL.
Returning""can create malformedlinkUrls downstream.@@ private String getKeycloakBaseUrl() { // Strip /realms/REALM from issuer URI to get base URL - if (keycloakIssuerUri != null && keycloakIssuerUri.contains("/realms/")) { - return keycloakIssuerUri.substring(0, keycloakIssuerUri.indexOf("/realms/")); + if (keycloakIssuerUri == null || keycloakIssuerUri.isBlank()) { + return null; + } + String issuer = keycloakIssuerUri.endsWith("/") ? keycloakIssuerUri.substring(0, keycloakIssuerUri.length() - 1) : keycloakIssuerUri; + if (issuer.contains("/realms/")) { + return issuer.substring(0, issuer.indexOf("/realms/")); } - return keycloakIssuerUri; + return issuer; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (43)
AGENTS.md(1 hunks)STYLE_GUIDE.md(1 hunks)docker/.env.example(1 hunks)docker/compose.app.yaml(1 hunks)docker/compose.core.yaml(2 hunks)docker/preview/compose.app.yaml(1 hunks)docs/admin/production-setup.mdx(1 hunks)docs/admin/slack-integration.mdx(1 hunks)docs/contributor/local-development.mdx(1 hunks)docs/sidebars.admin.ts(1 hunks)server/application-server/.env.example(1 hunks)server/application-server/compose.yaml(1 hunks)server/application-server/keycloak-hephaestus-realm-example-config.json(34 hunks)server/application-server/openapi.yaml(7 hunks)server/application-server/pom.xml(2 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/account/AccountController.java(3 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/account/SlackConnectionDTO.java(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/account/SlackConnectionService.java(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/User.java(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/UserInfoDTO.java(2 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/leaderboard/tasks/SlackWeeklyLeaderboardTask.java(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/SlackCallbackController.java(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceSlackController.java(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceSlackIntegrationService.java(1 hunks)server/application-server/src/main/resources/application-prod.yml(1 hunks)server/application-server/src/main/resources/application.yml(1 hunks)server/application-server/src/main/resources/db/changelog/1765321080884_changelog.xml(1 hunks)server/application-server/src/main/resources/db/master.xml(1 hunks)server/application-server/src/test/java/de/tum/in/www1/hephaestus/account/SlackConnectionServiceTest.java(1 hunks)server/application-server/src/test/java/de/tum/in/www1/hephaestus/leaderboard/tasks/SlackWeeklyLeaderboardTaskMatchingTest.java(1 hunks)server/application-server/src/test/java/de/tum/in/www1/hephaestus/workspace/WorkspaceSlackIntegrationServiceTest.java(1 hunks)webapp/src/api/@tanstack/react-query.gen.ts(4 hunks)webapp/src/api/sdk.gen.ts(4 hunks)webapp/src/api/types.gen.ts(5 hunks)webapp/src/components/admin/AdminSettingsPage.stories.tsx(2 hunks)webapp/src/components/admin/AdminSettingsPage.tsx(4 hunks)webapp/src/components/admin/AdminSlackSettings.tsx(1 hunks)webapp/src/components/settings/SettingsPage.stories.tsx(4 hunks)webapp/src/components/settings/SettingsPage.tsx(2 hunks)webapp/src/components/settings/SlackConnectionSection.stories.tsx(1 hunks)webapp/src/components/settings/SlackConnectionSection.tsx(1 hunks)webapp/src/routes/_authenticated/settings.tsx(3 hunks)webapp/src/routes/_authenticated/w/$workspaceSlug/admin/_admin/settings.tsx(4 hunks)
🧰 Additional context used
📓 Path-based instructions (16)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/instructions/general-coding.instructions.md)
**/*.{ts,tsx}: Use PascalCase for component names, interfaces, and type aliases
Use camelCase for variables, functions, and methods
Prefix private class members with underscore (_)
Use ALL_CAPS for constants
Use try/catch blocks for async operations
Implement proper error boundaries in React components
Always log errors with contextual information
**/*.{ts,tsx}: Ship new code as TypeScript. Reach fortypealiases when composing shapes andinterfacewhen you need declaration merging or extension.
Default toconst, mark collectionsreadonlywhen practical, and avoid mutating function arguments.
Rely on optional chaining (?.) and nullish coalescing (??) when working with generated API responses.
Export prop and return-value types so other modules can reuse them (for exampleexport interface MentorCardProps).
Import with the@/*alias defined intsconfig.json. Keep relative paths shallow.
Use discriminated unions orzodschemas for request and response guards. Assert intent with thesatisfiesoperator instead of broad casts.
Model async flows withPromiseSettledResulthelpers and exhaustively handle states.
Author components as named functions (export function Component(props: ComponentProps)). AvoidReact.FC; annotate props explicitly.
Obey the Rules of Hooks. No conditional hook calls or hook invocations inside loops.
Keep render logic pure—never mutate stores, perform navigation, or touch the DOM during render.
Prefer named exports for components and hooks so tree shaking stays predictable.
Fetch data with TanStack Query v5. Spread the generated option helpers fromsrc/api/@tanstack/react-query.gen.tsintouseQuery/useMutationso request typing, retries, and query keys stay in sync with the server.
Use the sharedQueryClientprovided bysrc/integrations/tanstack-query/root-provider.tsx. Invalidate with the generated...QueryKey()helpers instead of literal arrays.
Reuse the preconfigured OpenAPIclient(set up in `main.t...
Files:
webapp/src/components/admin/AdminSlackSettings.tsxwebapp/src/components/settings/SlackConnectionSection.tsxwebapp/src/components/settings/SettingsPage.stories.tsxwebapp/src/components/settings/SlackConnectionSection.stories.tsxwebapp/src/routes/_authenticated/w/$workspaceSlug/admin/_admin/settings.tsxwebapp/src/api/sdk.gen.tsdocs/sidebars.admin.tswebapp/src/components/admin/AdminSettingsPage.stories.tsxwebapp/src/components/admin/AdminSettingsPage.tsxwebapp/src/api/@tanstack/react-query.gen.tswebapp/src/routes/_authenticated/settings.tsxwebapp/src/api/types.gen.tswebapp/src/components/settings/SettingsPage.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use descriptive variable and function names in code
Files:
webapp/src/components/admin/AdminSlackSettings.tsxwebapp/src/components/settings/SlackConnectionSection.tsxwebapp/src/components/settings/SettingsPage.stories.tsxwebapp/src/components/settings/SlackConnectionSection.stories.tsxwebapp/src/routes/_authenticated/w/$workspaceSlug/admin/_admin/settings.tsxwebapp/src/api/sdk.gen.tsdocs/sidebars.admin.tswebapp/src/components/admin/AdminSettingsPage.stories.tsxwebapp/src/components/admin/AdminSettingsPage.tsxwebapp/src/api/@tanstack/react-query.gen.tswebapp/src/routes/_authenticated/settings.tsxwebapp/src/api/types.gen.tswebapp/src/components/settings/SettingsPage.tsx
webapp/src/{routes,components}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
webapp/src/{routes,components}/**/*.{ts,tsx}: Follow container/presentation split: route files undersrc/routes/**fetch data and pass to components undersrc/components/**. Keep components pure and side-effect free
Fetch data exclusively with TanStack Query v5 and the generated helpers in@/api/@tanstack/react-query.gen.ts. Spread option objects:useQuery(getTeamsOptions({ ... }))
Files:
webapp/src/components/admin/AdminSlackSettings.tsxwebapp/src/components/settings/SlackConnectionSection.tsxwebapp/src/components/settings/SettingsPage.stories.tsxwebapp/src/components/settings/SlackConnectionSection.stories.tsxwebapp/src/routes/_authenticated/w/$workspaceSlug/admin/_admin/settings.tsxwebapp/src/components/admin/AdminSettingsPage.stories.tsxwebapp/src/components/admin/AdminSettingsPage.tsxwebapp/src/routes/_authenticated/settings.tsxwebapp/src/components/settings/SettingsPage.tsx
webapp/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
webapp/src/**/*.{ts,tsx}: Do not callfetchdirectly; reuse the generated@hey-apiclient configured insrc/api/client.tsand the shared QueryClient fromsrc/integrations/tanstack-query/root-provider.tsx
TypeScript style in webapp: explicit prop interfaces, noReact.FC, favourtypealiases for composite shapes, prefer discriminated unions or Zod guards when needed, keep imports using@/*alias, avoid deep relative paths
Files:
webapp/src/components/admin/AdminSlackSettings.tsxwebapp/src/components/settings/SlackConnectionSection.tsxwebapp/src/components/settings/SettingsPage.stories.tsxwebapp/src/components/settings/SlackConnectionSection.stories.tsxwebapp/src/routes/_authenticated/w/$workspaceSlug/admin/_admin/settings.tsxwebapp/src/api/sdk.gen.tswebapp/src/components/admin/AdminSettingsPage.stories.tsxwebapp/src/components/admin/AdminSettingsPage.tsxwebapp/src/api/@tanstack/react-query.gen.tswebapp/src/routes/_authenticated/settings.tsxwebapp/src/api/types.gen.tswebapp/src/components/settings/SettingsPage.tsx
webapp/src/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Tailwind utility classes + shadcn primitives in
src/components/ui. Compose class names withclsx/tailwind-merge; use tokenized colors (bg-surface,text-muted, etc.)
Files:
webapp/src/components/admin/AdminSlackSettings.tsxwebapp/src/components/settings/SlackConnectionSection.tsxwebapp/src/components/settings/SettingsPage.stories.tsxwebapp/src/components/settings/SlackConnectionSection.stories.tsxwebapp/src/components/admin/AdminSettingsPage.stories.tsxwebapp/src/components/admin/AdminSettingsPage.tsxwebapp/src/components/settings/SettingsPage.tsx
webapp/src/components/**/*.{ts,tsx,stories.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Follow shadcn patterns for accessibility, wire up ARIA roles, and manage focus for dialogs/menus. Storybook stories should cover default/variant/edge states
Files:
webapp/src/components/admin/AdminSlackSettings.tsxwebapp/src/components/settings/SlackConnectionSection.tsxwebapp/src/components/settings/SettingsPage.stories.tsxwebapp/src/components/settings/SlackConnectionSection.stories.tsxwebapp/src/components/admin/AdminSettingsPage.stories.tsxwebapp/src/components/admin/AdminSettingsPage.tsxwebapp/src/components/settings/SettingsPage.tsx
webapp/src/{components,routes}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
React Compiler (via
babel-plugin-react-compilerinvite.config.js) automatically optimizes components. Do not useuseMemo,useCallback, orReact.memofor new code—the compiler handles memoization automatically
Files:
webapp/src/components/admin/AdminSlackSettings.tsxwebapp/src/components/settings/SlackConnectionSection.tsxwebapp/src/components/settings/SettingsPage.stories.tsxwebapp/src/components/settings/SlackConnectionSection.stories.tsxwebapp/src/routes/_authenticated/w/$workspaceSlug/admin/_admin/settings.tsxwebapp/src/components/admin/AdminSettingsPage.stories.tsxwebapp/src/components/admin/AdminSettingsPage.tsxwebapp/src/routes/_authenticated/settings.tsxwebapp/src/components/settings/SettingsPage.tsx
**/*.stories.{ts,tsx}
📄 CodeRabbit inference engine (.github/instructions/tsx.instructions.md)
**/*.stories.{ts,tsx}: Keep stories colocated and represent the real UI states that Chromatic validates.
Use Storybook play functions for interactive flows instead of end-to-end tests when the surface is presentational.
Files:
webapp/src/components/settings/SettingsPage.stories.tsxwebapp/src/components/settings/SlackConnectionSection.stories.tsxwebapp/src/components/admin/AdminSettingsPage.stories.tsx
server/application-server/src/main/java/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
server/application-server/src/main/java/**/*.java: Keep business logic in services annotated with@Serviceand use transactional boundaries (@Transactional) where needed. Controllers should be thin (input validation + delegation)
Use Lombok consistently (@Getter,@Setter, etc.) but prefer explicit builders or records when immutability helps
Files:
server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/User.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/SlackCallbackController.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/leaderboard/tasks/SlackWeeklyLeaderboardTask.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/account/SlackConnectionDTO.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceSlackController.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/account/SlackConnectionService.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/account/AccountController.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceSlackIntegrationService.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/UserInfoDTO.java
server/application-server/src/main/resources/db/changelog/*.xml
📄 CodeRabbit inference engine (AGENTS.md)
Liquibase changelog files live under
server/application-server/src/main/resources/db/changelog/and must be included viamaster.xml. Trim generated changelogs to minimal delta before committing
Files:
server/application-server/src/main/resources/db/changelog/1765321080884_changelog.xml
server/application-server/src/main/{java/**/entity/**,resources/db/changelog}/**/*.{java,xml}
📄 CodeRabbit inference engine (AGENTS.md)
Keep Liquibase changelog IDs monotonic and descriptive. Align entity annotations with the generated change sets
Files:
server/application-server/src/main/resources/db/changelog/1765321080884_changelog.xml
server/application-server/src/test/**/*.java
📄 CodeRabbit inference engine (.github/instructions/java-tests.instructions.md)
server/application-server/src/test/**/*.java: Each test case should have a single responsibility: one behavior or assertion per test
Test cases should have clear and concise objectives: state the objective simply without ambiguity
Test cases must be independent with no hidden dependencies between tests
Test cases should be traceable: link directly to requirements
Test cases must be repeatable with consistent setup and data
Test cases should be maintainable: easy to update when implementation changes
Focus on risk: cover critical flows and edge cases first when writing tests
Test cases should have minimal setup: avoid unnecessary initialization steps
Test cases must execute fast to fit seamlessly into CI pipelines
Test cases should use realistic data: use representative scenarios that mirror production use cases
Test cases should have concise expected results: one clear outcome per test case
Test cases should follow Arrange-Act-Assert (AAA) structure: keep structure clear with distinct setup, execution, and verification phases
Test cases may run in parallel, so avoid required cleanup steps and assume that there might be data from previous tests in the database
Files:
server/application-server/src/test/java/de/tum/in/www1/hephaestus/workspace/WorkspaceSlackIntegrationServiceTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/account/SlackConnectionServiceTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/leaderboard/tasks/SlackWeeklyLeaderboardTaskMatchingTest.java
server/application-server/src/test/java/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
Group new Java tests under proper JUnit tag so CI picks them up (
@Tag("unit"),@Tag("integration"), or@Tag("live")) and follow AAA structure with single assertion focus
Files:
server/application-server/src/test/java/de/tum/in/www1/hephaestus/workspace/WorkspaceSlackIntegrationServiceTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/account/SlackConnectionServiceTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/leaderboard/tasks/SlackWeeklyLeaderboardTaskMatchingTest.java
webapp/src/routes/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Declare new routes via
createFileRoute, respect loader/guard conventions, and keep loader side effects out of render paths
Files:
webapp/src/routes/_authenticated/w/$workspaceSlug/admin/_admin/settings.tsxwebapp/src/routes/_authenticated/settings.tsx
webapp/src/api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never hand-edit
webapp/src/api/**/*,webapp/src/api/@tanstack/react-query.gen.ts,webapp/src/api/client.gen.ts,webapp/src/api/types.gen.ts— regenerate vianpm run generate:api:application-server:client
Files:
webapp/src/api/sdk.gen.tswebapp/src/api/@tanstack/react-query.gen.tswebapp/src/api/types.gen.ts
server/application-server/openapi.yaml
📄 CodeRabbit inference engine (AGENTS.md)
Never hand-edit
server/application-server/openapi.yaml— regenerate vianpm run generate:api:application-server:specs
Files:
server/application-server/openapi.yaml
🧬 Code graph analysis (11)
webapp/src/components/admin/AdminSlackSettings.tsx (2)
webapp/src/components/ui/card.tsx (5)
Card(85-85)CardHeader(86-86)CardTitle(88-88)CardDescription(90-90)CardContent(91-91)webapp/src/components/ui/button.tsx (1)
Button(61-61)
server/application-server/src/test/java/de/tum/in/www1/hephaestus/workspace/WorkspaceSlackIntegrationServiceTest.java (1)
server/application-server/src/test/java/de/tum/in/www1/hephaestus/testconfig/BaseUnitTest.java (1)
ExtendWith(11-15)
webapp/src/components/settings/SlackConnectionSection.stories.tsx (2)
webapp/src/components/settings/SlackConnectionSection.tsx (1)
SlackConnectionSection(25-103)webapp/src/components/settings/SettingsPage.stories.tsx (1)
Loading(94-115)
webapp/src/api/sdk.gen.ts (1)
webapp/src/api/types.gen.ts (10)
SlackCallbackData(1690-1698)SlackCallbackResponses(1700-1705)DisconnectSlackData(1753-1758)DisconnectSlackResponses(1760-1765)GetSlackConnectionData(1769-1774)GetSlackConnectionResponses(1776-1781)SyncSlackConnectionData(1785-1790)SyncSlackConnectionResponses(1792-1797)InstallSlackBotData(2757-2767)InstallSlackBotResponses(2769-2774)
webapp/src/components/admin/AdminSettingsPage.stories.tsx (1)
webapp/src/components/admin/AdminTeamsTable.stories.tsx (1)
Story(130-130)
server/application-server/src/test/java/de/tum/in/www1/hephaestus/account/SlackConnectionServiceTest.java (1)
server/application-server/src/test/java/de/tum/in/www1/hephaestus/testconfig/BaseUnitTest.java (1)
ExtendWith(11-15)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/account/SlackConnectionService.java (1)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceSlackIntegrationService.java (1)
Service(38-259)
webapp/src/api/@tanstack/react-query.gen.ts (2)
webapp/src/api/sdk.gen.ts (6)
Options(7-19)slackCallback(36-41)disconnectSlack(84-89)getSlackConnection(95-100)syncSlackConnection(106-111)installSlackBot(533-538)webapp/src/api/types.gen.ts (7)
SlackCallbackData(1690-1698)DisconnectSlackData(1753-1758)DisconnectSlackResponse(1767-1767)GetSlackConnectionData(1769-1774)SyncSlackConnectionData(1785-1790)SyncSlackConnectionResponse(1799-1799)InstallSlackBotData(2757-2767)
webapp/src/routes/_authenticated/settings.tsx (1)
webapp/src/api/@tanstack/react-query.gen.ts (6)
getUserSettingsQueryKey(103-103)getUserSettingsOptions(109-122)getSlackConnectionOptions(187-200)syncSlackConnectionMutation(227-239)getSlackConnectionQueryKey(181-181)disconnectSlackMutation(167-179)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceSlackIntegrationService.java (1)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/account/SlackConnectionService.java (1)
Service(28-168)
webapp/src/components/settings/SettingsPage.tsx (5)
webapp/src/components/settings/NotificationsSection.tsx (1)
NotificationsSectionProps(4-17)webapp/src/components/settings/ResearchParticipationSection.tsx (2)
ResearchParticipationSectionProps(4-17)ResearchParticipationSection(23-67)webapp/src/components/settings/AccountSection.tsx (2)
AccountSectionProps(14-27)AccountSection(33-92)webapp/src/components/settings/SlackConnectionSection.tsx (2)
SlackConnectionSectionProps(15-23)SlackConnectionSection(25-103)webapp/src/integrations/posthog/config.ts (1)
isPosthogEnabled(33-34)
🪛 ast-grep (0.40.0)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceSlackIntegrationService.java
[warning] 230-230: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Mac.getInstance(HMAC_SHA256)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 230-230: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Mac.getInstance(HMAC_SHA256)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
🪛 dotenv-linter (4.0.0)
docker/.env.example
[warning] 182-182: [UnorderedKey] The KEYCLOAK_SLACK_CLIENT_ID key should go before the KEYCLOAK_SLACK_ENABLED key
(UnorderedKey)
[warning] 183-183: [UnorderedKey] The KEYCLOAK_SLACK_CLIENT_SECRET key should go before the KEYCLOAK_SLACK_ENABLED key
(UnorderedKey)
[warning] 188-188: [ExtraBlankLine] Extra blank line detected
(ExtraBlankLine)
server/application-server/.env.example
[warning] 4-4: [UnorderedKey] The KEYCLOAK_GITHUB_ADMIN_USERNAME key should go before the KEYCLOAK_GITHUB_CLIENT_ID key
(UnorderedKey)
🪛 LanguageTool
AGENTS.md
[uncategorized] ~3-~3: The official name of this software platform is spelled with a capital “H”.
Context: ...ails with the scoped instructions under .github/instructions/** (general coding, TSX, ...
(GITHUB)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: docker-build / intelligence-service-build / Build linux/amd64 Docker Image for ls1intum/hephaestus/intelligence-service
- GitHub Check: docker-build / intelligence-service-build / Build linux/arm64 Docker Image for ls1intum/hephaestus/intelligence-service
- GitHub Check: docker-build / webapp-build / Build linux/amd64 Docker Image for ls1intum/hephaestus/webapp
- GitHub Check: docker-build / webapp-build / Build linux/arm64 Docker Image for ls1intum/hephaestus/webapp
- GitHub Check: quality-gates / database-documentation-validation
- GitHub Check: quality-gates / docs-quality
- GitHub Check: quality-gates / database-schema-validation
- GitHub Check: quality-gates / webapp-quality
- GitHub Check: security-scan / dependencies
- GitHub Check: test-suite / webapp-visual
- GitHub Check: security-scan / sast
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (java)
- GitHub Check: Docs / Build (quality gate)
🔇 Additional comments (54)
docker/preview/compose.app.yaml (1)
276-277: LGTM — feature-flagged configuration addition.Default-disabled Slack linking in preview deployments is appropriate; the comment clarifies the setting's purpose.
server/application-server/compose.yaml (1)
26-34: LGTM — Slack IdP and proxy configuration for local development.The configuration is safe, disabled by default, and the comments guide developers. Keycloak proxy settings (
KC_PROXY_HEADERS: xforwarded,KC_HTTP_ENABLED: true) are appropriate for local ngrok/proxy scenarios.Please confirm that the referenced documentation (
docs/dev/slack-account-linking.md) exists and contains clear setup instructions for developers enabling Slack locally.server/application-server/pom.xml (2)
493-494: Verify necessity of extended wait time and JMX configuration.The 30-second
<wait>timeout is substantially longer than typical Spring Boot defaults (300ms–1000ms). This will slow downmvn spring-boot:runand OpenAPI spec generation during CI pipelines.Questions:
- Is the 30-second wait necessary? If so, please document the rationale (e.g., artifact download, slow CI runner).
- If this is temporary (e.g., for a specific build environment), consider using an environment variable override or Maven property.
- Is JMX port 9001 exposed intentionally? Confirm it doesn't conflict with other services or create unintended security surface.
Suggest adding a comment explaining the extended timeout rationale:
+ <!-- Wait longer for slow CI environments or large dependency downloads --> <wait>30000</wait>Or consider making it conditional:
- <wait>30000</wait> + <wait>${spring-boot.wait.timeout:-30000}</wait>
508-508: Verify RMI/JMX configuration for OpenAPI spec generation.The extended JVM arguments enable RMI and JMX management for the OpenAPI spec generation phase. Confirm that:
- The hostname binding to
127.0.0.1and JMX port9001don't conflict with other services.- Developers are aware of the exposed management port during local builds.
- The
spring.application.admin.jmx-nameconfiguration is necessary for Spring Boot Admin or another monitoring tool.server/application-server/src/main/resources/db/master.xml (1)
38-38: LGTM — changelog inclusion properly ordered.The Slack changelog is correctly included in chronological order. Verify that the referenced file (
./changelog/1765321080884_changelog.xml) contains only theslack_user_idcolumn addition and no other unrelated schema changes.Please confirm that the changelog file adds the
slack_user_idnullable column to theusertable and nothing more.server/application-server/src/main/resources/application.yml (1)
162-168: LGTM — Slack configuration properly feature-flagged.The configuration is safe, well-documented, and aligns with environment variables defined in Docker Compose files. Default-disabled behavior and empty credential defaults are appropriate.
STYLE_GUIDE.md (1)
1-78: LGTM — comprehensive style guide for the project.The guide is well-structured, with clear absolutes, language-specific recommendations, and actionable patterns. The React Compiler guidance and mutation naming examples are particularly helpful.
Adoption note: Verify that this guide is integrated into:
- CI/CD linting rules (ESLint, Biome, Checkstyle, Spotbugs for Java)
- Pre-commit hooks or GitHub Actions enforcements
- Code review checklists
Without tooling enforcement, style guides risk becoming advisory only. Consider flagging high-priority rules (e.g., "no
elseblocks," "noanytype") in lint configuration.Please confirm that linting and CI rules are updated to enforce high-priority style rules (e.g., no bare
else, noanyin TypeScript, constructor injection in Java).AGENTS.md (1)
3-3: LGTM — STYLE_GUIDE.md reference added correctly.The Markdown link is properly formatted and positioned early in the handbook. This ensures developers encounter the style guide alongside other foundational instructions.
server/application-server/keycloak-hephaestus-realm-example-config.json (2)
1642-1670: LGTM — Slack OIDC identity provider correctly configured.The Slack IdP is secure and well-designed:
linkOnly: trueensures Slack is for account linking, not primary authentication.hideOnLogin: trueprevents confusion on the login page.- Signature validation and JWKS URL usage follow security best practices.
- Scope
"openid email profile"is minimal and appropriate.- Environment variable templating with safe defaults prevents accidental misconfiguration.
Please verify that the Slack OIDC endpoints remain current:
https://slack.com/api/openid.connect.tokenhttps://slack.com/api/openid.connect.userInfohttps://slack.com/openid/connect/keyshttps://slack.com/openid/connect/authorizeIf these endpoints have changed, update the configuration to match Slack's latest OpenID Connect documentation.
66-75: LGTM — JSON formatting changes are cosmetic and safe.The reflowing of arrays and objects throughout the file improves readability without changing semantics. All role composites, client scopes, and protocol mapper configurations remain functionally identical.
Also applies to: 242-246, 365-367, 435-437
docker/compose.app.yaml (1)
105-109: LGTM! Secure defaults for optional Slack integration.The environment variables follow secure defaults (disabled by default, empty credentials), and the comments clearly indicate the optional nature and Keycloak dependency. This aligns with defense-in-depth principles.
docker/.env.example (1)
153-187: Excellent documentation for Slack integration.The comprehensive documentation clearly distinguishes between:
- OpenID Connect flow for user identity linking (Keycloak)
- OAuth v2 flow for workspace bot installation
The separation of concerns and explicit redirect URLs will help operators configure this correctly.
server/application-server/src/main/java/de/tum/in/www1/hephaestus/leaderboard/tasks/SlackWeeklyLeaderboardTask.java (3)
113-136: Excellent Javadoc explaining the deterministic matching strategy.The documentation clearly articulates why fuzzy matching was removed (non-unique names, false positives) and establishes a clear priority order. This will help future maintainers understand the design rationale.
148-168: Linked Slack ID matching with appropriate error handling.The priority 1 logic correctly handles:
- Successful match with debug logging
- Stale/deactivated users with a warning
The warning message at lines 161-166 provides actionable context for operators investigating why a linked user isn't being matched.
170-203: Email-based fallback with informative guidance.The fallback logic is clean and the info-level message at lines 195-201 helpfully guides users on how to enable Slack mentions. The method correctly returns null when no match is found, allowing the caller to handle the absence gracefully.
docker/compose.core.yaml (3)
102-104: LGTM! Keycloak Slack configuration properly wired.The environment variables correctly default to disabled (false) and empty credentials, consistent with the secure-by-default pattern used elsewhere.
379-380: Correct use of linkOnly and hideOnLogin for account linking.Setting
linkOnly: trueandhideOnLogin: trueis the correct configuration for a "Connect Slack" feature that links accounts rather than providing an alternative login method. This prevents users from attempting to log in with Slack alone.
369-396: The Slack identity provider configuration uses the correct OpenID Connect endpoints as documented in Slack's official OIDC specification. No changes needed.server/application-server/.env.example (1)
1-19: Well-organized environment variable structure.The clear separation between GitHub OAuth (required), PAT mode (alternative), and Slack integration (optional) makes it easy for developers to understand the configuration options. The ngrok note is a helpful addition for local Slack testing.
webapp/src/components/settings/SlackConnectionSection.tsx (2)
38-41: Secure window.open usage.The
handleConnectfunction correctly usesnoopener,noreferrerflags when opening the Slack OAuth flow in a new window, preventing potential security issues (tabnapping, referrer leakage).
44-52: Good accessibility practices.The component uses semantic HTML with proper ARIA labeling (
aria-labelledby, heading hierarchy) and descriptive text for screen readers.server/application-server/src/test/java/de/tum/in/www1/hephaestus/leaderboard/tasks/SlackWeeklyLeaderboardTaskMatchingTest.java (2)
86-227: Excellent test coverage for the deterministic matching strategy.The nested test structure clearly documents the matching behavior:
- Linked Slack ID takes priority (lines 86-146)
- Email matching as fallback (lines 148-187)
- Explicit verification that fuzzy matching is NOT performed (lines 189-227)
The test at lines 194-209 ("should NOT match 'Khiem Nguyen' to 'Khoa Nguyen'") directly validates the critical bug fix mentioned in the PR context.
229-286: Comprehensive edge case coverage.The edge case tests properly handle:
- Null user in entry
- Missing email with no linked ID
- Empty Slack user list
- Null Slack profile
This defensive testing ensures the mapper won't throw NPEs in production.
server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/User.java (1)
118-119: Database migration confirmed for slackUserId column.The Liquibase migration file (
1765321080884_changelog.xml) exists and correctly adds theslack_user_idcolumn to theusertable with typeVARCHAR(255). The migration includes proper preconditions and documentation.webapp/src/components/admin/AdminSettingsPage.stories.tsx (2)
69-70: LGTM! Slack integration props added to stories.The new
workspaceSlugandhasSlackTokenprops are properly typed and align with the Slack integration UI changes inAdminSettingsPage.
85-92: Good coverage of Slack connection state.The
ConnectedToSlackstory provides visual regression testing for the Slack-connected UI state, which aligns with the Storybook guideline to represent real UI states.server/application-server/src/main/resources/application-prod.yml (1)
73-79: Well-structured Slack integration configuration.The new
hephaestus.slackblock follows the existing pattern for service integrations (likeposthog), uses appropriate environment variable placeholders, and defaults to disabled for safe production deployment. The comments clearly explain the relationship to Keycloak's Slack IdP and the bot installation flow.docs/sidebars.admin.ts (1)
4-7: LGTM! Slack Integration properly added to admin sidebar.The new entry follows the Docusaurus sidebar config structure and maintains consistency with the existing Production Setup entry.
docs/contributor/local-development.mdx (1)
159-189: Comprehensive Slack integration setup for local development.The optional Slack integration section clearly explains the HTTPS requirement, provides step-by-step instructions, and correctly notes the need to access Keycloak via the ngrok URL for testing. The scopes and environment variables align with the production setup documentation.
docs/admin/production-setup.mdx (1)
36-48: Good documentation structure for Slack integration.The optional Slack integration subsection follows the DRY principle by referencing the dedicated guide rather than duplicating setup steps. The environment variable table is complete and matches the configuration in
application-prod.yml.server/application-server/src/main/java/de/tum/in/www1/hephaestus/account/SlackConnectionDTO.java (1)
1-23: Well-designed DTO with clear factory methods.The record structure is appropriate for an immutable DTO, and the static factory methods (
connected()anddisconnected()) provide clear construction semantics. The@JsonInclude(NON_EMPTY)annotation correctly omits null/empty fields while preserving primitive boolean values.server/application-server/src/main/resources/db/changelog/1765321080884_changelog.xml (1)
1-17: Changelog properly included in master.xml.The changelog has been correctly added to
server/application-server/src/main/resources/db/master.xmlat line 38. The timestamp-based ID is monotonic, preConditions prevent duplicate execution, and the nullable column is appropriate for optional Slack linking. The structure aligns with Liquibase best practices and coding guidelines.webapp/src/routes/_authenticated/w/$workspaceSlug/admin/_admin/settings.tsx (1)
15-24: Good:validateSearch+Route.useSearch()makes Slack status handling safer.Optional improvement: export
type SlackStatus = z.infer<typeof searchSchema>["slack"]and thread that type intoAdminSettingsPageso the toast switch can be exhaustive.Also applies to: 36-37, 161-164
webapp/src/components/settings/SettingsPage.stories.tsx (1)
15-32: Nice: stories now cover SettingsPage with Slack props across key states.Also applies to: 41-63, 68-89, 94-115
webapp/src/components/settings/SettingsPage.tsx (1)
12-15: LGTM: Slack section integrated cleanly and loading is derived consistently.Also applies to: 17-23, 25-69
server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/UserInfoDTO.java (1)
7-16: Good:slackUserIdis added in a null-friendly way and wired through the factory.Also applies to: 21-31
webapp/src/components/settings/SlackConnectionSection.stories.tsx (1)
1-54: LGTM! Well-structured Storybook stories covering all UI states.The stories comprehensively cover the four key states (Disconnected, Connected, Loading, Disabled) with appropriate prop combinations. The use of
satisfiesfor type safety and the centered layout decorator with fixed width provide good visual testing coverage.server/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/SlackCallbackController.java (2)
94-99: Verify redirect behavior whenhephaestus.webapp.urlis not configured.When
webappUrlis blank, redirects become relative paths (e.g.,/settings?slack=error). This may fail if the webapp is hosted on a different origin or behind a reverse proxy. Consider logging a warning or throwing an error if the URL is not configured in production.
74-88: LGTM! Proper OAuth callback handling with appropriate error categorization.The flow correctly validates the state parameter for CSRF protection, exchanges the code for tokens via the service, and provides meaningful redirect states for different failure modes.
webapp/src/routes/_authenticated/settings.tsx (1)
120-150: LGTM! Clean integration of Slack props with proper loading state composition.The Slack connection state is correctly wired to the SettingsPage component with comprehensive loading state aggregation from query and both mutations.
server/application-server/src/main/java/de/tum/in/www1/hephaestus/account/SlackConnectionService.java (2)
88-130: LGTM! Robust sync logic with proper edge case handling.The method correctly handles null/empty Keycloak user IDs, clears stale Slack IDs when the identity is unlinked, and gracefully returns current status on errors to avoid breaking the user flow.
67-75: LGTM! Efficient status check prioritizing local cache.Checking the User entity first before querying Keycloak is a good optimization that reduces external API calls.
webapp/src/api/sdk.gen.ts (3)
80-112: Slack connection endpoints look consistent with the REST surface.
DELETE/GET/POST/user/slack-connection*wrappers are straightforward.
529-538:installSlackBot()should not be called via XHR—this endpoint must redirect the browser to Slack's OAuth authorization URL.The correct pattern for Slack bot installation is direct browser navigation to the install endpoint (as currently done with
window.location.hrefinAdminSlackSettings.tsx), not a fetch/XHR request. The backend endpoint should return a 302 redirect to Slack's authorize URL with the proper state and scopes pre-configured. The generated SDK function exposes this as a data operation, which is structurally incorrect for a redirect endpoint and should never be invoked.
32-41: This is a generated file that cannot be edited; the actual issue is in the backend OpenAPI spec design.The
/integrations/slack/callbackendpoint is documented inopenapi.yamlto return302 Redirectas its primary response. When this endpoint is called via XHR (as the generatedslackCallback()wrapper would), the @hey-api client follows the redirect by default (Fetch API behavior) and returns the final response body (HTML), not a typed API response. This is a fundamental mismatch: Slack OAuth callbacks should be browser navigations, not API calls.However,
slackCallback()is not invoked anywhere in the webapp, so this doesn't currently impact functionality. The proper fix is to either:
- Handle the callback server-side (don't expose it as an API endpoint), or
- If frontend involvement is needed, navigate directly via
window.location.hrefinstead of calling via the clientSince this is generated code, the fix belongs in the OpenAPI schema or backend endpoint design.
server/application-server/openapi.yaml (2)
1345-1371: Install endpoint returns 302; confirm the generated client is meant to call it.
If frontend uses the generated SDK, consider returning200 { url: string }instead of302(or keep 302 but ensure UI uses navigation, not fetch).
3666-3668:UserInfo.slackUserIdshould also be nullable.
Same contract issue asSlackConnection.slackUserId.@@ UserInfo: type: object properties: @@ slackUserId: - type: string + type: [string, "null"]Likely an incorrect or invalid review comment.
server/application-server/src/main/java/de/tum/in/www1/hephaestus/account/AccountController.java (2)
48-58: Constructor injection looks good; controller stays thin.
Slack-specific logic remains inSlackConnectionService.
173-199:/synccorrectly requires auth.
This endpoint is the right place to hit Keycloak.webapp/src/api/@tanstack/react-query.gen.ts (3)
64-83: Be careful usingslackCallbackOptions()as a query (redirect endpoint).
AuseQuery()may refetch (window focus/retry), which is a bad fit for “complete OAuth + redirect” flows. Prefer a route loader that triggers navigation, or don’t model this as queryable data.
163-239: Slack connection query/mutations look fine for TanStack Query.
GET status + POST sync + DELETE disconnect map well to query/mutation usage.
1271-1291:installSlackBotOptions()is also a redirect endpoint; avoiduseQuery()here.
This should usually be a user-initiated navigation, not a background query.server/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceSlackIntegrationService.java (1)
50-67: Fail fast on missing Slack credentials (not only ingenerateInstallUrl).
completeInstallation()can currently run with blankclientId/clientSecretuntil the Slack call fails; better to validate at entry (or constructor) so failures are deterministic.@@ @Transactional public String completeInstallation(String code, String state, String redirectUri) { + if (clientId == null || clientId.isBlank() || clientSecret == null || clientSecret.isBlank()) { + throw new IllegalStateException("Slack client credentials are not configured."); + } + if (code == null || code.isBlank()) { + throw new IllegalArgumentException("Missing code parameter"); + } + if (redirectUri == null || redirectUri.isBlank()) { + throw new IllegalArgumentException("Missing redirectUri parameter"); + } String workspaceSlug = validateAndParseState(state); @@ }Likely an incorrect or invalid review comment.
webapp/src/api/types.gen.ts (1)
1-2984: Auto-generated types for Slack integration look correct.The new Slack-related types (
SlackConnection,SlackCallbackData,DisconnectSlackData,GetSlackConnectionData,SyncSlackConnectionData,InstallSlackBotData, and associated response types) follow the established @hey-api/openapi-ts generation patterns. TheUserInfoextension withslackUserIdis also properly typed as optional.As per coding guidelines, ensure these changes were regenerated via
npm run generate:api:application-server:clientrather than hand-edited.
| --- | ||
| id: slack-integration | ||
| sidebar_position: 2 | ||
| title: Slack Integration | ||
| description: Configure Slack for leaderboard notifications and user mentions. | ||
| --- | ||
|
|
||
| # Slack Integration | ||
|
|
||
| Enables weekly leaderboard notifications to Slack with @-mentions for linked users. | ||
|
|
||
| ## Setup | ||
|
|
||
| ### 1. Create Slack App | ||
|
|
||
| Go to [api.slack.com/apps](https://api.slack.com/apps) → **Create New App** → **From scratch** → Name: `Hephaestus` | ||
|
|
||
| ### 2. Configure OAuth | ||
|
|
||
| **Features → OAuth & Permissions → Redirect URLs** (add both): | ||
|
|
||
| ``` | ||
| https://YOUR_DOMAIN/keycloak/realms/hephaestus/broker/slack/endpoint | ||
| https://YOUR_DOMAIN/api/integrations/slack/callback | ||
| ``` | ||
|
|
||
| Replace `YOUR_DOMAIN` with your production domain (e.g., `hephaestus.example.com`). | ||
|
|
||
| **Scopes** (Features → OAuth & Permissions): | ||
|
|
||
| | Type | Scopes | | ||
| |------|--------| | ||
| | User Token Scopes | `openid`, `profile`, `email` | | ||
| | Bot Token Scopes | `chat:write`, `channels:read`, `users:read`, `team:read` | | ||
|
|
||
| ### 3. Get Credentials | ||
|
|
||
| From **Settings → Basic Information → App Credentials**, copy: | ||
|
|
||
| - **Client ID** → `KEYCLOAK_SLACK_CLIENT_ID` | ||
| - **Client Secret** → `KEYCLOAK_SLACK_CLIENT_SECRET` | ||
| - **Signing Secret** → `SLACK_SIGNING_SECRET` | ||
|
|
||
| ### 4. Install & Get Bot Token | ||
|
|
||
| **Features → OAuth & Permissions** → **Install to Workspace** → Copy **Bot User OAuth Token** (`xoxb-...`) → `SLACK_BOT_TOKEN` | ||
|
|
||
| ### 5. Set Environment Variables | ||
|
|
||
| Add to `docker/.env` (or your secret manager): | ||
|
|
||
| ```bash | ||
| # Required for Slack integration | ||
| KEYCLOAK_SLACK_ENABLED=true | ||
| SLACK_LINKING_ENABLED=true | ||
| KEYCLOAK_SLACK_CLIENT_ID=... | ||
| KEYCLOAK_SLACK_CLIENT_SECRET=... | ||
| SLACK_BOT_TOKEN=xoxb-... | ||
| SLACK_SIGNING_SECRET=... | ||
|
|
||
| # Leaderboard notifications (optional) | ||
| LEADERBOARD_NOTIFICATION_ENABLED=true | ||
| LEADERBOARD_NOTIFICATION_CHANNEL_ID=C0123456789 | ||
| ``` | ||
|
|
||
| To find channel ID: right-click channel in Slack → **View channel details** → scroll to bottom. | ||
|
|
||
| ### 6. Restart & Verify | ||
|
|
||
| 1. Restart the application server | ||
| 2. Go to **Workspace Settings → Slack Integration** → **Connect Workspace** | ||
| 3. Users link accounts in **Settings → Slack Connection** | ||
|
|
||
| ## Troubleshooting | ||
|
|
||
| | Issue | Solution | | ||
| |-------|----------| | ||
| | "Slack integration is not enabled" | Verify `KEYCLOAK_SLACK_ENABLED=true` and `SLACK_LINKING_ENABLED=true` | | ||
| | "Invalid state" after OAuth | Token expired (10 min). Start the flow again. | | ||
| | Bot not posting | Invite bot: `/invite @Hephaestus`. Verify `SLACK_BOT_TOKEN` and channel ID. | | ||
| | User not @-mentioned | User must link in Settings AND click **I've Linked My Account**. | |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "slack.*post|slack.*chat|slack.*user|slackClient|webClient" --type java -A 2 -B 2Repository: ls1intum/Hephaestus
Length of output: 17932
🏁 Script executed:
rg "slackApp\.client\(\)\." --type java -A 1 -B 1Repository: ls1intum/Hephaestus
Length of output: 1348
🏁 Script executed:
cat -n server/application-server/src/main/java/de/tum/in/www1/hephaestus/leaderboard/SlackMessageService.javaRepository: ls1intum/Hephaestus
Length of output: 3977
🏁 Script executed:
rg "channels\.list|conversations\.list|team\.|teams\." --type javaRepository: ls1intum/Hephaestus
Length of output: 25913
🏁 Script executed:
rg "\.client\(\)\." --type java | grep -i slack | head -20Repository: ls1intum/Hephaestus
Length of output: 586
Comprehensive Slack integration guide with minor scope documentation update needed.
The documentation is well-structured and covers the complete setup flow, from app creation through troubleshooting. The dual redirect URL requirement (Keycloak IdP and bot callback) is clearly documented, and the scope separation between User Token Scopes (for identity linking) and Bot Token Scopes (for posting) is correct.
However, the Bot Token Scopes section lists channels:read and team:read, but the codebase only uses chat:write and users:read for leaderboard functionality:
usersList()requiresusers:read✓chatPostMessage()requireschat:write✓
The channels:read and team:read scopes are not used in the current implementation. Consider removing them from the documented scopes or adding a note explaining they are reserved for future features.
🤖 Prompt for AI Agents
In docs/admin/slack-integration.mdx around lines 1 to 81, the Bot Token Scopes
list includes channels:read and team:read which are not used by the current
implementation; update the Bot Token Scopes to only list the scopes required by
the codebase (chat:write and users:read) and either remove channels:read and
team:read entirely or add a one-line note that those scopes are
optional/reserved for future features and not required for current leaderboard
functionality.
| - description: |- | ||
| Controller for managing Slack integration settings within a workspace | ||
| context. | ||
| <p> | ||
| This controller is workspace-scoped, meaning all endpoints are prefixed with | ||
| <code>/workspaces/{workspaceSlug</code>}. | ||
| name: workspace-slack-controller | ||
| - description: |- | ||
| Controller for managing workspace memberships and roles. | ||
| Handles CRUD operations for workspace members and role assignments. | ||
| name: workspace-membership-controller |
There was a problem hiding this comment.
Fix tag typos/inconsistencies (they affect generated docs + SDK grouping).
Examples in this hunk: missing } in /workspaces/{workspaceSlug...}, and tag name mismatch between declared tags and usage later (workspace-slack-controller vs workspace-slack).
🤖 Prompt for AI Agents
In server/application-server/openapi.yaml around lines 27 to 37, the tags and
path text contain typos that break generated docs/SDKs: add the missing closing
brace in the path fragment so it reads /workspaces/{workspaceSlug} and reconcile
the tag names so they are identical between the tag declaration and any usage
(choose one consistent name such as workspace-slack or
workspace-slack-controller and update all occurrences), ensuring the tag entry's
name matches how controllers/operations reference it.
| /integrations/slack/callback: | ||
| get: | ||
| description: Handles the callback from Slack after bot installation. Validates | ||
| the HMAC-signed state parameter and exchanges the authorization code for an | ||
| access token. | ||
| operationId: slackCallback | ||
| parameters: | ||
| - description: OAuth authorization code from Slack | ||
| in: query | ||
| name: code | ||
| required: false | ||
| schema: | ||
| type: string | ||
| - description: HMAC-signed state containing workspace slug | ||
| in: query | ||
| name: state | ||
| required: false | ||
| schema: | ||
| type: string | ||
| - description: Error code if user cancelled or Slack returned an error | ||
| in: query | ||
| name: error | ||
| required: false | ||
| schema: | ||
| type: string | ||
| responses: | ||
| "302": | ||
| description: Redirect to workspace settings on success or error page on | ||
| failure | ||
| "400": | ||
| description: Missing required OAuth parameters | ||
| summary: Slack OAuth Callback | ||
| tags: | ||
| - 'Workspace ' | ||
| /user: |
There was a problem hiding this comment.
Slack callback tags value looks wrong and params are modeled as optional.
tags: - 'Workspace ' (trailing space + odd quoting) is likely accidental; also consider whether code/state should be required in the “success” path (even if error exists).
@@
/integrations/slack/callback:
get:
@@
summary: Slack OAuth Callback
tags:
- - 'Workspace '
+ - Workspace IntegrationCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In server/application-server/openapi.yaml around lines 73 to 107, the Slack
callback operation has an incorrect tags entry and query parameters marked
optional: remove the trailing space and odd quoting so the tag is a normal value
(e.g. Workspace) and normalize quoting (or leave unquoted), and make the code
and state query parameters required: set required: true for both (keep error as
required: false) so the successful OAuth callback path requires code and state;
keep responses unchanged.
| SlackConnection: | ||
| type: object | ||
| description: DTO representing the user's Slack connection status. | ||
| properties: | ||
| connected: | ||
| type: boolean | ||
| description: Whether the user has linked their Slack account | ||
| linkUrl: | ||
| type: string | ||
| slackEnabled: | ||
| type: boolean | ||
| description: Whether Slack integration is enabled for this instance | ||
| slackUserId: | ||
| type: string | ||
| description: "The Slack User ID if connected, null otherwise" | ||
| SortObject: |
There was a problem hiding this comment.
SlackConnection.linkUrl / slackUserId should be nullable in the schema.
Backend behavior (and even your own description “null otherwise”) implies these can be null; modeling them as plain string will generate incorrect TS types.
@@
SlackConnection:
type: object
description: DTO representing the user's Slack connection status.
properties:
connected:
type: boolean
description: Whether the user has linked their Slack account
linkUrl:
- type: string
+ type: [string, "null"]
slackEnabled:
type: boolean
description: Whether Slack integration is enabled for this instance
slackUserId:
- type: string
+ type: [string, "null"]
description: "The Slack User ID if connected, null otherwise"
+ required: [connected, linkUrl, slackEnabled, slackUserId]Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In server/application-server/openapi.yaml around lines 2606 to 2621,
SlackConnection.linkUrl and SlackConnection.slackUserId are declared as plain
strings but must allow null; update their schema entries to permit null (e.g.,
add nullable: true or use oneOf: [ { type: string }, { type: "null" } ]) and
adjust the descriptions if needed to state they can be null so generated
TypeScript types become string | null.
| @GetMapping("/slack-connection") | ||
| @Operation( | ||
| summary = "Get Slack connection status", | ||
| description = "Check if the current user has linked their Slack account. " + | ||
| "If Slack is enabled, also returns the URL where users can link their Slack account in Keycloak." | ||
| ) | ||
| public ResponseEntity<SlackConnectionDTO> getSlackConnection(@AuthenticationPrincipal JwtAuthenticationToken auth) { | ||
| var user = userRepository.getCurrentUser(); | ||
| if (user.isEmpty()) { | ||
| return ResponseEntity.notFound().build(); | ||
| } | ||
|
|
||
| JwtAuthenticationToken token = resolveAuthentication(auth); | ||
| String keycloakUserId = null; | ||
| if (token != null) { | ||
| keycloakUserId = token.getToken().getClaimAsString(StandardClaimNames.SUB); | ||
| } | ||
|
|
||
| SlackConnectionDTO status; | ||
| if (keycloakUserId != null && slackConnectionService.isSlackEnabled()) { | ||
| status = slackConnectionService.syncSlackIdentity(user.get(), keycloakUserId, getKeycloakBaseUrl()); | ||
| } else { | ||
| status = slackConnectionService.getConnectionStatus(user.get(), getKeycloakBaseUrl()); | ||
| } | ||
|
|
||
| return ResponseEntity.ok(status); | ||
| } |
There was a problem hiding this comment.
Avoid turning “unauthenticated” into 404, and consider not auto-syncing on every GET.
Right now, missing auth can fall through to notFound() (via getCurrentUser()), and GET may call Keycloak every time when Slack is enabled.
@@
public ResponseEntity<SlackConnectionDTO> getSlackConnection(@AuthenticationPrincipal JwtAuthenticationToken auth) {
+ JwtAuthenticationToken token = resolveAuthentication(auth);
+ if (token == null) {
+ return ResponseEntity.status(HttpStatus.UNAUTHORIZED).build();
+ }
var user = userRepository.getCurrentUser();
if (user.isEmpty()) {
return ResponseEntity.notFound().build();
}
- JwtAuthenticationToken token = resolveAuthentication(auth);
- String keycloakUserId = null;
- if (token != null) {
- keycloakUserId = token.getToken().getClaimAsString(StandardClaimNames.SUB);
- }
+ String keycloakUserId = token.getToken().getClaimAsString(StandardClaimNames.SUB);
@@
- if (keycloakUserId != null && slackConnectionService.isSlackEnabled()) {
- status = slackConnectionService.syncSlackIdentity(user.get(), keycloakUserId, getKeycloakBaseUrl());
- } else {
- status = slackConnectionService.getConnectionStatus(user.get(), getKeycloakBaseUrl());
- }
+ // Consider: keep GET as a cheap read, and require explicit POST /sync for Keycloak calls.
+ status = slackConnectionService.getConnectionStatus(user.get(), getKeycloakBaseUrl());🤖 Prompt for AI Agents
server/application-server/src/main/java/de/tum/in/www1/hephaestus/account/AccountController.java
lines 145-171: the current GET handler can return 404 for unauthenticated users
(via getCurrentUser()) and performs an automatic sync with Keycloak on every GET
when Slack is enabled; change the flow to first detect missing authentication
and return 401 Unauthorized instead of 404, resolve the JwtAuthenticationToken
up-front and only use keycloakUserId if token is present; remove or disable the
automatic sync on a plain GET (call
slackConnectionService.getConnectionStatus(...) for idempotent reads) and move
sync logic to a separate POST/PUT endpoint or make it opt-in via a query
parameter so Keycloak is not contacted on every GET.
| package de.tum.in.www1.hephaestus.leaderboard.tasks; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.*; | ||
|
|
||
| import com.slack.api.model.User; | ||
| import de.tum.in.www1.hephaestus.gitprovider.user.UserInfoDTO; | ||
| import de.tum.in.www1.hephaestus.leaderboard.LeaderboardEntryDTO; | ||
| import de.tum.in.www1.hephaestus.testconfig.BaseUnitTest; | ||
| import java.util.List; | ||
| import java.util.function.Function; | ||
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.DisplayName; | ||
| import org.junit.jupiter.api.Nested; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.springframework.test.util.ReflectionTestUtils; | ||
|
|
||
| /** | ||
| * Unit tests for Slack user matching logic in SlackWeeklyLeaderboardTask. | ||
| * | ||
| * <p> | ||
| * The matching follows industry best practices for cross-platform identity | ||
| * resolution: | ||
| * <ol> | ||
| * <li><b>Linked Slack User ID</b>: Highest priority - users explicitly link | ||
| * their Slack account</li> | ||
| * <li><b>Email matching</b>: Fallback for users who haven't linked their | ||
| * account</li> | ||
| * </ol> | ||
| * | ||
| * <p> | ||
| * Fuzzy name matching is intentionally NOT supported - names are not reliable | ||
| * identifiers. | ||
| */ | ||
| class SlackWeeklyLeaderboardTaskMatchingTest extends BaseUnitTest { | ||
|
|
||
| private SlackWeeklyLeaderboardTask task; | ||
|
|
||
| @BeforeEach | ||
| void setUp() { | ||
| task = new SlackWeeklyLeaderboardTask(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if BaseUnitTest has @Tag annotation
rg -n "@Tag|class BaseUnitTest" server/application-server/src/test/java -A 5 -B 5 | grep -A 10 -B 10 "BaseUnitTest"Repository: ls1intum/Hephaestus
Length of output: 3773
🏁 Script executed:
# Check the complete SlackWeeklyLeaderboardTaskMatchingTest file for @Tag annotation
cat server/application-server/src/test/java/de/tum/in/www1/hephaestus/leaderboard/tasks/SlackWeeklyLeaderboardTaskMatchingTest.javaRepository: ls1intum/Hephaestus
Length of output: 10143
🏁 Script executed:
# Also find where BaseUnitTest is located
fd -name "BaseUnitTest.java" server/application-server/src/test/javaRepository: ls1intum/Hephaestus
Length of output: 293
Add explicit @Tag("unit") annotation to the test class.
JUnit 5 does not inherit tag annotations from parent classes. Although BaseUnitTest has @Tag("unit"), the test class must declare it explicitly for CI to properly categorize and pick up the test. Add @Tag("unit") above the class declaration.
🤖 Prompt for AI Agents
In
server/application-server/src/test/java/de/tum/in/www1/hephaestus/leaderboard/tasks/SlackWeeklyLeaderboardTaskMatchingTest.java
around lines 1 to 41, the test class relies on BaseUnitTest's @Tag("unit") but
JUnit 5 does not inherit tags; add an explicit @Tag("unit") annotation directly
above the class declaration so CI recognizes and categorizes these tests as unit
tests.
| @Test | ||
| @DisplayName("should generate valid Slack OAuth URL with signed state") | ||
| void shouldGenerateValidUrl() { | ||
| String slug = "test-workspace"; | ||
| String redirectUri = "https://example.com/callback"; | ||
|
|
||
| String url = service.generateInstallUrl(slug, redirectUri); | ||
|
|
||
| // Verify URL structure | ||
| assertThat(url).startsWith("https://slack.com/oauth/v2/authorize?"); | ||
| assertThat(url).contains("client_id=" + CLIENT_ID); | ||
| assertThat(url).contains("scope=chat%3Awrite%2Cchannels%3Aread%2Cusers%3Aread%2Cteam%3Aread"); | ||
|
|
||
| // Verify redirect_uri is URL encoded | ||
| assertThat(url).contains("redirect_uri=https%3A%2F%2Fexample.com%2Fcallback"); | ||
|
|
||
| // Verify state is present and URL encoded | ||
| assertThat(url).contains("state="); | ||
|
|
||
| // Extract and verify state contains the workspace slug (double base64 encoded) | ||
| String state = extractAndDecodeStateFromUrl(url); | ||
| assertThat(state).isNotEmpty(); | ||
|
|
||
| String decoded = new String(Base64.getUrlDecoder().decode(state), StandardCharsets.UTF_8); | ||
| String[] parts = decoded.split("\\|"); | ||
| assertThat(parts).hasSize(3); // encodedSlug|timestamp|signature | ||
|
|
||
| // Decode the inner slug | ||
| String decodedSlug = new String(Base64.getUrlDecoder().decode(parts[0]), StandardCharsets.UTF_8); | ||
| assertThat(decodedSlug).isEqualTo(slug); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("should handle workspace slugs with special characters") | ||
| void shouldHandleSpecialCharacters() { | ||
| String slug = "my|workspace|with|pipes"; | ||
| String redirectUri = "https://example.com/callback"; | ||
|
|
||
| String url = service.generateInstallUrl(slug, redirectUri); | ||
|
|
||
| // Extract and verify the slug survives the round-trip | ||
| String state = extractAndDecodeStateFromUrl(url); | ||
| String decoded = new String(Base64.getUrlDecoder().decode(state), StandardCharsets.UTF_8); | ||
| String[] parts = decoded.split("\\|"); | ||
|
|
||
| // The first part is the base64-encoded slug | ||
| String decodedSlug = new String(Base64.getUrlDecoder().decode(parts[0]), StandardCharsets.UTF_8); | ||
| assertThat(decodedSlug).isEqualTo(slug); | ||
| } | ||
|
|
There was a problem hiding this comment.
Tests mix multiple responsibilities and are brittle to OAuth scope changes.
shouldGenerateValidUrl asserts URL prefix, client_id, scopes, redirect encoding, and state format/slug round-trip all at once; this makes failures noisy and tightly couples tests to scope ordering/encoding. Consider splitting into focused tests (URL contains required params vs state round-trip correctness), per test guidelines (single objective / minimal assertions). As per coding guidelines, ...
🤖 Prompt for AI Agents
In
server/application-server/src/test/java/de/tum/in/www1/hephaestus/workspace/WorkspaceSlackIntegrationServiceTest.java
around lines 56-105, the test shouldGenerateValidUrl mixes many concerns (URL
structure, exact scope string, redirect encoding, and state round-trip) making
it brittle; split it into focused tests: one test that only asserts the presence
of required OAuth parameters (startsWith slack URL, contains client_id,
redirect_uri correctly encoded, and that each required scope is present without
relying on exact ordering or full encoded scope string) and a second test that
only verifies the state round-trip (state exists, decode base64 twice, validate
parts count and that the inner slug decodes to the original, including a
separate test for special characters); remove assertions that tie to scope
ordering/encoding and move scope checks to individual contains assertions (or
URL-decoded contains) so failures are narrow and stable.
| @Test | ||
| @DisplayName("should reject expired state token (security: 10-minute expiry)") | ||
| void shouldRejectExpiredState() { | ||
| // Create a state with a timestamp from 11 minutes ago (beyond the 10-minute | ||
| // expiry) | ||
| String slug = "test-workspace"; | ||
| String encodedSlug = Base64.getUrlEncoder().withoutPadding() | ||
| .encodeToString(slug.getBytes(StandardCharsets.UTF_8)); | ||
| long expiredTimestamp = java.time.Instant.now().getEpochSecond() - 660; // 11 minutes ago | ||
|
|
||
| // We need to create a validly-signed but expired state | ||
| // This is tricky because we don't have access to the sign() method | ||
| // Instead, we test that the service correctly identifies timestamps in the past | ||
|
|
||
| // For this test, we'll create a properly formatted state with an old timestamp | ||
| // and wrong signature (since we can't compute the real signature without the | ||
| // secret) | ||
| String payload = encodedSlug + "|" + expiredTimestamp + "|fake-signature"; | ||
| String expiredState = Base64.getUrlEncoder() | ||
| .encodeToString(payload.getBytes(StandardCharsets.UTF_8)); | ||
|
|
||
| // This will fail on signature first, which is fine - signature check happens | ||
| // before expiry | ||
| assertThatThrownBy(() -> service.completeInstallation("code", expiredState, "uri")) | ||
| .isInstanceOf(IllegalArgumentException.class); | ||
| // Note: In practice, signature validation happens before expiry check, | ||
| // so this tests the overall validation pipeline | ||
| } |
There was a problem hiding this comment.
“Expired state” test does not validate expiry behavior (fails earlier on signature).
shouldRejectExpiredState constructs an expired timestamp but intentionally uses a fake signature and then accepts any IllegalArgumentException, so the expiry check could be broken without this test failing. To actually test the 10‑minute expiry branch, generate a validly signed payload with an old timestamp (e.g., by extracting the signing logic into a package-private collaborator or injecting a clock/state-signer that the test can control). As per coding guidelines, ...
🤖 Prompt for AI Agents
In
server/application-server/src/test/java/de/tum/in/www1/hephaestus/workspace/WorkspaceSlackIntegrationServiceTest.java
around lines 252 to 279, the test constructs an expired state but uses a fake
signature and asserts any IllegalArgumentException, so it cannot verify the
specific 10-minute expiry branch; change the test to produce a legitimately
signed state with an old timestamp and assert the expiry-specific failure:
refactor or expose the real signing behavior (make the signer package-private or
inject a StateSigner or Clock into the service), use that signer (or a test
signer) to sign the slug|oldTimestamp payload, or inject a controllable Clock to
create an expired timestamp deterministically, then call completeInstallation
and assert the exact exception/message that indicates expiry (not a generic
IllegalArgumentException).
| /** Status from Slack OAuth callback: 'success' | 'error' | 'cancelled' | 'invalid' | undefined */ | ||
| slackStatus?: string | null; | ||
| onAddRepository: (nameWithOwner: string) => void; |
There was a problem hiding this comment.
Tighten slackStatus to the validated union to make the switch exhaustive.
Instead of string | null, use "success" | "error" | "cancelled" | "invalid" | undefined (ideally inferred from the zod schema) and add a default branch to avoid silently ignoring unexpected values.
Also applies to: 54-79
🤖 Prompt for AI Agents
In webapp/src/components/admin/AdminSettingsPage.tsx around lines 24-26 (and
similarly update usages around lines 54-79), slackStatus is currently typed as
string | null; tighten it to the validated union "success" | "error" |
"cancelled" | "invalid" | undefined (preferably by importing/inferencing the
union from the zod schema or a shared type) and update any switch/conditional
handling to include a default branch that catches unexpected values (e.g., treat
as undefined or log/handle as unknown). Ensure all props, state, and handlers
use the new union type so the switch is exhaustive and update related type
annotations in the 54-79 range accordingly.
| const handleConnect = () => { | ||
| window.location.href = `/api/workspaces/${workspaceSlug}/slack/install`; | ||
| }; |
There was a problem hiding this comment.
Encode workspaceSlug when building the install URL.
Today: /api/workspaces/${workspaceSlug}/slack/install. Safer: /api/workspaces/${encodeURIComponent(workspaceSlug)}/slack/install to avoid path issues if slugs ever contain reserved characters.
🤖 Prompt for AI Agents
In webapp/src/components/admin/AdminSlackSettings.tsx around lines 20 to 22, the
install URL is built using the raw workspaceSlug which can break if the slug
contains reserved URL/path characters; update the handler to encode the slug
with encodeURIComponent when constructing the path (i.e., use
encodeURIComponent(workspaceSlug) in the URL) so the redirect URL is safe and
correctly formed.
Description
How to test
Screenshots
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.