23 create security config file Add API key management, role-based security, and Google OIDC integration#31
23 create security config file Add API key management, role-based security, and Google OIDC integration#31efpcode wants to merge 54 commits into
Conversation
Co-authored-by: Sedya seyda.kinaci@iths.se>
Co-authored-by: seyda.kinaci@iths.se
Co-authored-by: seyda.kinaci@iths.se
Co-authored-by: seyda.kinaci@iths.se
# Conflicts: # src/main/resources/application-dev.properties
# Conflicts: # src/main/resources/application-dev.properties # src/test/java/org/storkforge/barkr/web/controller/WebControllerTest.java
# Conflicts: # src/test/java/org/storkforge/barkr/api/controller/ApiAccountControllerTest.java # src/test/java/org/storkforge/barkr/api/controller/ApiPostControllerTest.java
This reverts commit 902f001. Correct signed version of previous commit
WalkthroughThis update introduces a comprehensive API key management and role-based security system to the application. It adds new domain entities, repositories, services, and controllers for generating, listing, updating, and revoking API keys, with secure storage and validation. The security configuration is overhauled to integrate OAuth2, role-based access control, and custom API key authentication for REST and GraphQL endpoints. The database schema is expanded with new tables and constraints to support API keys, Google OIDC integration, and user roles. The UI is enhanced with new templates for API key management and premium feature unlocking, and supporting JavaScript and CSS files are added. Extensive localization updates and new/updated tests ensure robust coverage of the new features. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebApp
participant ApiKeyController
participant IssuedApiKeyService
participant AccountRepository
participant GoogleAccountApiKeyLinkRepository
participant IssuedApiKeyRepository
User->>WebApp: GET /apikeys/apikeyform
WebApp->>ApiKeyController: showApiKeyForm()
ApiKeyController-->>WebApp: Render API key form
User->>WebApp: POST /apikeys/generate (name, expiresAt)
WebApp->>ApiKeyController: generateApiKey()
ApiKeyController->>IssuedApiKeyService: generateRawApiKey(), hashedApiKey()
ApiKeyController->>AccountRepository: findByGoogleOidc2Id()
ApiKeyController->>GoogleAccountApiKeyLinkRepository: findByAccount()
ApiKeyController->>IssuedApiKeyService: apiKeyGenerate()
IssuedApiKeyService->>IssuedApiKeyRepository: Save new API key
ApiKeyController-->>WebApp: Redirect with raw key (flash attribute)
User->>WebApp: GET /apikeys/result
WebApp->>ApiKeyController: showGeneratedApiKey()
ApiKeyController-->>WebApp: Render result page with API key
User->>WebApp: GET /apikeys/mykeys
WebApp->>ApiKeyController: myKeys()
ApiKeyController->>IssuedApiKeyService: allApiKeys()
IssuedApiKeyService->>IssuedApiKeyRepository: findByGoogleAccountApiKeyLinkOrderByIssuedAtDesc()
ApiKeyController-->>WebApp: Render mykeys.html
User->>WebApp: POST /apikeys/mykeys/revoke
WebApp->>ApiKeyController: revokeKey(referenceId)
ApiKeyController->>IssuedApiKeyService: updateApiKey(revoke=true)
IssuedApiKeyService->>IssuedApiKeyRepository: Update API key as revoked
ApiKeyController-->>WebApp: Redirect to mykeys
User->>WebApp: POST /apikeys/mykeys/nameupdate
WebApp->>ApiKeyController: nameUpdate(apiKeyName, referenceId)
ApiKeyController->>IssuedApiKeyService: updateApiKey(apiKeyName)
IssuedApiKeyService->>IssuedApiKeyRepository: Update API key name
ApiKeyController-->>WebApp: Redirect to mykeys
sequenceDiagram
participant User
participant WebApp
participant SecurityConfig
participant ApiKeyAuthenticationFilter
participant IssuedApiKeyService
participant ApiController
User->>WebApp: REST/GraphQL request with API-KEY header
WebApp->>SecurityConfig: Security filter chain
SecurityConfig->>ApiKeyAuthenticationFilter: doFilterInternal()
ApiKeyAuthenticationFilter->>IssuedApiKeyService: apiKeyValidation()
alt Valid API key
ApiKeyAuthenticationFilter->>IssuedApiKeyService: updateApiKey(lastUsedAt)
ApiKeyAuthenticationFilter-->>WebApp: Set authentication context
WebApp->>ApiController: Handle request as authenticated
else Invalid API key
ApiKeyAuthenticationFilter-->>WebApp: Deny access / no authentication
end
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
coderabbitai
Bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🔭 Outside diff range comments (1)
src/test/java/org/storkforge/barkr/web/controller/WebControllerTest.java (1)
1-250: 💡 Verification agent🧩 Analysis chain
Fix pipeline failure related to missing 'SECRET_KEY'.
The pipeline is failing with an error about a missing 'SECRET_KEY' placeholder, which is preventing the ApplicationContext from initializing properly. This suggests that the tests require this configuration property to be set.
🏁 Script executed:
#!/bin/bash # Description: Identify where 'SECRET_KEY' is used and determine how to fix the pipeline failure # First, search for SECRET_KEY references in the configuration files echo "Searching for SECRET_KEY references in configuration files..." rg -A 5 'SECRET_KEY' --glob '*.properties' --glob '*.yml' --glob '*.yaml' # Search for environment variable references in the security configuration echo "Searching for references in security configuration..." rg -A 5 '(SECRET_KEY|@Value)' --glob '*SecurityConfig*.java' --glob '*Security*.java' # Check if there's a placeholder mechanism in test configuration echo "Checking test configuration for placeholder mechanism..." rg 'placeholder' --glob '*Test*.java' --glob '*test*.properties' --glob '*test*.yml'Length of output: 698
Add a default fallback for
SECRET_KEYto prevent test failuresThe
application-dev.propertiesfile references${SECRET_KEY}but tests load thedevprofile without setting this environment variable, causing the context to fail on startup. You can fix this by providing a default value:• File:
src/main/resources/application-dev.properties- barkr.api.secret = ${SECRET_KEY} + barkr.api.secret = ${SECRET_KEY:dev-secret}This ensures the application (and its tests) will default to
dev-secretifSECRET_KEYisn’t set.🧰 Tools
🪛 GitHub Actions: Build & Test
[error] 1-1: Multiple test errors due to failure to load ApplicationContext caused by missing 'SECRET_KEY' placeholder preventing proper bean initialization.
🧹 Nitpick comments (49)
src/main/resources/db/migration/V5__Create_issued_api_key_table.sql .sql (2)
14-16: Unique constraint onhashed_api_keyis correct but consider naming consistency.The unique constraint ensures API key hashes remain distinct, which is essential. For consistency with snake_case conventions seen elsewhere, you might rename:
ALTER TABLE issued_api_key - ADD CONSTRAINT uc_issued_api_key_hashedapikey UNIQUE (hashed_api_key); + ADD CONSTRAINT uc_issued_api_key_hashed_api_key UNIQUE (hashed_api_key);This change is optional but improves readability and maintainability.
17-18: AddON DELETE CASCADEand an index for the foreign key.Linking to
google_account_api_key_linkis correct, but consider:
- ON DELETE CASCADE: Automatically removes API keys if the parent link is deleted.
- Index: Speeds up lookups/join operations on
google_account_api_key_link_id.ALTER TABLE issued_api_key - ADD CONSTRAINT FK_ISSUED_API_KEY_ON_GOOGLE_ACCOUNT_API_KEY_LINK FOREIGN KEY (google_account_api_key_link_id) REFERENCES google_account_api_key_link (id); + ADD CONSTRAINT fk_issued_api_key_google_link FOREIGN KEY (google_account_api_key_link_id) + REFERENCES google_account_api_key_link (id) + ON DELETE CASCADE; +CREATE INDEX idx_issued_api_key_google_link_id + ON issued_api_key(google_account_api_key_link_id);These adjustments enhance data integrity and query performance.
src/main/java/org/storkforge/barkr/dto/apiKeyDto/GenerateApiKeyRequest.java (1)
9-10: Enhance validation forexpiresAt.
Consider enforcing non-null and future timestamps by annotatingexpiresAtwith:@NotNull @FutureOrPresent LocalDateTime expiresAtAlso, remove the extra space before the comma in
apiKeyName.src/main/java/org/storkforge/barkr/filters/ApiKeyAuthenticationToken.java (1)
12-16: Add null check forapiKey.
Guard against null values in the constructor to prevent NPE during authentication:- this.apiKey = apiKey; + this.apiKey = Objects.requireNonNull(apiKey, "apiKey must not be null");src/main/java/org/storkforge/barkr/dto/apiKeyDto/ResponseApiKey.java (1)
11-19: Use temporal types for timestamps instead of strings.
DefiningissuedAt,expiresAt, andlastUsedAtasStringsacrifices type safety and date validation. Consider refactoring these toLocalDateTimewith Jackson annotations:@NotNull @JsonFormat(pattern = "yyyy-MM-dd'T'HH:mm:ss") LocalDateTime issuedAt, ...This leverages built-in serialization and validation for date-time fields.
src/main/java/org/storkforge/barkr/dto/apiKeyDto/CreateApiKey.java (1)
3-8: Consider decoupling domain entity from DTO
Passing aGoogleAccountApiKeyLinkentity directly into a DTO can tightly couple your API layer to the persistence model, making it harder to evolve independently. Instead, consider using an identifier (e.g., a UUID) or a dedicated lightweight DTO for the link to maintain separation of concerns.src/main/resources/templates/index.html (1)
33-38: Remove redundant authorization check on the button elementThe authorization check is duplicated on both the containing card div and the button itself, which is unnecessary and could lead to maintenance issues.
<div class="card" th:if="${#authorization.expression('hasAuthority(''ROLE_PREMIUM'')')}"> - <button - th:if="${#authorization.expression('hasAuthority(''ROLE_PREMIUM'')')}" - id="generate-joke-btn" class="btn">Generate Joke - </button> + <button id="generate-joke-btn" class="btn">Generate Joke</button> <div id="joke-display"></div> </div>Since the parent div already checks for the PREMIUM role, there's no need to repeat the check on the button.
src/main/resources/application-dev.properties (2)
6-7: Security debug logging should be restricted to developmentThe debug logging for Spring Security and OAuth2 is useful for development but should be disabled in production environments to avoid potential information leakage.
Consider creating a production-specific properties file that explicitly disables these debug logs, or ensure this dev-specific configuration isn't loaded in production.
23-23: Ensure proper documentation for required environment variablesThe application now requires several environment variables (
GOOGLE_CLIENT_ID,GOOGLE_CLIENT_SECRET,SECRET_KEY) for security functionality.Consider adding documentation (such as in README.md) listing all required environment variables with descriptions of their purpose and how to obtain them.
src/main/java/org/storkforge/barkr/infrastructure/persistence/AccountRepository.java (1)
15-16: Remove excessive blank linesThe repository interface contains several unnecessary blank lines. While whitespace can improve readability, excessive blank lines can make the code look sparse and harder to scan.
Optional<byte[]> getAccountImage(Long id); - - Optional<Account> findByGoogleOidc2Id(@NotBlank String googleOidc2Id); - -Also applies to: 18-19
src/main/java/org/storkforge/barkr/ai/controller/MistralAiController.java (1)
8-8: Ensure imported classes are used.The
BarkrRoleclass is imported but doesn't appear to be directly used in this file.-import org.storkforge.barkr.domain.roles.BarkrRole;Also applies to: 11-11
src/main/resources/static/js/copy-to-clipboard.js (2)
1-13: Add null checks to prevent JavaScript errors.The function doesn't verify if the input elements exist before accessing their values, which could lead to errors if the function is called on a page where these elements aren't present.
function copyKey() { - const keyInput = document.getElementById("rawKey"); - const valueInput = document.getElementById("apiKey"); + const keyInput = document.getElementById("rawKey"); + const valueInput = document.getElementById("apiKey"); + + if (!keyInput || !valueInput) { + alert("API key elements not found on page"); + return; + } const key = keyInput.value; const value = valueInput.value; const credentials = `key: ${value}\nvalue: ${key}`; navigator.clipboard.writeText(credentials) .then(() => alert("API key copied!")) .catch(err => alert("Failed to copy.")); }
2-6: Consider more intuitive variable naming.The variable naming is a bit confusing:
keyInputretrieves the element with ID "rawKey" andvalueInputretrieves "apiKey", but thenkeyis assigned fromkeyInputandvaluefromvalueInput. This could lead to confusion when maintaining the code.- const keyInput = document.getElementById("rawKey"); - const valueInput = document.getElementById("apiKey"); - - const key = keyInput.value; - const value = valueInput.value; + const rawKeyInput = document.getElementById("rawKey"); + const apiKeyInput = document.getElementById("apiKey"); + + const rawKey = rawKeyInput.value; + const apiKey = apiKeyInput.value;Then update the credentials string accordingly:
- const credentials = `key: ${value}\nvalue: ${key}`; + const credentials = `key: ${apiKey}\nvalue: ${rawKey}`;src/main/java/org/storkforge/barkr/dto/apiKeyDto/ResponseApiKeyList.java (1)
5-5: Consider defensive copying of the input list.Although records are immutable, the provided
List<ResponseApiKey>can still be modified externally. To guarantee deep immutability, wrap or copy the list in a canonical constructor:public ResponseApiKeyList(List<ResponseApiKey> apiKeys) { this.apiKeys = List.copyOf(apiKeys); }src/main/resources/messages_sv.properties (1)
16-16: Remove unintended leading space.The value for
apikey.gencontains a leading space (" Skapa API Nyckel"), which will render an extra blank before the text in the UI. Please trim it to"Skapa API Nyckel".src/main/resources/db/migration/V3__Add_unique_constraints_on_username_and_google_oidc2id.sql (1)
11-13: Avoid redundant index on unique constraint column.PostgreSQL automatically creates a unique index when you add a
UNIQUEconstraint. The explicitCREATE UNIQUE INDEXongoogle_oidc2idis redundant and could be removed. Additionally, consider standardizing naming conventions between the constraint (uc_account_googleoidc2id) and the index (idx_account_google_oidc2_id).Also applies to: 15-16
src/main/resources/templates/fragments/barkr-logout.html (1)
13-18: Handle popup blockers and provide fallback.Opening Google's logout endpoint in a new tab may be blocked by some browsers. Consider showing a manual link or an instruction if the pop-up fails:
<noscript> <p><a href="https://accounts.google.com/Logout" target="_blank">Logga ut från Google</a></p> </noscript>src/main/resources/static/js/update-apikey-name.js (2)
5-5: Consider replacingwindow.promptfor better UX
The nativepromptdialog is blocking and not stylable. You may want to introduce a custom modal dialog component for consistency with the rest of your UI and to provide better accessibility.
6-9: Provide user feedback on invalid input
If the user cancels or enters only whitespace, the function silently does nothing. Consider adding anelsebranch or inline error message to inform the user that a valid name is required.src/main/resources/messages_en.properties (1)
9-17: Localization entries look correct
The new keys (mykeys,apikey.revoke,apikey.nameupdate,code.label,code.button) align with the added API-key UI flows. Ensure these are also added to your other locale files (e.g.,messages_sv.properties,messages_uwu.properties) so translations remain in sync across languages.src/main/java/org/storkforge/barkr/domain/roles/BarkrRole.java (2)
3-7: Consider implementingGrantedAuthoritydirectly
You could haveBarkrRoleimplement Spring Security’sGrantedAuthorityinterface and overridegetAuthority()(returningauthorityValue). This would eliminate the need for extra adapter code when mapping enum values toSimpleGrantedAuthority.
12-18: Add Javadoc for clarity
A brief Javadoc on the constructor andgetAuthorityValue()method would improve readability by explaining the purpose ofauthorityValueand how it integrates with your security model.src/main/resources/db/migration/V7__adds_referenceID_uuid_to_apikey_table.sql (1)
4-6: Consider adding a default UUID generator
Future inserts will require manually settingreference_id. You could simplify this by adding the column withDEFAULT gen_random_uuid()andNOT NULLin one statement:ALTER TABLE issued_api_key ADD COLUMN reference_id UUID NOT NULL DEFAULT gen_random_uuid();src/main/resources/templates/apikeys.html (2)
16-18: Add server-side validation feedback
Currently, invalid submissions won’t surface errors. Consider using Thymeleaf’sth:errorsor conditional blocks to display validation messages forapiKeyNameandexpiresAt.
14-15: Externalize labels for localization
Hardcoded labels like “API Key Name:” and “Expires At (optional):” could be moved to message bundles (th:text="#{label.apikey.name}", etc.) for i18n consistency.src/main/java/org/storkforge/barkr/exceptions/IssuedApiKeyNotFound.java (1)
1-14: Enhance exception constructors and serialization
Consider adding aserialVersionUIDand constructors that accept aThrowablecause for better error chaining:private static final long serialVersionUID = 1L; public IssuedApiKeyNotFound(String message, Throwable cause) { super(message, cause); } public IssuedApiKeyNotFound(Throwable cause) { super(cause); }src/main/resources/templates/apikeys/result.html (2)
15-20: Remove redundant static placeholder values
Thevalue="API-KEY"andvalue="RAW-APIKEY"attributes are overridden byth:value. Removing these placeholders avoids confusion:- <input ... readonly value="API-KEY" th:value="${response.key}" /> + <input ... readonly th:value="${response.key}" />Likewise for the raw key input.
11-14: Externalize UI text for localization
Headings and button text (e.g., “Your API Key”, “Copy to Clipboard”, “Done”) are hardcoded. Consider replacing with message bundle keys (th:text="#{apikey.result.title}", etc.) for consistency with the rest of the app.Also applies to: 23-28
src/main/resources/db/migration/V6__add_roles_column_to_account_table.sql (1)
1-8: Well-structured role table migration with proper constraintsThis migration script correctly creates the account_roles table with appropriate constraints:
- Foreign key reference to the account table with cascade delete
- Index on account_id for performance
- Default role value of 'ROLE_USER'
One suggestion:
Consider adding a composite primary key or unique constraint on (account_id, role) to prevent duplicate role assignments. You could add:
CREATE TABLE IF NOT EXISTS account_roles ( account_id BIGINT NOT NULL, role VARCHAR(255) NOT NULL DEFAULT 'ROLE_USER', + PRIMARY KEY (account_id, role), FOREIGN KEY (account_id) REFERENCES account(account_id) ON DELETE CASCADE );src/main/resources/static/js/unlock-easter-egg.js (1)
1-32: Code works but has room for improvement.While the implementation works and correctly includes CSRF protection, there are several opportunities for improvement:
- Consider using the FormData API instead of manually building the form data string
- Add loading state indicators and disable the submit button during the AJAX request
- Improve error handling beyond just logging to console
Here's an improved implementation:
document.addEventListener('DOMContentLoaded', function () { var form = document.getElementById('unlock-premium-form'); if (!form) return; form.addEventListener('submit', function(event) { event.preventDefault(); + var submitButton = form.querySelector('button[type="submit"]'); + var responseElement = document.getElementById('premium-response'); var code = document.getElementById('code').value; var csrfToken = document.getElementById('csrfToken').value; + // Disable submit button and show loading state + submitButton.disabled = true; + responseElement.textContent = 'Processing...'; var xhr = new XMLHttpRequest(); xhr.open('POST', '/unlock-easter-egg', true); xhr.setRequestHeader('Content-Type', 'application/x-www-form-urlencoded'); xhr.setRequestHeader('X-CSRF-TOKEN', csrfToken); - var body = 'code=' + encodeURIComponent(code); // <-- manually build form data string + // Use FormData API for cleaner code + var formData = new FormData(form); + var body = new URLSearchParams(formData).toString(); xhr.onreadystatechange = function() { if (xhr.readyState === 4) { + // Re-enable submit button + submitButton.disabled = false; var responseText = xhr.responseText; if (xhr.status === 200 || xhr.status === 202) { - document.getElementById('premium-response').textContent = responseText; + responseElement.textContent = responseText; } else { console.error("XHR error:", responseText); - document.getElementById('premium-response').textContent = '❌ ' + responseText; + responseElement.textContent = '❌ ' + responseText; } } }; xhr.send(body); }); });src/main/resources/templates/apikeys/mykeys.html (2)
8-52: Consider accessibility enhancements for the tableThe table styling is well-designed with responsive elements, but consider adding ARIA attributes to enhance accessibility. For example, adding
role="table"to the table element androle="row"totrelements would improve screen reader compatibility.
79-85: Consider formatting date-time values for better readabilityThe date-time values are displayed in their raw format (e.g.,
2025-04-20T15:44). Consider using Thymeleaf's formatting utilities to display these in a more user-friendly format.- <td th:text="${key.issuedAt}">2025-04-20T15:44</td> - <td th:text="${key.expiresAt}">2025-04-20T15:49</td> - <td th:text="${key.lastUsedAt}">2025-04-20T15:44</td> + <td th:text="${#temporals.format(key.issuedAt, 'yyyy-MM-dd HH:mm')}">2025-04-20 15:44</td> + <td th:text="${#temporals.format(key.expiresAt, 'yyyy-MM-dd HH:mm')}">2025-04-20 15:49</td> + <td th:text="${#temporals.format(key.lastUsedAt, 'yyyy-MM-dd HH:mm')}">2025-04-20 15:44</td>src/main/java/org/storkforge/barkr/filters/ApiKeyAuthenticationFilter.java (2)
50-55: Improve exception handling with proper loggingThe current exception handling simply wraps and rethrows exceptions as RuntimeException without logging. Consider adding proper logging to help with troubleshooting.
- } catch (NoSuchAlgorithmException e) { - throw new RuntimeException(e); - - } catch (InvalidKeyException e) { - throw new RuntimeException(e); + } catch (NoSuchAlgorithmException e) { + logger.error("Cryptographic algorithm not available", e); + throw new RuntimeException("Error validating API key", e); + + } catch (InvalidKeyException e) { + logger.error("Invalid key exception during API key validation", e); + throw new RuntimeException("Error validating API key", e);
62-68: Consider optimizing expired API key revocationThe
revokeExpiredApiKeys()method is called on every request that passes through this filter, which could be inefficient for high-traffic applications. Consider moving this to a scheduled task that runs periodically instead.src/main/java/org/storkforge/barkr/infrastructure/persistence/IssuedApiKeyRepository.java (1)
19-24: Add Javadoc comments to repository methodsConsider adding Javadoc comments to describe the purpose and expected behavior of each finder method, especially for methods with non-trivial query semantics.
src/main/java/org/storkforge/barkr/domain/entity/GoogleAccountApiKeyLink.java (2)
25-26: Consider adding indexing for performance optimizationSince this entity maintains a one-to-many relationship with IssuedApiKey entities, consider adding an index on the foreign key column in the IssuedApiKey table to improve query performance, especially if you expect a large number of API keys per account.
70-74: Enhance toString method to include more entity informationThe current toString method only includes the ID field. Consider including additional non-sensitive fields to make debugging easier. However, be careful not to include the entire object graph which could cause lazy loading issues.
- public String toString() { - return getClass().getSimpleName() + "(" + - "id = " + id + ")"; - } + public String toString() { + return getClass().getSimpleName() + "(" + + "id = " + id + ", " + + "account = " + (account != null ? account.getId() : null) + ", " + + "issuedApiKeys count = " + (issuedApiKeys != null ? issuedApiKeys.size() : 0) + ")"; + }src/test/java/org/storkforge/barkr/web/controller/WebControllerTest.java (2)
58-59: Review@Transientannotation usage in test class.The
@Transientannotation is typically used in entity classes to mark fields that shouldn't be persisted to the database. Using it in a test class where no persistence is happening is unnecessary and potentially confusing. Consider removing this annotation as it doesn't serve a purpose in this context.- @Transient private GoogleAccountApiKeyLink googleAccountApiKeyLink;
213-213: Remove duplicatesetBreed()call.There's a duplicate call to
setBreed("beagle")formockAccount2. The same method is called on line 95.- mockAccount2.setBreed("beagle");src/main/java/org/storkforge/barkr/mapper/ApiKeyMapper.java (1)
67-67: Remove extra blank lines.There are extra blank lines that can be removed to improve code readability.
} - - return new GenerateApiKeyRequest(createApiKey.apiKeyName(), inputDate);src/main/java/org/storkforge/barkr/domain/entity/IssuedApiKey.java (1)
45-47: Add length constraints to apiKeyName.The
apiKeyNamefield has@NotBlankvalidation, but no length constraints. Consider adding@Sizeannotation to restrict the minimum and maximum length for better validation.@Column(nullable = false) @NotBlank +@Size(min = 3, max = 100) private String apiKeyName;src/test/java/org/storkforge/barkr/api/controller/ApiKeyControllerTest.java (3)
16-16: Remove redundant import.The import of
ApiKeyControlleris redundant since it's already in the same package (org.storkforge.barkr.api.controller).-import org.storkforge.barkr.api.controller.ApiKeyController;
49-58: Enhance mock OAuth2 authentication with additional OIDC attributes.The
mockOAuth2Authmethod creates a basic OAuth2 token but lacks common OIDC attributes like subject identifier or email that might be used in the controller. Consider enhancing the mock to include these attributes for more realistic testing.private OAuth2AuthenticationToken mockOAuth2Auth(String name) { OidcUser mockUser = Mockito.mock(OidcUser.class); Mockito.when(mockUser.getName()).thenReturn(name); + // Add common OIDC attributes + Mockito.when(mockUser.getSubject()).thenReturn("sub-" + name); + Mockito.when(mockUser.getEmail()).thenReturn(name + "@example.com"); + Mockito.when(mockUser.getFullName()).thenReturn("Full " + name); return new OAuth2AuthenticationToken( mockUser, List.of(new SimpleGrantedAuthority("ROLE_USER")), "oidc" ); }
122-159: Consider adding tests for error scenarios.The tests cover successful operations but don't test error scenarios like invalid input, unauthorized access, or service exceptions. Consider adding tests for these cases to ensure robust error handling.
@Test @DisplayName("Returns 400 for invalid name update") void updateKeyName_invalidInput() throws Exception { String fakeReferenceId = UUID.randomUUID().toString(); Mockito.doThrow(new IllegalArgumentException("Invalid name")) .when(issuedApiKeyService).updateName(any(), any()); mvc.perform(postWithCsrf("/apikeys/mykeys/nameupdate") .param("referenceId", fakeReferenceId) .param("apiKeyName", "")) .andExpect(status().isBadRequest()); } @Test @DisplayName("Returns 404 for non-existent key") void revokeKey_nonExistent() throws Exception { String fakeReferenceId = UUID.randomUUID().toString(); Mockito.doThrow(new RuntimeException("Key not found")) .when(issuedApiKeyService).revokeApiKey(any()); mvc.perform(postWithCsrf("/apikeys/mykeys/revoke") .param("referenceId", fakeReferenceId)) .andExpect(status().isNotFound()); }src/main/java/org/storkforge/barkr/config/SecurityConfig.java (1)
26-43: Consider grouping static resources & trimming requestMatchers listThe list of
requestMatchers(..).permitAll()has grown quite large and mixes static resources, public pages, and business endpoints.
- Spring offers
PathRequest.toStaticResources().atCommonLocations()to cover/css/**,/js/**,/images/**, etc.- Moving business‑endpoints (e.g.
/post/load,/barkr/logout) into clearly‑named constants or a separatepublicEndpoints()helper will improve readability and make future maintenance less error‑prone.No functional bug, but adopting these helpers will shorten the config and make intent clearer.
src/main/java/org/storkforge/barkr/domain/entity/Account.java (2)
56-60:getAuthorities()returns a mutableSet
roles.stream().map(...).collect(Collectors.toSet())produces a mutable collection.
Returning an unmodifiable set prevents accidental modification:- return roles.stream() - .map(role -> new SimpleGrantedAuthority(role.getAuthorityValue())) - .collect(Collectors.toSet()); + return Collections.unmodifiableSet( + roles.stream() + .map(r -> new SimpleGrantedAuthority(r.getAuthorityValue())) + .collect(Collectors.toSet()));
162-168: Logging the entireimagebyte[] blows up logs & memory
toString()includes theimagearray which can be up to 5 MB.
This can flood logs and reveal base64/binary content unintentionally.
Recommend omitting the field or printing its size only.- "image = " + image + ", " + + "imageSize = " + (image == null ? 0 : image.length) + ", " +[security]
src/main/java/org/storkforge/barkr/domain/IssuedApiKeyService.java (1)
108-116: Minor: cacheMacinstance or document the cost
Mac.getInstance("HmacSHA256")is relatively expensive and is executed for every hash.
If this service becomes hot, consider lazily initialising aThreadLocal<Mac>or using Spring’sMacAlgorithmUtils(if available) to reuse the instance.src/main/java/org/storkforge/barkr/web/controller/WebController.java (1)
58-65: Hard‑coded fallback to account ID = 1 is fragileUsing
1Las a universal “guest” account will break as soon as real record #1 is deleted or repurposed.
Prefer an explicit guest DTO, or render the page with an emptyCreatePostwhen the visitor is anonymous.-long id = 1L; +Long id = null; // null = anonymous ... - id = currentUser.isEmpty() ? 1L : currentUser.get().getId(); + id = currentUser.map(ResponseAccount::getId).orElse(null); ... -model.addAttribute("createPostDto", new CreatePost("", id)); +model.addAttribute("createPostDto", new CreatePost("", id == null ? -1 : id));(Adjust downstream code to cope with
-1/null).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (59)
.gitignore(1 hunks)pom.xml(1 hunks)src/main/java/org/storkforge/barkr/ai/controller/MistralAiController.java(3 hunks)src/main/java/org/storkforge/barkr/api/controller/ApiAccountController.java(1 hunks)src/main/java/org/storkforge/barkr/api/controller/ApiKeyController.java(1 hunks)src/main/java/org/storkforge/barkr/config/Config.java(2 hunks)src/main/java/org/storkforge/barkr/config/SecurityConfig.java(1 hunks)src/main/java/org/storkforge/barkr/domain/AccountService.java(2 hunks)src/main/java/org/storkforge/barkr/domain/CustomOidc2UserService.java(1 hunks)src/main/java/org/storkforge/barkr/domain/IssuedApiKeyService.java(1 hunks)src/main/java/org/storkforge/barkr/domain/entity/Account.java(3 hunks)src/main/java/org/storkforge/barkr/domain/entity/GoogleAccountApiKeyLink.java(1 hunks)src/main/java/org/storkforge/barkr/domain/entity/IssuedApiKey.java(1 hunks)src/main/java/org/storkforge/barkr/domain/roles/BarkrRole.java(1 hunks)src/main/java/org/storkforge/barkr/dto/apiKeyDto/CreateApiKey.java(1 hunks)src/main/java/org/storkforge/barkr/dto/apiKeyDto/GenerateApiKeyRequest.java(1 hunks)src/main/java/org/storkforge/barkr/dto/apiKeyDto/ResponseApiKey.java(1 hunks)src/main/java/org/storkforge/barkr/dto/apiKeyDto/ResponseApiKeyList.java(1 hunks)src/main/java/org/storkforge/barkr/dto/apiKeyDto/ResponseApiKeyOnce.java(1 hunks)src/main/java/org/storkforge/barkr/dto/apiKeyDto/UpdateApiKey.java(1 hunks)src/main/java/org/storkforge/barkr/exceptions/IssuedApiKeyNotFound.java(1 hunks)src/main/java/org/storkforge/barkr/filters/ApiKeyAuthenticationFilter.java(1 hunks)src/main/java/org/storkforge/barkr/filters/ApiKeyAuthenticationToken.java(1 hunks)src/main/java/org/storkforge/barkr/infrastructure/persistence/AccountRepository.java(2 hunks)src/main/java/org/storkforge/barkr/infrastructure/persistence/GoogleAccountApiKeyLinkRepository.java(1 hunks)src/main/java/org/storkforge/barkr/infrastructure/persistence/IssuedApiKeyRepository.java(1 hunks)src/main/java/org/storkforge/barkr/mapper/ApiKeyMapper.java(1 hunks)src/main/java/org/storkforge/barkr/web/controller/WebController.java(5 hunks)src/main/resources/application-dev.properties(1 hunks)src/main/resources/application.properties(1 hunks)src/main/resources/db/migration/V3__Add_unique_constraints_on_username_and_google_oidc2id.sql(1 hunks)src/main/resources/db/migration/V4__Create_google_account_api_key_link_table.sql(1 hunks)src/main/resources/db/migration/V5__Create_issued_api_key_table.sql .sql(1 hunks)src/main/resources/db/migration/V6__add_roles_column_to_account_table.sql(1 hunks)src/main/resources/db/migration/V7__adds_referenceID_uuid_to_apikey_table.sql(1 hunks)src/main/resources/messages.properties(1 hunks)src/main/resources/messages_en.properties(1 hunks)src/main/resources/messages_sv.properties(1 hunks)src/main/resources/messages_uwu.properties(1 hunks)src/main/resources/static/css/styles.css(1 hunks)src/main/resources/static/js/copy-to-clipboard.js(1 hunks)src/main/resources/static/js/generate-joke.js(1 hunks)src/main/resources/static/js/unlock-easter-egg.js(1 hunks)src/main/resources/static/js/update-apikey-name.js(1 hunks)src/main/resources/templates/apikeys.html(1 hunks)src/main/resources/templates/apikeys/mykeys.html(1 hunks)src/main/resources/templates/apikeys/result.html(1 hunks)src/main/resources/templates/fragments/barkr-logout.html(1 hunks)src/main/resources/templates/fragments/header.html(2 hunks)src/main/resources/templates/fragments/sidebar.html(1 hunks)src/main/resources/templates/index.html(2 hunks)src/main/resources/templates/profile.html(2 hunks)src/test/java/org/storkforge/barkr/api/controller/ApiAccountControllerTest.java(5 hunks)src/test/java/org/storkforge/barkr/api/controller/ApiKeyControllerTest.java(1 hunks)src/test/java/org/storkforge/barkr/api/controller/ApiPostControllerTest.java(4 hunks)src/test/java/org/storkforge/barkr/graphql/AccountResolverTest.java(4 hunks)src/test/java/org/storkforge/barkr/graphql/PostResolverTest.java(4 hunks)src/test/java/org/storkforge/barkr/web/controller/WebControllerImageRouteTest.java(5 hunks)src/test/java/org/storkforge/barkr/web/controller/WebControllerTest.java(9 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/main/java/org/storkforge/barkr/config/Config.java (1)
src/main/java/org/storkforge/barkr/config/SecurityConfig.java (1)
Configuration(17-94)
src/main/java/org/storkforge/barkr/domain/entity/GoogleAccountApiKeyLink.java (2)
src/main/java/org/storkforge/barkr/domain/entity/IssuedApiKey.java (1)
Entity(15-171)src/main/java/org/storkforge/barkr/domain/entity/Account.java (1)
Entity(15-170)
src/main/java/org/storkforge/barkr/domain/entity/IssuedApiKey.java (2)
src/main/java/org/storkforge/barkr/domain/entity/GoogleAccountApiKeyLink.java (1)
Entity(12-77)src/main/java/org/storkforge/barkr/domain/entity/Account.java (1)
Entity(15-170)
🪛 GitHub Actions: Build & Test
src/test/java/org/storkforge/barkr/web/controller/WebControllerTest.java
[error] 1-1: Multiple test errors due to failure to load ApplicationContext caused by missing 'SECRET_KEY' placeholder preventing proper bean initialization.
🔇 Additional comments (55)
src/main/resources/application.properties (1)
5-5: Approve addition of the joke prompt property.
The newai.mistral.joke-promptkey with the AI joke-generation prompt is correctly formatted, and the trailing newline ensures consistent file formatting.src/main/java/org/storkforge/barkr/dto/apiKeyDto/UpdateApiKey.java (1)
9-13: Record definition looks correct.
TheUpdateApiKeyrecord captures the necessary fields for partial API key updates, with proper@NotNullvalidation onreferenceId. Optional fields allow selective updates without forcing values..gitignore (1)
34-34: No review needed for this formatting-only update.src/test/java/org/storkforge/barkr/graphql/AccountResolverTest.java (1)
13-13: Security enhancement properly implemented with @WithMockUser annotationsThe addition of
@WithMockUserannotations to all test methods ensures they run with proper authentication context, which is necessary for the new security configuration. This approach correctly simulates an authenticated user when testing GraphQL API endpoints that now require authentication.Also applies to: 37-37, 57-57, 84-84
src/main/resources/messages_uwu.properties (1)
13-16: API key management localization keys added properlyThe new localization keys for API key management features align with the security enhancements and maintain the consistent "uwu" style of this localization file.
src/test/java/org/storkforge/barkr/api/controller/ApiPostControllerTest.java (1)
7-7: Security enhancement properly implemented with @WithMockUser annotationsThe addition of
@WithMockUserannotations to all test methods ensures they run with proper authentication context, which is necessary for the new security configuration. This approach correctly simulates an authenticated user when testing REST API endpoints that now require authentication.Also applies to: 37-37, 62-62, 80-80
src/test/java/org/storkforge/barkr/graphql/PostResolverTest.java (1)
11-11: Good addition of security context for GraphQL testsThe addition of
@WithMockUserannotations to the test methods properly configures Spring Security's mock authentication context for these tests. This ensures that the tests accurately simulate an authenticated user when calling the GraphQL endpoints, which is crucial for testing secured endpoints.Also applies to: 39-40, 66-67, 92-93
src/main/resources/application-dev.properties (1)
16-19: OAuth2 client registration correctly configuredThe Google OAuth2 client registration is properly set up with appropriate scopes and redirect URI pattern.
src/main/resources/static/js/generate-joke.js (1)
1-15: Good defensive programming implementationThe refactored code properly handles the case where the joke button doesn't exist in the DOM (likely due to role-based access control). This ensures the script doesn't throw errors for users who don't have the PREMIUM role, and the detailed error message makes debugging easier.
src/main/java/org/storkforge/barkr/infrastructure/persistence/AccountRepository.java (1)
3-3: Good addition of Google OIDC repository method with validationThe new
findByGoogleOidc2Idmethod with@NotBlankvalidation is correctly implemented to support account lookup via Google OIDC ID, which is essential for OAuth2 authentication integration.Also applies to: 17-17
src/test/java/org/storkforge/barkr/api/controller/ApiAccountControllerTest.java (2)
7-7: Good addition of the Spring Security test support.Adding the import for
@WithMockUseris appropriate for the security enhancements being implemented.
39-39: Proper test authentication setup.Adding
@WithMockUserannotations to the test methods ensures they can access secured endpoints by providing mock authentication. This is a good practice when implementing Spring Security in a project.Also applies to: 61-61, 75-75
src/main/java/org/storkforge/barkr/domain/AccountService.java (1)
33-35: Good implementation of Google OIDC account lookup.The method correctly handles looking up an account by Google OIDC ID, returning an
Optional<Account>which is consistent with repository pattern best practices.src/main/java/org/storkforge/barkr/ai/controller/MistralAiController.java (1)
29-29: Proper role-based access control implementation.Using
@PreAuthorize("hasRole('PREMIUM')")is an effective way to restrict the AI generation endpoint to premium users only. This aligns well with the security enhancements described in the PR objectives.src/main/java/org/storkforge/barkr/dto/apiKeyDto/ResponseApiKeyList.java (1)
5-5: Approve immutable DTO implementation.The new
ResponseApiKeyListrecord cleanly encapsulates the list of API key DTOs and leverages Java 16 records for immutability.src/main/java/org/storkforge/barkr/dto/apiKeyDto/ResponseApiKeyOnce.java (1)
7-11: Approve one-time API key DTO.The
ResponseApiKeyOncerecord correctly enforces non-blank validation and implementsSerializablefor transport. The design aligns with other API key DTOs.src/main/resources/templates/fragments/barkr-logout.html (1)
1-25: Approve logout confirmation page.The template cleanly informs users about Google logout and redirects back to the application. The Thymeleaf resource inclusion and script flow are straightforward.
src/main/resources/static/js/update-apikey-name.js (1)
1-10: Clean and correct function logic
ThepromptAndSetNamefunction correctly retrieves the input element, usespromptto gather user input, validates non-empty names, and submits the form accordingly.src/main/resources/db/migration/V4__Create_google_account_api_key_link_table.sql (1)
9-10: Unique constraint ensures one-to-one relationship
ApplyingUNIQUE (google_oidc2id)correctly enforces that each Google account can be linked to at most one API key record, matching your domain model.src/main/resources/db/migration/V7__adds_referenceID_uuid_to_apikey_table.sql (4)
2-2: Ensurepgcryptoextension availability
UsingCREATE EXTENSION IF NOT EXISTS pgcryptoguaranteesgen_random_uuid()is available without error if already installed.
7-9: Backfill existing rows with UUIDs
TheUPDATEcorrectly assigns unique UUIDs to pre-existing records, preparing for theNOT NULLconstraint.
11-13: Enforce the NOT NULL constraint
Altering the column toSET NOT NULLafter backfilling ensures data integrity without risking constraint violations.
14-16: Add unique constraint onreference_id
AddingUNIQUE (reference_id)enforces distinctness of API key identifiers, aligning with the requirement for UUID-based key lookups.pom.xml (4)
61-64: Dependencies correctly managed by Spring Boot BOM
Thespring-boot-starter-securitydependency leverages the parent BOM for its version, so no explicit version is required.
65-69: Security testing dependency scoped appropriately
Includingspring-security-testwithtestscope ensures security-related tests can use mock authentication without polluting production artifacts.
70-73: Thymeleaf Extras integration for Spring Security
Thethymeleaf-extras-springsecurity6dependency enables thesec:dialect. Verify that the BOM pulls in a compatible version for Thymeleaf 3 / Spring Boot 3.
74-77: OAuth2 client starter in place
Addingspring-boot-starter-oauth2-clientis necessary for Google OIDC integration. Confirm that no additional configuration is needed beyond what’s inSecurityConfig.src/main/resources/templates/apikeys.html (1)
21-23: Ensurenowmodel attribute is provided in correct format
Theminconstraint on thedatetime-localfield relies on anowattribute formatted asyyyy-MM-dd'T'HH:mm. Verify that the controller populatesnowaccordingly.src/main/resources/templates/profile.html (3)
21-39: Profile actions section now includes API key generation and premium unlock featuresThe profile actions section has been enhanced with an API key generation form and a premium feature unlock section. This aligns well with the PR objectives of creating security configurations for different roles.
A few suggestions:
- Consider adding a brief explanation of what API keys are used for to help users understand their purpose
- The premium unlock section could benefit from more styling to make it stand out as a special feature
33-33: Good security implementation with CSRF tokenProperly including the CSRF token in the form submission is an important security practice to prevent cross-site request forgery attacks.
94-94: New JavaScript file inclusion for premium unlock functionalityThe addition of unlock-easter-egg.js script aligns with the new premium unlock feature. Make sure this script is properly created and includes appropriate error handling for failed unlock attempts.
src/main/java/org/storkforge/barkr/config/Config.java (2)
38-54: Added role-based security to seeded accountsGood implementation of role-based security for the initial accounts. Each account is properly assigned the USER role, which aligns with the security configuration shown in the relevant code snippets.
56-67: Properly linked API key associations to accountsThe GoogleAccountApiKeyLink creation and bidirectional relationship setup is well implemented. This ensures that each seeded account has the necessary infrastructure for API key management.
src/test/java/org/storkforge/barkr/web/controller/WebControllerImageRouteTest.java (2)
42-43: Tests properly updated with authentication contextAdding the @WithMockUser annotation to each test method ensures they will run with the appropriate security context. This is essential when testing secured endpoints.
Also applies to: 54-55, 72-73, 83-84, 95-96, 108-109
76-76: CSRF token properly added to form submissions in testsAdding the CSRF token to form submissions in tests using
.with(csrf())is crucial for simulating secure form submissions. This ensures the tests properly reflect the security requirements of the application.Also applies to: 88-88, 101-101, 116-116
src/main/java/org/storkforge/barkr/infrastructure/persistence/GoogleAccountApiKeyLinkRepository.java (1)
10-15: Looks good!The repository interface is well-designed, extending JpaRepository with appropriate entity type and ID. The custom findByAccount method with @NotNull constraint ensures proper input validation.
src/main/resources/static/css/styles.css (1)
463-525: Responsive form styling follows best practices.The CSS additions provide good responsive design for smaller screens, properly using existing theme variables and creating consistent styling for forms and tables. The use of CSS transitions for focus states improves user experience.
src/main/java/org/storkforge/barkr/domain/CustomOidc2UserService.java (1)
62-67: Good security practice for role mapping.The code properly maps application-specific roles to Spring Security authorities, ensuring that authenticated users have the appropriate permissions in the security context.
src/main/resources/templates/apikeys/mykeys.html (2)
92-96: Ensure server-side validation for API key name updatesThe form for updating the API key name collects input via JavaScript but doesn't show client-side validation. Ensure there's proper server-side validation in the controller to prevent empty or malicious inputs.
105-108: LGTM: Scripts properly included at end of bodyGood practice to include all scripts at the end of the body for better page loading performance.
src/main/java/org/storkforge/barkr/filters/ApiKeyAuthenticationFilter.java (1)
32-38: LGTM: Secure token creation after API key validationThe filter correctly validates the API key before setting the authentication token in the security context.
src/main/java/org/storkforge/barkr/infrastructure/persistence/IssuedApiKeyRepository.java (2)
15-18: LGTM: Repository follows Spring Data JPA conventionsThe repository interface properly extends JpaRepository with appropriate entity and ID type parameters.
25-27: LGTM: Well-implemented modifying query for revoking expired keysThe modifying query for revoking expired keys is correctly implemented with proper annotation and parameter handling.
src/main/java/org/storkforge/barkr/domain/entity/GoogleAccountApiKeyLink.java (2)
20-23: LGTM: Properly configured one-to-one relationship with immutable join columnThe one-to-one relationship with the Account entity is correctly configured with appropriate fetch type and constraints. The join column is properly marked as unique, non-nullable, and non-updatable.
54-68: LGTM: Properly implemented equals and hashCode methods for Hibernate entitiesThe implementation correctly handles Hibernate proxies, which is important for entity comparison in JPA.
src/test/java/org/storkforge/barkr/web/controller/WebControllerTest.java (2)
83-90: Good implementation of bidirectional relationship setup.The bidirectional relationship between
AccountandGoogleAccountApiKeyLinkis properly established with both sides of the relationship being set.
140-140: Good addition of@WithMockUserannotations for secured endpoints testing.Adding
@WithMockUserannotations to these test methods properly simulates authenticated contexts, which is essential for testing secured endpoints. This aligns with the security enhancements introduced in the PR.Also applies to: 161-161, 175-175, 196-196, 242-242
src/main/java/org/storkforge/barkr/mapper/ApiKeyMapper.java (1)
14-15: Good implementation of utility class with private constructor.The private constructor prevents instantiation, which is the correct pattern for a utility class with static methods.
src/main/java/org/storkforge/barkr/domain/entity/IssuedApiKey.java (2)
15-56: Well-designed entity with appropriate constraints.The
IssuedApiKeyentity is well-structured with appropriate JPA annotations and validation constraints. The relationship withGoogleAccountApiKeyLinkis properly defined with a@ManyToOneannotation and a foreign key column.
67-72:Details
❓ Verification inconclusive
Review the short API key expiration time.
The
@PrePersistcallback sets the expiration time to just 5 minutes after issuance if not specified. This seems very short for an API key. Consider if this is the intended default behavior or if a longer default expiration would be more appropriate (e.g., 24 hours).
🏁 Script executed:
#!/bin/bash # Description: Check for references to API key expiration to determine if this short default is intentional # Check for references to API key expiration in service classes echo "Checking service implementations for API key expiration handling..." rg -A 5 -B 5 'expiresAt|expiration' --glob '*ApiKey*Service*.java' # Check for UI/form references to expiration to see if users can select it echo "Checking UI templates for expiration settings..." rg 'expiresAt|expiration' --glob '*.html' --glob '*.js'Length of output: 1835
Review default API key expiration fallback
The
@PrePersistmethod in IssuedApiKey (lines 67–72) assigns a 5 minute expiry if noexpiresAtis provided. Since the UI (apikeys.html) marks expiration as optional, any key created without an explicit date will expire in just 5 minutes.Please confirm whether a 5‑minute fallback is intentional or if you’d prefer a longer default (e.g. 24 hours) or make it configurable. You may also want to centralize this value in a constant or application property and document it.
• src/main/java/org/storkforge/barkr/domain/entity/IssuedApiKey.java (lines 67–72) – adjust default expiration
• src/main/resources/templates/apikeys.html – ensure UI guidance matches chosen defaultsrc/test/java/org/storkforge/barkr/api/controller/ApiKeyControllerTest.java (3)
35-48: Good test class configuration for controller testing.The test class is well-configured with
@WebMvcTestfor focused controller testing and appropriate mock dependencies. The@WithMockUserannotation at the class level ensures all tests run with an authenticated user context.
60-102: Well structured API key generation and display tests.These tests effectively verify the behavior of the API key generation and display endpoints, including error cases like redirects on null responses.
104-119: Excellent test for retry logic on duplicate API keys.This test correctly verifies the controller's retry logic for generating unique API keys, using Mockito to simulate first a duplicate and then a unique key.
src/main/java/org/storkforge/barkr/config/SecurityConfig.java (1)
31-31: Verify role prefix consistency
hasRole(BarkrRole.PREMIUM.name())translates to the authorityROLE_PREMIUMonly ifBarkrRole#getAuthorityValue()returns"ROLE_PREMIUM".
Double‑check your enum; if it already prefixes withROLE_, callers will holdROLE_ROLE_PREMIUMand the check will silently fail.src/main/java/org/storkforge/barkr/api/controller/ApiKeyController.java (1)
90-96: PotentialNoSuchElementExceptionon missing account
accountRepository.findByGoogleOidc2Id(..)returns anOptional, butget()is called without checking presence.
If an OIDC user exists in the IdP but not in your DB (race‑condition first login), the method crashes with 500.
Wrap withorElseThrow()that returns a controlled 404/403 page or create the account automatically.
| CREATE TABLE issued_api_key | ||
| ( | ||
| id BIGINT GENERATED BY DEFAULT AS IDENTITY NOT NULL, | ||
| hashed_api_key VARCHAR(255) NOT NULL, | ||
| issued_at TIMESTAMP WITHOUT TIME ZONE NOT NULL, | ||
| expires_at TIMESTAMP WITHOUT TIME ZONE NOT NULL, | ||
| revoked BOOLEAN NOT NULL, | ||
| last_used_at TIMESTAMP WITHOUT TIME ZONE NOT NULL, | ||
| api_key_name VARCHAR(255) NOT NULL, | ||
| google_account_api_key_link_id BIGINT NOT NULL, | ||
| CONSTRAINT pk_issued_api_key PRIMARY KEY (id) | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making last_used_at nullable and adding default values.
Currently, last_used_at is NOT NULL, which forces a timestamp even before any usage; allowing it to be NULL more clearly represents “never used,” and you can update it on first use. Similarly, adding DEFAULT NOW() for issued_at simplifies inserts:
- issued_at TIMESTAMP WITHOUT TIME ZONE NOT NULL,
+ issued_at TIMESTAMP WITHOUT TIME ZONE NOT NULL DEFAULT NOW(),
...
- last_used_at TIMESTAMP WITHOUT TIME ZONE NOT NULL,
+ last_used_at TIMESTAMP WITHOUT TIME ZONE NULL,This reduces boilerplate and aligns the schema with typical usage patterns.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CREATE TABLE issued_api_key | |
| ( | |
| id BIGINT GENERATED BY DEFAULT AS IDENTITY NOT NULL, | |
| hashed_api_key VARCHAR(255) NOT NULL, | |
| issued_at TIMESTAMP WITHOUT TIME ZONE NOT NULL, | |
| expires_at TIMESTAMP WITHOUT TIME ZONE NOT NULL, | |
| revoked BOOLEAN NOT NULL, | |
| last_used_at TIMESTAMP WITHOUT TIME ZONE NOT NULL, | |
| api_key_name VARCHAR(255) NOT NULL, | |
| google_account_api_key_link_id BIGINT NOT NULL, | |
| CONSTRAINT pk_issued_api_key PRIMARY KEY (id) | |
| ); | |
| CREATE TABLE issued_api_key | |
| ( | |
| id BIGINT GENERATED BY DEFAULT AS IDENTITY NOT NULL, | |
| hashed_api_key VARCHAR(255) NOT NULL, | |
| issued_at TIMESTAMP WITHOUT TIME ZONE NOT NULL DEFAULT NOW(), | |
| expires_at TIMESTAMP WITHOUT TIME ZONE NOT NULL, | |
| revoked BOOLEAN NOT NULL, | |
| last_used_at TIMESTAMP WITHOUT TIME ZONE NULL, | |
| api_key_name VARCHAR(255) NOT NULL, | |
| google_account_api_key_link_id BIGINT NOT NULL, | |
| CONSTRAINT pk_issued_api_key PRIMARY KEY (id) | |
| ); |
| public record CreateApiKey( | ||
| @NotBlank String hashedApiKey, | ||
| @PastOrPresent LocalDateTime issuedAt, | ||
| @FutureOrPresent LocalDateTime expiresAt, | ||
| @NotNull Boolean revoked, | ||
| @PastOrPresent LocalDateTime lastUsedAt, | ||
| @NotBlank String apiKeyName, | ||
| @NotNull GoogleAccountApiKeyLink googleAccountApiKeyLink, | ||
| @NotBlank UUID referenceId) implements Serializable { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix validation annotation on referenceId and tighten lastUsedAt nullability
- The
@NotBlankannotation applies only to CharSequence types, notUUID. Replace it with@NotNullforreferenceId. - If
lastUsedAtmust always be provided (even if it's the same asissuedAtinitially), annotate it with@NotNull; otherwise document that it may benulluntil first use.
| <li><a th:href="@{/apikeys/mykeys}"><i>🔑</i> <span th:text="#{mykeys}">My Keys</span> | ||
| </a> | ||
| </li> |
There was a problem hiding this comment.
Ensure translation key for mykeys exists
The new menu item uses th:text="#{mykeys}", but there’s no corresponding key in your localization files. This will render the literal #{mykeys}. Please add mykeys=My Keys to each messages_*.properties file.
| var account = accountService.findByGoogleOidc2Id(user.getName()).orElseThrow(() -> new AccountNotFound("No account found")); | ||
| account.setRoles(new HashSet<>(Set.of(BarkrRole.USER, BarkrRole.PREMIUM))); | ||
|
|
||
| Set<GrantedAuthority> mappedAuthorities = account.getRoles().stream() | ||
| .map(role -> new SimpleGrantedAuthority(role.getAuthorityValue())) | ||
| .collect(Collectors.toSet()); | ||
|
|
||
| OidcUser updateUser = new DefaultOidcUser(mappedAuthorities, user.getIdToken(), user.getUserInfo()); | ||
| OAuth2AuthenticationToken auth = new OAuth2AuthenticationToken(updateUser, updateUser.getAuthorities(), updateUser.getAccessTokenHash()); | ||
|
|
There was a problem hiding this comment.
Compilation & persistence issues in premium‑unlock flow
DefaultOidcUserhas nogetAccessTokenHash(); the third ctor argument ofOAuth2AuthenticationTokenexpects the registrationId.- Role changes are not persisted; the modified entity isn’t saved and the method is not
@Transactional. - Missing null‑check on
user.
- account.setRoles(new HashSet<>(Set.of(BarkrRole.USER, BarkrRole.PREMIUM)));
+ account.setRoles(new HashSet<>(Set.of(BarkrRole.USER, BarkrRole.PREMIUM)));
+ accountService.save(account); // or wrap the method with @Transactional
...
- OAuth2AuthenticationToken auth = new OAuth2AuthenticationToken(updateUser, updateUser.getAuthorities(), updateUser.getAccessTokenHash());
+ String registrationId = ((OAuth2AuthenticationToken) SecurityContextHolder.getContext()
+ .getAuthentication()).getAuthorizedClientRegistrationId();
+ OAuth2AuthenticationToken auth =
+ new OAuth2AuthenticationToken(updateUser, updateUser.getAuthorities(), registrationId);Add a guard:
if (user == null) {
return ResponseEntity.status(HttpStatus.UNAUTHORIZED).body("Login required");
}Without these fixes the code will not compile and the role upgrade will not survive the request.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/org/storkforge/barkr/web/controller/WebController.java (1)
177-211:⚠️ Potential issueRole changes are not persisted to the database.
The method correctly validates the unlock code and updates the user's roles in memory, but these changes won't persist between sessions because the account isn't saved back to the database.
Add a call to save the updated account:
var account = accountService.findByGoogleOidc2Id(user.getName()).orElseThrow(() -> new AccountNotFound("No account found")); account.setRoles(new HashSet<>(Set.of(BarkrRole.USER, BarkrRole.PREMIUM))); +accountService.save(account); // or wrap the method with @Transactional
🧹 Nitpick comments (2)
src/main/java/org/storkforge/barkr/web/controller/WebController.java (2)
186-189: Consider moving the secret code to a configuration property.The hardcoded Konami code is clever, but storing it directly in the code makes it difficult to change and potentially exposes it in source control. Consider moving it to a configuration property.
+@Value("${barkr.premium.secretCode:upupdowndownleftrightleftrightba}") +private String premiumUnlockCode; ... -if (!sanitizedCode.trim().equals("upupdowndownleftrightleftrightba")) { +if (!sanitizedCode.trim().equals(premiumUnlockCode)) {Don't forget to add the required import:
import org.springframework.beans.factory.annotation.Value;
184-187: Improve input sanitization and validation.The code sanitizes by removing non-alphanumeric characters, but then still calls trim() on the result. Consider combining these operations for cleaner, more efficient code.
-var sanitizedCode = code.toLowerCase().replaceAll("[^a-zA-Z0-9]", ""); - -if (!sanitizedCode.trim().equals("upupdowndownleftrightleftrightba")) { +var sanitizedCode = code.toLowerCase().replaceAll("[^a-zA-Z0-9]", "").trim(); + +if (!sanitizedCode.equals("upupdowndownleftrightleftrightba")) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/main/java/org/storkforge/barkr/ai/controller/MistralAiController.java(3 hunks)src/main/java/org/storkforge/barkr/api/controller/ApiKeyController.java(1 hunks)src/main/java/org/storkforge/barkr/config/SecurityConfig.java(1 hunks)src/main/java/org/storkforge/barkr/domain/CustomOidc2UserService.java(1 hunks)src/main/java/org/storkforge/barkr/domain/IssuedApiKeyService.java(1 hunks)src/main/java/org/storkforge/barkr/domain/PostService.java(0 hunks)src/main/java/org/storkforge/barkr/domain/entity/IssuedApiKey.java(1 hunks)src/main/java/org/storkforge/barkr/dto/apiKeyDto/CreateApiKey.java(1 hunks)src/main/java/org/storkforge/barkr/dto/apiKeyDto/ResponseApiKey.java(1 hunks)src/main/java/org/storkforge/barkr/filters/ApiKeyAuthenticationFilter.java(1 hunks)src/main/java/org/storkforge/barkr/mapper/ApiKeyMapper.java(1 hunks)src/main/java/org/storkforge/barkr/web/controller/WebController.java(5 hunks)src/main/resources/messages.properties(1 hunks)src/main/resources/messages_uwu.properties(1 hunks)src/main/resources/templates/fragments/header.html(2 hunks)src/main/resources/templates/fragments/sidebar.html(1 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/org/storkforge/barkr/domain/PostService.java
🚧 Files skipped from review as they are similar to previous changes (14)
- src/main/java/org/storkforge/barkr/ai/controller/MistralAiController.java
- src/main/resources/templates/fragments/sidebar.html
- src/main/resources/messages.properties
- src/main/resources/messages_uwu.properties
- src/main/resources/templates/fragments/header.html
- src/main/java/org/storkforge/barkr/dto/apiKeyDto/ResponseApiKey.java
- src/main/java/org/storkforge/barkr/filters/ApiKeyAuthenticationFilter.java
- src/main/java/org/storkforge/barkr/dto/apiKeyDto/CreateApiKey.java
- src/main/java/org/storkforge/barkr/mapper/ApiKeyMapper.java
- src/main/java/org/storkforge/barkr/domain/CustomOidc2UserService.java
- src/main/java/org/storkforge/barkr/api/controller/ApiKeyController.java
- src/main/java/org/storkforge/barkr/domain/entity/IssuedApiKey.java
- src/main/java/org/storkforge/barkr/config/SecurityConfig.java
- src/main/java/org/storkforge/barkr/domain/IssuedApiKeyService.java
🔇 Additional comments (3)
src/main/java/org/storkforge/barkr/web/controller/WebController.java (3)
58-70: User authentication logic enhancement looks good.The changes to dynamically identify users based on their OIDC identity rather than using hardcoded IDs improve security and personalization. This properly integrates with the new authentication system.
76-82: User profile loading logic correctly handles authenticated users.The implementation properly retrieves the user's account by their Google OIDC ID and falls back to the URL parameter when needed. The username override when available is a good touch for consistency.
103-107: Well-implemented logout functionality.The logout implementation is clean and straightforward, properly clearing the security context before redirecting to the logout confirmation page.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/resources/application.properties (1)
8-11: Enhance Google OIDC auto-configuration
While the client registration properties are correct, you can simplify maintenance and improve reliability by leveraging Spring Boot’s OIDC metadata discovery via theissuer-uri. This allows auto-configuration of endpoints (authorization, token, JWKS) and keeps your setup DRY.Consider adding:
+ spring.security.oauth2.client.provider.google.issuer-uri=https://accounts.google.comAlso verify that your
{baseUrl}placeholder resolves as expected in all deployment environments—otherwise switch toredirect-uri-templateor explicitly configure profiles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/storkforge/barkr/config/SecurityConfig.java(1 hunks)src/main/resources/application-dev.properties(1 hunks)src/main/resources/application.properties(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/resources/application-dev.properties
- src/main/java/org/storkforge/barkr/config/SecurityConfig.java
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/org/storkforge/barkr/config/SecurityConfig.java (1)
89-94: Remove unnecessary empty lines.There are several empty lines at the end of the file that can be removed to improve code cleanliness.
- - - - - -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/storkforge/barkr/config/SecurityConfig.java(1 hunks)src/main/resources/application.properties(1 hunks)src/test/java/org/storkforge/barkr/web/controller/WebControllerTest.java(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/resources/application.properties
- src/test/java/org/storkforge/barkr/web/controller/WebControllerTest.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/org/storkforge/barkr/config/SecurityConfig.java (1)
src/main/java/org/storkforge/barkr/filters/ApiKeyAuthenticationFilter.java (1)
ApiKeyAuthenticationFilter(18-76)
🔇 Additional comments (8)
src/main/java/org/storkforge/barkr/config/SecurityConfig.java (8)
19-22: Well-structured security configuration with appropriate annotations.The class is properly configured with
@Configuration,@EnableWebSecurity, and@EnableMethodSecurityannotations, which align with Spring Security best practices for enabling both web and method-level security.
24-26: Correctly ordered filter chain configuration.The web security filter chain has a lower priority (order 3) than the API filter chain, ensuring that API requests are handled by the appropriate chain first.
29-46: URL security patterns follow the principle of least privilege.The authorization rules are well-structured, following security best practices:
- Explicitly permitting public access only to necessary resources
- Requiring authentication for user-specific endpoints
- Requiring the PREMIUM role for premium features
- Denying all other requests by default (fail-safe approach)
48-55: Thorough logout configuration.The logout configuration properly handles session cleanup by invalidating the session, clearing authentication, and removing cookies, which helps prevent session fixation attacks.
64-68: Good separation of API security concerns.Using a separate security filter chain for API endpoints allows for different security requirements like stateless sessions and API key authentication.
69-72: API security configuration follows best practices.The API security configuration:
- Correctly implements stateless session management (addressing the past review comment)
- Disables CSRF for the stateless API (appropriate for token-based authentication)
- Adds the custom API key authentication filter in the right position
73-83: Secure API endpoint authorization rules.The authorization configuration for API endpoints follows security best practices:
- All API endpoints require authentication
- GraphQL POST requests require authentication
- Denies all other requests by default (fail-safe approach)
56-58:Details
✅ Verification successful
Verify CSRF exclusion for the /ai/generate endpoint.
CSRF protection is disabled for the
/ai/generateendpoint. This might be a security risk if this endpoint performs state-changing operations, especially since it's marked as requiring the PREMIUM role.
🏁 Script executed:
#!/bin/bash # Let's check the controller method for /ai/generate to understand if it's modifying state echo "Searching for controller method handling /ai/generate..." rg -t java "(/ai/generate|@.*Mapping.*\"ai/generate\")" --context 10Length of output: 7261
CSRF exclusion for
/ai/generateis safe and redundant
The/ai/generateendpoint is a@GetMappingthat merely returns AI‑generated content and doesn’t modify any server‑side state. Spring Security’s CSRF protection only applies to state‑changing methods (POST, PUT, PATCH, DELETE), so excluding this GET is safe (and effectively redundant). No changes required.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/test/java/org/storkforge/barkr/web/controller/WebControllerTest.java (2)
93-108: Security context and entity setup for OIDC integrationThe
@WithMockUserannotation and the setup ofGoogleAccountApiKeyLinkcorrectly reflect the new security model. However, there are a few points to consider:
- The explicit setup of
GoogleAccountApiKeyLinkseems repetitive across test methods- The bidirectional relationship setup between Account and GoogleAccountApiKeyLink is good practice
- Setting roles with
HashSet<>(Set.of(...))is redundant - you could just useSet.of(...)Consider extracting the account setup logic to a helper method to reduce duplication:
- Account mockAccount = new Account(); - GoogleAccountApiKeyLink mockKeyLink = new GoogleAccountApiKeyLink(); - - mockAccount.setUsername("mockAccount"); - mockAccount.setBreed("husky"); - mockAccount.setGoogleOidc2Id("6"); - - mockAccount.setImage(null); - mockAccount.setRoles(new HashSet<>(Set.of(BarkrRole.USER, BarkrRole.PREMIUM))); - - mockKeyLink.setAccount(mockAccount); - mockAccount.setGoogleAccountApiKeyLink(mockKeyLink); + Account mockAccount = createTestAccount("mockAccount", "husky", "6", Set.of(BarkrRole.USER, BarkrRole.PREMIUM));With a helper method:
private Account createTestAccount(String username, String breed, String oidcId, Set<BarkrRole> roles) { Account account = new Account(); GoogleAccountApiKeyLink keyLink = new GoogleAccountApiKeyLink(); account.setUsername(username); account.setBreed(breed); account.setGoogleOidc2Id(oidcId); account.setImage(null); account.setRoles(roles); keyLink.setAccount(account); account.setGoogleAccountApiKeyLink(keyLink); return account; }
144-145: Specific authorities in @WithMockUser annotationYou're using
@WithMockUser(authorities = {"ROLE_USER", "ROLE_PREMIUM"})here but the simple@WithMockUserelsewhere. Using specific authorities is more precise and tests the actual role-based access control configuration.Consider using specific authorities in all
@WithMockUserannotations to ensure your tests accurately reflect the security configuration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/java/org/storkforge/barkr/web/controller/WebControllerTest.java(11 hunks)
🔇 Additional comments (4)
src/test/java/org/storkforge/barkr/web/controller/WebControllerTest.java (4)
10-28: Appropriate imports for new security and API key featuresThe added imports support the new security features and API key management functionality. The inclusion of Spring Security test utilities like
@WithMockUseris particularly important for testing authenticated endpoints.
59-64: Unused mocks in test methodsYou've added mocks for
IssuedApiKeyServiceand injected them intoApiKeyController, but these aren't being used in any of the test methods. This suggests that either:
- Tests for the API key functionality are missing, or
- These mocks were added prematurely before their corresponding tests were implemented.
Are there plans to add tests that use these mocks in a future PR? If not, consider adding tests that verify the API key functionality using these mocks.
148-149: Form retrieval by name is more robustUsing
getFormByName("index-first-form")is more precise than getting forms by position. This change aligns with best practices for form selection in tests.
253-255: Verify role-based access control testingClearing the roles of
mockAccount2is a good way to test role-dependent behavior. However, this change occurs after the page is loaded, so it won't affect the current test's assertions.Verify if this change is intentional or if you meant to clear the roles before loading the page to test different access patterns. If you're testing that users can only see their own posts (not role-based), this is correct, but the comment could be clarified.
| mockAccount2.setRoles(Set.of()); | ||
| String pageContent = page.asNormalizedText(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding specific role-based access tests
Since you've implemented role-based security, consider adding specific tests that verify that premium features are only accessible to accounts with the PREMIUM role.
Add test methods that specifically verify:
- Users with PREMIUM role can access premium features
- Users without PREMIUM role cannot access premium features
Example test structure:
@Test
@WithMockUser(authorities = {"ROLE_USER"})
@DisplayName("Regular users cannot access premium features")
void regularUserCannotAccessPremiumFeatures() throws IOException {
// Setup accounts with different roles
// ...
// Attempt to access premium feature
// ...
// Verify access is denied
// ...
}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/main/java/org/storkforge/barkr/infrastructure/persistence/IssuedApiKeyRepository.java (4)
16-18: Consider removing redundant method override.The
findByIdmethod is already provided by the JpaRepository interface, so explicitly overriding it here is unnecessary unless you're planning to add custom behavior. Consider removing this override to reduce code clutter.- @Override - Optional<IssuedApiKey> findById(Long id);
21-21: Method name is clear but quite long.The method name clearly describes its purpose but is quite lengthy. This is a common trade-off in Spring Data JPA query methods. Consider adding a documentation comment to explain the method's purpose and behavior.
+ /** + * Finds all API keys for a Google account, ordered by issuance date (newest first). + * + * @param googleAccountApiKeyLink The Google account to find API keys for + * @return List of API keys ordered by issuance date descending + */ List<IssuedApiKey> findByGoogleAccountApiKeyLinkOrderByIssuedAtDesc(@NotNull GoogleAccountApiKeyLink googleAccountApiKeyLink);
29-29: Remove extra blank line.There's an extra blank line that creates inconsistent spacing in the interface.
int revokeExpiredKeys(@Param("now") LocalDateTime now); - Boolean existsByReferenceIdAndGoogleAccountApiKeyLink(@NotNull UUID uuid, GoogleAccountApiKeyLink googleAccountApiKeyLink);
30-30: Consider consistency in@NotNullannotations and return type.
- The method applies
@NotNullto theuuidparameter but not togoogleAccountApiKeyLink. Consider being consistent with validation annotations.- The method returns a boxed
Booleanrather than a primitiveboolean. Using the primitive type is generally more efficient unless you specifically need to handlenullreturn values.- Boolean existsByReferenceIdAndGoogleAccountApiKeyLink(@NotNull UUID uuid, GoogleAccountApiKeyLink googleAccountApiKeyLink); + boolean existsByReferenceIdAndGoogleAccountApiKeyLink(@NotNull UUID uuid, @NotNull GoogleAccountApiKeyLink googleAccountApiKeyLink);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/org/storkforge/barkr/api/controller/ApiKeyController.java(1 hunks)src/main/java/org/storkforge/barkr/domain/IssuedApiKeyService.java(1 hunks)src/main/java/org/storkforge/barkr/infrastructure/persistence/GoogleAccountApiKeyLinkRepository.java(1 hunks)src/main/java/org/storkforge/barkr/infrastructure/persistence/IssuedApiKeyRepository.java(1 hunks)src/test/java/org/storkforge/barkr/api/controller/ApiKeyControllerTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/java/org/storkforge/barkr/infrastructure/persistence/GoogleAccountApiKeyLinkRepository.java
- src/main/java/org/storkforge/barkr/api/controller/ApiKeyController.java
- src/main/java/org/storkforge/barkr/domain/IssuedApiKeyService.java
- src/test/java/org/storkforge/barkr/api/controller/ApiKeyControllerTest.java
🔇 Additional comments (1)
src/main/java/org/storkforge/barkr/infrastructure/persistence/IssuedApiKeyRepository.java (1)
25-27: Good implementation of expired key revocation.The modifying query to automatically revoke expired keys is well-implemented. Consider adding a
@Transactionalannotation if this method will be called directly from service layer to ensure transaction management.@Modifying + @Transactional @Query("UPDATE IssuedApiKey k SET k.revoked = true WHERE k.expiresAt <= :now AND k.revoked = false") int revokeExpiredKeys(@Param("now") LocalDateTime now);
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/main/java/org/storkforge/barkr/infrastructure/persistence/IssuedApiKeyRepository.java (1)
24-25: Consider adding an index for efficient expired key queriesThe
revokeExpiredKeysquery will be executed periodically to clean up expired keys. Consider adding a database index onexpiresAtandrevokedfields to optimize this query's performance, especially as the table grows.src/test/java/org/storkforge/barkr/domain/IssuedApiKeyServiceTest.java (1)
48-49: Remove debug print statementSystem.out.println statements should be avoided in test code. Use logging or assertions instead.
- System.out.println(secretKey); assertThat(secretKey).isEqualTo("test-value");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/org/storkforge/barkr/api/controller/ApiKeyController.java(1 hunks)src/main/java/org/storkforge/barkr/domain/IssuedApiKeyService.java(1 hunks)src/main/java/org/storkforge/barkr/infrastructure/persistence/IssuedApiKeyRepository.java(1 hunks)src/test/java/org/storkforge/barkr/api/controller/ApiKeyControllerTest.java(1 hunks)src/test/java/org/storkforge/barkr/domain/IssuedApiKeyServiceTest.java(1 hunks)src/test/java/org/storkforge/barkr/web/controller/WebControllerTest.java(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/test/java/org/storkforge/barkr/api/controller/ApiKeyControllerTest.java
- src/test/java/org/storkforge/barkr/web/controller/WebControllerTest.java
- src/main/java/org/storkforge/barkr/domain/IssuedApiKeyService.java
🔇 Additional comments (2)
src/main/java/org/storkforge/barkr/infrastructure/persistence/IssuedApiKeyRepository.java (1)
15-27: Well-designed repository interface with appropriate query methodsThe interface provides all the necessary methods for API key management with good use of Optional returns for potentially missing entities. The @Modifying annotation is correctly used for the update operation.
src/main/java/org/storkforge/barkr/api/controller/ApiKeyController.java (1)
70-74: Good correction to the key generation logicThe key generation logic has been correctly implemented to generate and check uniqueness before insertion, addressing the previous feedback about the endless loop.
| @SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT) | ||
| @TestPropertySource(properties = {"barkr.api.secret=test-value"}) | ||
| class IssuedApiKeyServiceTest { | ||
| @Container | ||
| @ServiceConnection | ||
| static PostgreSQLContainer<?> postgres = new PostgreSQLContainer<>(DockerImageName.parse("postgres:latest")); | ||
|
|
||
| @Container | ||
| @ServiceConnection | ||
| static RedisContainer redis = new RedisContainer(DockerImageName.parse("redis:latest")); | ||
|
|
||
| @MockitoBean | ||
| private IssuedApiKeyRepository issuedApiKeyRepository; | ||
|
|
||
| @Value("${barkr.api.secret}") | ||
| private String secretKey; | ||
|
|
||
| @MockitoBean | ||
| private GoogleAccountApiKeyLinkRepository googleAccountApiKeyLinkRepository; | ||
|
|
||
| @Autowired | ||
| private IssuedApiKeyService service; | ||
|
|
||
|
|
||
| @Test | ||
| @DisplayName("testValue injection is working") | ||
| void testValueInjectionIsWorking() { | ||
| System.out.println(secretKey); | ||
| assertThat(secretKey).isEqualTo("test-value"); | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Test class lacks comprehensive service method coverage
While the test environment is set up correctly with test containers for PostgreSQL and Redis, the class only tests property injection and doesn't verify any actual service functionality.
Consider adding tests for core service methods:
@Test
@DisplayName("Should generate a valid API key")
void shouldGenerateValidApiKey() {
// Test implementation
}
@Test
@DisplayName("Should hash API key correctly")
void shouldHashApiKeyCorrectly() {
// Test implementation
}
@Test
@DisplayName("Should revoke expired keys")
void shouldRevokeExpiredKeys() {
// Test implementation
}| @GetMapping("/mykeys") | ||
| public String myKeys(Model model , @AuthenticationPrincipal OidcUser user) { | ||
| var currentUser = issuedApiKeyService.findByGoogleOidc2Id(user.getName()); | ||
| var keys = issuedApiKeyService.allApiKeys(user.getName()); | ||
| model.addAttribute("account", currentUser.get().getAccount()); | ||
| model.addAttribute("keys", keys.apiKeys()); | ||
| return "apikeys/mykeys"; | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add @PreAuthorize annotation for consistency
The myKeys method is missing the @PreAuthorize("hasRole('USER')") annotation that's present on other endpoints. Add it for consistency and to ensure proper role-based access control.
@GetMapping("/mykeys")
+ @PreAuthorize("hasRole('USER')")
public String myKeys(Model model , @AuthenticationPrincipal OidcUser user) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @GetMapping("/mykeys") | |
| public String myKeys(Model model , @AuthenticationPrincipal OidcUser user) { | |
| var currentUser = issuedApiKeyService.findByGoogleOidc2Id(user.getName()); | |
| var keys = issuedApiKeyService.allApiKeys(user.getName()); | |
| model.addAttribute("account", currentUser.get().getAccount()); | |
| model.addAttribute("keys", keys.apiKeys()); | |
| return "apikeys/mykeys"; | |
| } | |
| @GetMapping("/mykeys") | |
| @PreAuthorize("hasRole('USER')") | |
| public String myKeys(Model model, @AuthenticationPrincipal OidcUser user) { | |
| var currentUser = issuedApiKeyService.findByGoogleOidc2Id(user.getName()); | |
| var keys = issuedApiKeyService.allApiKeys(user.getName()); | |
| model.addAttribute("account", currentUser.get().getAccount()); | |
| model.addAttribute("keys", keys.apiKeys()); | |
| return "apikeys/mykeys"; | |
| } |
| if (issuedApiKeyService.isValidUuid(UUID.fromString(referenceId), user.getName())) { | ||
| var update = new UpdateApiKey(UUID.fromString(referenceId),null, true, null); | ||
| issuedApiKeyService.updateApiKey(update); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for UUID validation and provide user feedback
The method silently fails if the UUID is invalid. Consider adding proper error handling and redirecting with an error message if validation fails.
@PostMapping("/mykeys/revoke")
public String revokeKey(@RequestParam String referenceId, @AuthenticationPrincipal OidcUser user, RedirectAttributes redirectAttributes) {
- if (issuedApiKeyService.isValidUuid(UUID.fromString(referenceId), user.getName())) {
- var update = new UpdateApiKey(UUID.fromString(referenceId),null, true, null);
- issuedApiKeyService.updateApiKey(update);
- }
+ try {
+ UUID uuid = UUID.fromString(referenceId);
+ if (issuedApiKeyService.isValidUuid(uuid, user.getName())) {
+ var update = new UpdateApiKey(uuid, null, true, null);
+ issuedApiKeyService.updateApiKey(update);
+ redirectAttributes.addFlashAttribute("success", "API key successfully revoked");
+ } else {
+ redirectAttributes.addFlashAttribute("error", "You don't have permission to revoke this API key");
+ }
+ } catch (IllegalArgumentException e) {
+ redirectAttributes.addFlashAttribute("error", "Invalid API key reference");
+ }
return "redirect:/apikeys/mykeys";
}| public String nameUpdate(@RequestParam String apiKeyName, @RequestParam String referenceId, @AuthenticationPrincipal OidcUser user) { | ||
| if(issuedApiKeyService.isValidUuid(UUID.fromString(referenceId), user.getName())) { | ||
| var update = new UpdateApiKey(UUID.fromString(referenceId), apiKeyName, false, null); | ||
| issuedApiKeyService.updateApiKey(update); | ||
| } | ||
| return "redirect:/apikeys/mykeys"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for nameUpdate method
Similar to the revokeKey method, this method needs proper error handling for UUID validation and user feedback.
@PostMapping("mykeys/nameupdate")
- public String nameUpdate(@RequestParam String apiKeyName, @RequestParam String referenceId, @AuthenticationPrincipal OidcUser user) {
- if(issuedApiKeyService.isValidUuid(UUID.fromString(referenceId), user.getName())) {
- var update = new UpdateApiKey(UUID.fromString(referenceId), apiKeyName, false, null);
- issuedApiKeyService.updateApiKey(update);
- }
+ public String nameUpdate(@RequestParam String apiKeyName, @RequestParam String referenceId,
+ @AuthenticationPrincipal OidcUser user, RedirectAttributes redirectAttributes) {
+ try {
+ UUID uuid = UUID.fromString(referenceId);
+ if (issuedApiKeyService.isValidUuid(uuid, user.getName())) {
+ var update = new UpdateApiKey(uuid, apiKeyName, false, null);
+ issuedApiKeyService.updateApiKey(update);
+ redirectAttributes.addFlashAttribute("success", "API key name updated successfully");
+ } else {
+ redirectAttributes.addFlashAttribute("error", "You don't have permission to update this API key");
+ }
+ } catch (IllegalArgumentException e) {
+ redirectAttributes.addFlashAttribute("error", "Invalid API key reference");
+ }
return "redirect:/apikeys/mykeys";
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public String nameUpdate(@RequestParam String apiKeyName, @RequestParam String referenceId, @AuthenticationPrincipal OidcUser user) { | |
| if(issuedApiKeyService.isValidUuid(UUID.fromString(referenceId), user.getName())) { | |
| var update = new UpdateApiKey(UUID.fromString(referenceId), apiKeyName, false, null); | |
| issuedApiKeyService.updateApiKey(update); | |
| } | |
| return "redirect:/apikeys/mykeys"; | |
| @PostMapping("mykeys/nameupdate") | |
| public String nameUpdate(@RequestParam String apiKeyName, | |
| @RequestParam String referenceId, | |
| @AuthenticationPrincipal OidcUser user, | |
| RedirectAttributes redirectAttributes) { | |
| try { | |
| UUID uuid = UUID.fromString(referenceId); | |
| if (issuedApiKeyService.isValidUuid(uuid, user.getName())) { | |
| var update = new UpdateApiKey(uuid, apiKeyName, false, null); | |
| issuedApiKeyService.updateApiKey(update); | |
| redirectAttributes.addFlashAttribute("success", "API key name updated successfully"); | |
| } else { | |
| redirectAttributes.addFlashAttribute("error", "You don't have permission to update this API key"); | |
| } | |
| } catch (IllegalArgumentException e) { | |
| redirectAttributes.addFlashAttribute("error", "Invalid API key reference"); | |
| } | |
| return "redirect:/apikeys/mykeys"; | |
| } |
| var keys = issuedApiKeyService.allApiKeys(user.getName()); | ||
| model.addAttribute("account", currentUser.get().getAccount()); | ||
| model.addAttribute("keys", keys.apiKeys()); |
There was a problem hiding this comment.
Add null check for user and optional handling
The current implementation doesn't handle the case where the user might not be found. Add proper null checks and Optional handling.
@GetMapping("/mykeys")
public String myKeys(Model model , @AuthenticationPrincipal OidcUser user) {
- var currentUser = issuedApiKeyService.findByGoogleOidc2Id(user.getName());
- var keys = issuedApiKeyService.allApiKeys(user.getName());
- model.addAttribute("account", currentUser.get().getAccount());
+ var currentUser = issuedApiKeyService.findByGoogleOidc2Id(user.getName());
+ if (currentUser.isEmpty()) {
+ return "redirect:/error";
+ }
+ var keys = issuedApiKeyService.allApiKeys(user.getName());
+ model.addAttribute("account", currentUser.get().getAccount());
model.addAttribute("keys", keys.apiKeys());
return "apikeys/mykeys";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var keys = issuedApiKeyService.allApiKeys(user.getName()); | |
| model.addAttribute("account", currentUser.get().getAccount()); | |
| model.addAttribute("keys", keys.apiKeys()); | |
| @GetMapping("/mykeys") | |
| public String myKeys(Model model, @AuthenticationPrincipal OidcUser user) { | |
| var currentUser = issuedApiKeyService.findByGoogleOidc2Id(user.getName()); | |
| if (currentUser.isEmpty()) { | |
| return "redirect:/error"; | |
| } | |
| var keys = issuedApiKeyService.allApiKeys(user.getName()); | |
| model.addAttribute("account", currentUser.get().getAccount()); | |
| model.addAttribute("keys", keys.apiKeys()); | |
| return "apikeys/mykeys"; | |
| } |
2 participants
Creates security for 2 roles, fixes apikey generation, config file, no previous test are breaking.
Added some test but still more to come.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Style
Tests
Chores