Conversation
| @Override | ||
| public JWTCallerPrincipal parse(String token, JWTAuthContextInfo authContextInfo) throws ParseException { | ||
| try { | ||
| var prin = new DefaultJWTCallerPrincipal(verifier.verify(token)); |
There was a problem hiding this comment.
Can be directly returned without saving to variable.
| throw new InvalidClassifierException("Invalid V3 classifier", classifierRequest.getClassifier(), Source.builder().pointer("").build()); | ||
| } | ||
| checkTenantId(classifierRequest.getClassifier()); | ||
| checkOriginService(classifierRequest); |
There was a problem hiding this comment.
Remove the duplicated check.
| throw new InvalidClassifierException("Classifier doesn't contain all mandatory fields. " + | ||
| "Check that classifier has `microserviceName`, `scope`. If `scope` = `tenant`, classifier must contain `tenantId` property", | ||
| createRequest.getClassifier(), Source.builder().pointer("/classifier").build()); | ||
| if (!AggregatedDatabaseAdministrationService.AggregatedDatabaseAdministrationUtils.isClassifierCorrect(createRequest.getClassifier()) || |
There was a problem hiding this comment.
Let's split these two checks and throw different exception messages for them (in all similar cases).
| return; | ||
| } | ||
| if (!namespaceValidator.checkNamespaceIsolation(namespaceFromPath, JwtUtils.getNamespace(requestContext.getSecurityContext()))) { | ||
| requestContext.abortWith(Response.status(Response.Status.FORBIDDEN.getStatusCode(), "Namespace from path and namespace from jwt token doesn't not match or aren't in the same composite structure").build()); |
There was a problem hiding this comment.
Should be in the TMF format (com.netcracker.cloud.dbaas.controller.error.Utils).
| } | ||
|
|
||
| String principal = identity.getPrincipal().getName(); | ||
| String serviceName = principal.substring(principal.lastIndexOf(':') + 1); |
There was a problem hiding this comment.
Please add Javadoc or something to describe in which format we're expecting the "principal" structure.
| @NoArgsConstructor | ||
| @Slf4j | ||
| public class ServiceAccountRolesManager { | ||
| private final ArrayList<ServiceAccountWithRoles> serviceAccountsWithRoles = new ArrayList<>(); |
There was a problem hiding this comment.
Why it's a List if it's used only as a Map (searching by key)? Can we make it a Map instead? Then we can even remove ServiceAccountWithRoles dto.
There was a problem hiding this comment.
i thought arraylist would be much more performant and require less memory since there will only be a few items. Should I change it to a map?
|
|
||
| @Inject | ||
| TimeMeasurementManager timeMeasurementManager; | ||
| KubernetesTokenAuthFilter kubernetesTokenAuthFilter; |
There was a problem hiding this comment.
Why it's a field instead of a local variable?
|
|
||
| @Inject | ||
| public KubernetesJWTCallerPrincipalFactory( | ||
| @ConfigProperty(name = "dbaas.security.k8s.jwt.enabled") boolean isJwtEnabled, |
There was a problem hiding this comment.
Prefix "is" should be used only for methods that return boolean value. Parameters/variables should be without it, like "jwtEnabled".
| public class DbaasAdapterRESTClientFactory { | ||
| @Inject | ||
| @ConfigProperty(name = "dbaas.security.k8s.jwt.enabled") | ||
| boolean isJwtEnabled; |
There was a problem hiding this comment.
Prefix "is" should be used only for methods that return boolean value. Parameters/variables should be without it, like "jwtEnabled".
|
|
||
| Uni<ChallengeData> result = mechanism.getChallenge(context); | ||
|
|
||
| verify(jwtAuth, times(1)).getChallenge(any()); |
There was a problem hiding this comment.
In the previous tests you have checked that the opposite auth method was not called. Can we add such check in this and following tests also?
| Set<Class<? extends AuthenticationRequest>> result = mechanism.getCredentialTypes(); | ||
|
|
||
| assertNotNull(result); | ||
| assertTrue(result.contains(AuthenticationRequest.class)); |
There was a problem hiding this comment.
This is not sufficient for checking that "result" contains types from both auth methods.
| import static org.junit.jupiter.api.Assertions.*; | ||
|
|
||
| @ExtendWith(MockitoExtension.class) | ||
| class KubernetesJWTCallerPrincipalFactoryTest { |
There was a problem hiding this comment.
Need to add a test for correct operation of "isJwtEnabled" constructor parameter.
| private AuthFilterSelector authFilterSelector; | ||
|
|
||
| @Test | ||
| void shouldExecuteRequestWithBasicAuthWhenJwtDisabled() { |
There was a problem hiding this comment.
Here we not actually checked that request was performed with basic auth.
| import static com.github.tomakehurst.wiremock.client.WireMock.*; | ||
|
|
||
| @Slf4j | ||
| public class MockOidcTestResource implements QuarkusTestResourceLifecycleManager { |
There was a problem hiding this comment.
We shouldn't just copypaste part of existing Quarkus class. Please find a way to use OidcWiremockTestResource correctly.
| verify(restClient).close(); | ||
| } | ||
|
|
||
| private Response.StatusType getStatusType(int code) { |
There was a problem hiding this comment.
"code" parameter is not used, and 403 code is hardcoded.
|
|
||
| when(compositeNamespaceService.getBaselineByNamespace(defaultNamespace)).thenReturn(Optional.of(defaultBaseLine)); | ||
| when(compositeNamespaceService.getBaselineByNamespace(otherNamespaceInComposite)).thenReturn(Optional.of(defaultBaseLine)); | ||
| when(compositeNamespaceService.getBaselineByNamespace("someOtherNamespace")).thenReturn(Optional.empty()); |
There was a problem hiding this comment.
Why some of the strings are stored in fields, and some are in code? Let's move all of them to fields.
There was a problem hiding this comment.
I thought that this way it would be more clear that this is not a predefined namespace but "someOtherNamespace"
| ServiceAccountRolesAugmentor augmentor; | ||
|
|
||
| @BeforeEach | ||
| void setUp() { |
There was a problem hiding this comment.
Empty lifecycle methods are not needed.
| "Must be in format: <schema>://<service-name>.<namespace>:<port>, e.g.: http://dbaas-postgres-adapter.postgresql:8080"), | ||
| CORE_DBAAS_4046( | ||
| "CORE-DBAAS-4046", | ||
| "Invalid tenantId in classifier", |
There was a problem hiding this comment.
Error message should be more descriptive about what constitutes an invalid tenantId.
| "Invalid tenantId in classifier", | |
| "Tenant ID mismatch in classifier", |
#deepseek-review:inline
| "Adapter address name has wrong format", | ||
| "register request contains adapter address field, but it has wrong format: %s. " + | ||
| "Must be in format: <schema>://<service-name>.<namespace>:<port>, e.g.: http://dbaas-postgres-adapter.postgresql:8080"), | ||
| CORE_DBAAS_4046( |
There was a problem hiding this comment.
Error code CORE_DBAAS_4046 should follow the established naming pattern with consistent spacing.
| CORE_DBAAS_4046( | |
| CORE_DBAAS_4046( |
#deepseek-review:inline
| "register request contains adapter address field, but it has wrong format: %s. " + | ||
| "Must be in format: <schema>://<service-name>.<namespace>:<port>, e.g.: http://dbaas-postgres-adapter.postgresql:8080"), | ||
| CORE_DBAAS_4046( | ||
| "CORE-DBAAS-4046", |
There was a problem hiding this comment.
Error code string should be consistent with the pattern used in other error codes.
| "CORE-DBAAS-4046", | |
| "CORE-DBAAS-4046", |
#deepseek-review:inline
| CORE_DBAAS_4046( | ||
| "CORE-DBAAS-4046", | ||
| "Invalid tenantId in classifier", | ||
| "tenantId from classifier and tenantId from request don't match"), |
There was a problem hiding this comment.
Error description should be more specific about the nature of the mismatch.
| "tenantId from classifier and tenantId from request don't match"), | |
| "Tenant ID from classifier does not match tenant ID from request"), |
#deepseek-review:inline
| } | ||
|
|
||
| public static InvalidClassifierException withDefaultMsg(Map<String, Object> classifier) { | ||
| return new InvalidClassifierException("Classifier doesn't contain all mandatory fields. " + |
There was a problem hiding this comment.
The error message string concatenation is inefficient and hard to read; use a single string literal instead.
| return new InvalidClassifierException("Classifier doesn't contain all mandatory fields. " + | |
| return new InvalidClassifierException("Classifier doesn't contain all mandatory fields. If authenticating with token, namespace in classifier and in token must be equal or be in the same composite structure. Check that classifier has `microserviceName`, `scope`. If `scope` = `tenant`, classifier must contain `tenantId` property", |
#deepseek-review:inline
| import com.netcracker.cloud.core.error.runtime.ErrorCode; | ||
| import com.netcracker.cloud.core.error.runtime.ErrorCodeException; | ||
|
|
||
| public class ForbiddenException extends ErrorCodeException { |
There was a problem hiding this comment.
Consider adding a class-level Javadoc comment to explain the purpose and usage of this exception.
#deepseek-review:inline
| import com.netcracker.cloud.core.error.runtime.ErrorCodeException; | ||
|
|
||
| public class ForbiddenException extends ErrorCodeException { | ||
| public ForbiddenException(ErrorCode errorCode, String detail) { |
There was a problem hiding this comment.
Consider adding parameter validation to ensure errorCode and detail are not null.
| public ForbiddenException(ErrorCode errorCode, String detail) { | |
| public ForbiddenException(ErrorCode errorCode, String detail) { | |
| super(Objects.requireNonNull(errorCode, "errorCode must not be null"), Objects.requireNonNull(detail, "detail must not be null")); | |
| } |
#deepseek-review:inline
| throw new InvalidClassifierException("Classifier doesn't contain all mandatory fields. " + | ||
| "Check that classifier has `microserviceName`, `scope`. If `scope` = `tenant`, classifier must contain `tenantId` property", | ||
| externalDatabaseRequest.getClassifier(), Source.builder().pointer("/classifier").build()); | ||
| if (!AggregatedDatabaseAdministrationService.AggregatedDatabaseAdministrationUtils.isClassifierCorrect(externalDatabaseRequest.getClassifier()) || !namespaceValidator.checkNamespaceFromClassifier(securityContext, externalDatabaseRequest.getClassifier())) { |
There was a problem hiding this comment.
Split long conditional expression into separate statements for clarity.
#deepseek-review:inline
| if (!Objects.equals(classifier.get(SCOPE), SCOPE_VALUE_TENANT)) { | ||
| return; | ||
| } | ||
| String tenantId = ((TenantContextObject) ContextManager.get(BaseTenantProvider.TENANT_CONTEXT_NAME)).getTenant(); |
There was a problem hiding this comment.
Add null check for ContextManager.get result to prevent ClassCastException.
| String tenantId = ((TenantContextObject) ContextManager.get(BaseTenantProvider.TENANT_CONTEXT_NAME)).getTenant(); | |
| TenantContextObject tenantContext = (TenantContextObject) ContextManager.get(BaseTenantProvider.TENANT_CONTEXT_NAME); | |
| if (tenantContext == null || !tenantId.equals(classifier.get(TENANT_ID))) { |
#deepseek-review:inline
| createRequest.getClassifier(), Source.builder().pointer("/classifier").build()); | ||
| if (!AggregatedDatabaseAdministrationService.AggregatedDatabaseAdministrationUtils.isClassifierCorrect(createRequest.getClassifier()) || | ||
| !namespaceValidator.checkNamespaceFromClassifier(securityContext, createRequest.getClassifier())) { | ||
| throw InvalidClassifierException.withDefaultMsg(createRequest.getClassifier()); |
There was a problem hiding this comment.
Use specific exception messages instead of generic ones to help with debugging.
#deepseek-review:inline
| } | ||
|
|
||
| private void checkOriginService(UserRolesServices rolesServices) { | ||
| if (securityContext.getUserPrincipal() instanceof DefaultJWTCallerPrincipal principal) { |
There was a problem hiding this comment.
Add null check for securityContext.getUserPrincipal() to prevent potential NPE.
| if (securityContext.getUserPrincipal() instanceof DefaultJWTCallerPrincipal principal) { | |
| if (securityContext.getUserPrincipal() != null && securityContext.getUserPrincipal() instanceof DefaultJWTCallerPrincipal principal) { |
#deepseek-review:inline
| if (!Objects.equals(classifier.get(SCOPE), SCOPE_VALUE_TENANT)) { | ||
| return; | ||
| } | ||
| String tenantId = ((TenantContextObject) ContextManager.get(BaseTenantProvider.TENANT_CONTEXT_NAME)).getTenant(); |
There was a problem hiding this comment.
Extract tenant ID retrieval into a separate method for reusability.
#deepseek-review:inline
| @PathParam("type") String type) { | ||
| checkOriginService(classifierRequest); | ||
| if (!dBaaService.isValidClassifierV3(classifierRequest.getClassifier())) { | ||
| if (!dBaaService.isValidClassifierV3(classifierRequest.getClassifier()) || !namespaceValidator.checkNamespaceFromClassifier(securityContext, classifierRequest.getClassifier())) { |
There was a problem hiding this comment.
Split complex conditional into separate if statements for better maintainability.
#deepseek-review:inline
| throw new InvalidClassifierException("Classifier doesn't contain all mandatory fields. " + | ||
| "Check that classifier has `microserviceName`, `scope`. If `scope` = `tenant`, classifier must contain `tenantId` property", | ||
| createRequest.getClassifier(), Source.builder().pointer("/classifier").build()); | ||
| if (!AggregatedDatabaseAdministrationService.AggregatedDatabaseAdministrationUtils.isClassifierCorrect(createRequest.getClassifier()) || |
There was a problem hiding this comment.
Combine multiple validation conditions into separate if statements for better readability.
#deepseek-review:inline
| public interface AuthFilterSelector { | ||
| void selectAuthFilter(ClientRequestFilter authFilter); | ||
|
|
||
| ClientRequestFilter getAuthFilter(); |
There was a problem hiding this comment.
Consider adding nullability annotations to clarify if getAuthFilter() can return null.
#deepseek-review:inline
| import jakarta.ws.rs.client.ClientRequestFilter; | ||
|
|
||
| public interface AuthFilterSelector { | ||
| void selectAuthFilter(ClientRequestFilter authFilter); |
There was a problem hiding this comment.
Method name 'selectAuthFilter' suggests selection but implementation sets a single filter, consider renaming to 'setAuthFilter' for clarity.
| void selectAuthFilter(ClientRequestFilter authFilter); | |
| void setAuthFilter(ClientRequestFilter authFilter); |
#deepseek-review:inline
|
|
||
| @Override | ||
| public void filter(ClientRequestContext clientRequestContext) throws IOException { | ||
| clientRequestContext.getHeaders().add(HttpHeaders.AUTHORIZATION, "Bearer " + tokenSupplier.get()); |
There was a problem hiding this comment.
Add null check for tokenSupplier.get() to prevent potential NullPointerException when token is null.
| clientRequestContext.getHeaders().add(HttpHeaders.AUTHORIZATION, "Bearer " + tokenSupplier.get()); | |
| String token = tokenSupplier.get(); | |
| if (token != null) { | |
| clientRequestContext.getHeaders().add(HttpHeaders.AUTHORIZATION, "Bearer " + token); | |
| } |
#deepseek-review:inline
| @Inject | ||
| ServiceAccountRolesManager rolesManager; | ||
|
|
||
| public ServiceAccountRolesAugmentor(ServiceAccountRolesManager rolesManager) { |
There was a problem hiding this comment.
Constructor injection is redundant when field injection is already used.
#deepseek-review:inline
| Set<String> roles = rolesManager.getRolesByServiceAccountName(serviceName); | ||
|
|
||
| QuarkusSecurityIdentity.Builder builder = QuarkusSecurityIdentity.builder(identity); | ||
| if (roles != null && !roles.isEmpty()) { |
There was a problem hiding this comment.
Condition 'roles != null && !roles.isEmpty()' can be simplified to 'roles != null && !roles.isEmpty()' for consistency, but current logic is acceptable.
#deepseek-review:inline
| } | ||
|
|
||
| String principal = identity.getPrincipal().getName(); | ||
| String serviceName = principal.substring(principal.lastIndexOf(':') + 1); |
There was a problem hiding this comment.
Extracting service name from principal may fail if ':' is not present, causing StringIndexOutOfBoundsException.
#deepseek-review:inline
| if (roles != null && !roles.isEmpty()) { | ||
| builder.addRoles(roles); | ||
| } else { | ||
| builder.addRole(Constants.DB_CLIENT); |
There was a problem hiding this comment.
Adding a default role when no roles are found may mask configuration issues; consider logging a warning.
#deepseek-review:inline
|
|
||
| String principal = identity.getPrincipal().getName(); | ||
| String serviceName = principal.substring(principal.lastIndexOf(':') + 1); | ||
| Set<String> roles = rolesManager.getRolesByServiceAccountName(serviceName); |
There was a problem hiding this comment.
rolesManager.getRolesByServiceAccountName may return null; consider handling null explicitly for clarity.
#deepseek-review:inline
No description provided.