-
Notifications
You must be signed in to change notification settings - Fork 5
feat: kubernetes tokens #147
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be directly returned without saving to variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
| throw new InvalidClassifierException("Invalid V3 classifier", classifierRequest.getClassifier(), Source.builder().pointer("").build()); | ||
| } | ||
| checkTenantId(classifierRequest.getClassifier()); | ||
| checkOriginService(classifierRequest); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the duplicated check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's split these two checks and throw different exception messages for them (in all similar cases).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be in the TMF format (com.netcracker.cloud.dbaas.controller.error.Utils).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
| } | ||
|
|
||
| String principal = identity.getPrincipal().getName(); | ||
| String serviceName = principal.substring(principal.lastIndexOf(':') + 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add Javadoc or something to describe in which format we're expecting the "principal" structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
| @NoArgsConstructor | ||
| @Slf4j | ||
| public class ServiceAccountRolesManager { | ||
| private final ArrayList<ServiceAccountWithRoles> serviceAccountsWithRoles = new ArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
|
||
| @Inject | ||
| TimeMeasurementManager timeMeasurementManager; | ||
| KubernetesTokenAuthFilter kubernetesTokenAuthFilter; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why it's a field instead of a local variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
|
||
| @Inject | ||
| public KubernetesJWTCallerPrincipalFactory( | ||
| @ConfigProperty(name = "dbaas.security.k8s.jwt.enabled") boolean isJwtEnabled, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefix "is" should be used only for methods that return boolean value. Parameters/variables should be without it, like "jwtEnabled".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in other places too
| public class DbaasAdapterRESTClientFactory { | ||
| @Inject | ||
| @ConfigProperty(name = "dbaas.security.k8s.jwt.enabled") | ||
| boolean isJwtEnabled; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefix "is" should be used only for methods that return boolean value. Parameters/variables should be without it, like "jwtEnabled".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
|
||
| Uni<ChallengeData> result = mechanism.getChallenge(context); | ||
|
|
||
| verify(jwtAuth, times(1)).getChallenge(any()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will try
| Set<Class<? extends AuthenticationRequest>> result = mechanism.getCredentialTypes(); | ||
|
|
||
| assertNotNull(result); | ||
| assertTrue(result.contains(AuthenticationRequest.class)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not sufficient for checking that "result" contains types from both auth methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
| import static org.junit.jupiter.api.Assertions.*; | ||
|
|
||
| @ExtendWith(MockitoExtension.class) | ||
| class KubernetesJWTCallerPrincipalFactoryTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to add a test for correct operation of "isJwtEnabled" constructor parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
| private AuthFilterSelector authFilterSelector; | ||
|
|
||
| @Test | ||
| void shouldExecuteRequestWithBasicAuthWhenJwtDisabled() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we not actually checked that request was performed with basic auth.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
| import static com.github.tomakehurst.wiremock.client.WireMock.*; | ||
|
|
||
| @Slf4j | ||
| public class MockOidcTestResource implements QuarkusTestResourceLifecycleManager { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"code" parameter is not used, and 403 code is hardcoded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
|
||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that this way it would be more clear that this is not a predefined namespace but "someOtherNamespace"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
| ServiceAccountRolesAugmentor augmentor; | ||
|
|
||
| @BeforeEach | ||
| void setUp() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty lifecycle methods are not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
| "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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rolesManager.getRolesByServiceAccountName may return null; consider handling null explicitly for clarity.
#deepseek-review:inline
No description provided.