feat: container temporary credentials#5965
Conversation
MMaiero
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a temporary identity service feature for container authentication that allows containers to be provisioned with temporary credentials for accessing Kura REST APIs, enhancing security by eliminating the need for persistent credentials.
- Implements
TemporaryIdentityServiceAPI and core functionality for managing in-memory temporary identities - Adds container identity integration to automatically provision temporary credentials for containers
- Introduces REST authentication provider for validating temporary tokens
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| kura/org.eclipse.kura.api/src/main/java/org/eclipse/kura/identity/TemporaryIdentityService.java | Defines the new API interface for temporary identity management |
| kura/org.eclipse.kura.core.identity/src/main/java/org/eclipse/kura/core/identity/IdentityServiceImpl.java | Implements temporary identity service functionality within the existing identity service |
| kura/org.eclipse.kura.core.identity/src/main/java/org/eclipse/kura/core/identity/TemporaryIdentity.java | Data model for temporary identity objects with permissions and tokens |
| kura/org.eclipse.kura.rest.provider/src/main/java/org/eclipse/kura/internal/rest/auth/TemporaryTokenAuthenticationProvider.java | REST authentication provider for validating temporary tokens |
| kura/org.eclipse.kura.rest.provider/src/main/java/org/eclipse/kura/internal/rest/provider/AuthenticationFilter.java | Updates authentication filter to support temporary token validation |
| kura/org.eclipse.kura.container.provider/src/main/java/org/eclipse/kura/container/provider/ContainerInstance.java | Container integration to create and inject temporary credentials |
| kura/org.eclipse.kura.container.provider/src/main/java/org/eclipse/kura/container/provider/ContainerInstanceOptions.java | Configuration options for container identity integration |
| kura/test/org.eclipse.kura.core.identity.test/src/main/java/org/eclipse/kura/core/identity/test/TemporaryIdentityServiceTest.java | Comprehensive test suite for temporary identity functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // Check if this is a temporary token principal by examining the class name | ||
| if (requestUser.getClass().getSimpleName().contains("TemporaryToken")) { | ||
| return isTemporaryUserInRole(requestUser, role); | ||
| } |
There was a problem hiding this comment.
Using class name comparison is fragile and error-prone. Consider using instanceof check or implementing a marker interface/method to identify temporary token principals instead of string comparison on class name.
| // Get the token using reflection | ||
| final String token = (String) principal.getClass().getMethod("getToken").invoke(principal); |
There was a problem hiding this comment.
Using reflection to access the token is fragile and can fail at runtime. Consider defining an interface with a getToken() method that the TemporaryTokenPrincipal can implement, allowing for type-safe access.
| .setDeviceList(baseConfig.getContainerDevices()) | ||
| .setFrameworkManaged(baseConfig.isFrameworkManaged()) | ||
| .setLoggingType(baseConfig.getContainerLoggingType()) | ||
| .setContainerNetowrkConfiguration(baseConfig.getContainerNetworkConfiguration()) |
There was a problem hiding this comment.
There is a typo in the method name. It should be "setContainerNetworkConfiguration" instead of "setContainerNetowrkConfiguration".
| .setContainerNetowrkConfiguration(baseConfig.getContainerNetworkConfiguration()) | |
| .setContainerNetworkConfiguration(baseConfig.getContainerNetworkConfiguration()) |
MMaiero
force-pushed
the
feature/container-temporary-credentials
branch
from
ff7fd30 to
ac65a92
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 22 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private void createTemporaryIdentityIfEnabled(final ContainerInstanceOptions options) { | ||
| if (options.isIdentityIntegrationEnabled() && this.temporaryIdentityService != null) { | ||
| try { | ||
| cleanupTemporaryIdentity(); | ||
|
|
||
| final Set<Permission> permissions = options.getContainerPermissions().stream() | ||
| .map(Permission::new) | ||
| .collect(Collectors.toSet()); | ||
|
|
||
| final String identityName = "container_" + options.getContainerName().replace("-", "_"); | ||
| this.currentTemporaryToken.set( | ||
| this.temporaryIdentityService.createTemporaryIdentity(identityName, permissions)); | ||
|
|
||
| logger.info("Created temporary identity for container {} with {} permissions", | ||
| options.getContainerName(), permissions.size()); | ||
|
|
||
| } catch (KuraException e) { | ||
| logger.error("Failed to create temporary identity for container {}", options.getContainerName(), e); | ||
| this.currentTemporaryToken.set(null); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing Javadoc documentation for the createTemporaryIdentityIfEnabled() method. Private helper methods that encapsulate complex logic should include documentation explaining their purpose, especially when they handle important features like identity management.
| private boolean isTemporaryUserInRole(final TokenHolder tokenHolder, final String role) { | ||
| if (this.temporaryIdentityService == null) { | ||
| return false; | ||
| } | ||
|
|
||
| try { | ||
| final String token = tokenHolder.getToken(); | ||
|
|
||
| if (token == null) { | ||
| return false; | ||
| } | ||
|
|
||
| final Permission permission = new Permission("rest." + role); | ||
| this.temporaryIdentityService.checkTemporaryPermission(token, permission); | ||
| return true; | ||
| } catch (final Exception e) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing Javadoc documentation for the isTemporaryUserInRole() method. Private helper methods that implement authorization logic should include documentation explaining their purpose, parameters, and return values.
| public TemporaryIdentity(String name, Set<Permission> permissions, String token) { | ||
| this.name = Objects.requireNonNull(name, "name cannot be null"); | ||
| this.permissions = Objects.requireNonNull(permissions, "permissions cannot be null"); | ||
| this.token = Objects.requireNonNull(token, "token cannot be null"); | ||
| this.creationTime = System.currentTimeMillis(); |
There was a problem hiding this comment.
The name and token fields are used in field assignments but are not prefixed with this.. Based on the codebase convention, these should be this.name = Objects.requireNonNull(name, "name cannot be null"); and similar for other fields for consistency.
| this.token = Objects.requireNonNull(token, "token cannot be null"); | ||
| this.creationTime = System.currentTimeMillis(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing Javadoc documentation for the getName() getter method. Public API methods in this codebase should have Javadoc comments describing their purpose and return values.
| /** | |
| * Returns the name associated with this temporary identity. | |
| * | |
| * @return the identity name | |
| */ |
| <AD id="container.identity.enabled" name="Enable Identity Integration" type="Boolean" cardinality="1" required="true" default="false" | ||
| description="Enable integration with Kura Identity Service to provide temporary credentials to the container. Default: false"> | ||
| </AD> | ||
|
|
There was a problem hiding this comment.
Trailing whitespace on this line. This should be removed to maintain code cleanliness and consistency with the project's formatting standards.
| createTemporaryIdentityIfEnabled(options); | ||
| final ContainerConfiguration containerConfiguration = getContainerConfigurationWithCredentials(options); |
There was a problem hiding this comment.
Potential race condition: The createTemporaryIdentityIfEnabled() method calls cleanupTemporaryIdentity() at the start, and getContainerConfigurationWithCredentials() reads this.currentTemporaryToken.get() immediately after. If these are called from different threads or if there's any interruption between calls, the token might be null when building the configuration. Consider ensuring thread safety or documenting the expected calling context.
| public TemporaryTokenPrincipal(String name, String token) { | ||
| this.name = name; | ||
| this.token = token; |
There was a problem hiding this comment.
The name and token fields are used in field assignments but are not prefixed with this.. Based on the codebase convention, these should be this.name = name; and this.token = token; for consistency with other similar code patterns in the repository.
|
|
||
| public Set<Permission> getPermissions() { | ||
| return permissions; | ||
| } | ||
|
|
||
| public long getCreationTime() { | ||
| return creationTime; | ||
| } | ||
|
|
||
| public String getToken() { | ||
| return token; | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing Javadoc documentation for the getPermissions(), getCreationTime(), getToken(), and hasPermission() methods. Public methods in this codebase should have Javadoc comments describing their purpose, parameters, and return values.
| public Set<Permission> getPermissions() { | |
| return permissions; | |
| } | |
| public long getCreationTime() { | |
| return creationTime; | |
| } | |
| public String getToken() { | |
| return token; | |
| } | |
| /** | |
| * Returns the set of permissions associated with this identity. | |
| * | |
| * @return the set of permissions | |
| */ | |
| public Set<Permission> getPermissions() { | |
| return permissions; | |
| } | |
| /** | |
| * Returns the creation time of this identity in milliseconds since the epoch. | |
| * | |
| * @return the creation time in milliseconds | |
| */ | |
| public long getCreationTime() { | |
| return creationTime; | |
| } | |
| /** | |
| * Returns the token associated with this identity. | |
| * | |
| * @return the identity token | |
| */ | |
| public String getToken() { | |
| return token; | |
| } | |
| /** | |
| * Checks if this identity has the specified permission. | |
| * | |
| * @param permission the permission to check | |
| * @return {@code true} if the identity has the permission, {@code false} otherwise | |
| */ |
|
|
||
| private ContainerConfiguration getContainerConfigurationWithCredentials(final ContainerInstanceOptions options) { | ||
| ContainerConfiguration baseConfig = options.getContainerConfiguration(); | ||
|
|
There was a problem hiding this comment.
Trailing whitespace on this line. This should be removed to maintain code cleanliness and consistency with the project's formatting standards.
MMaiero
force-pushed
the
feature/container-temporary-credentials
branch
from
916bfb5 to
2ddf154
Compare
MMaiero
force-pushed
the
feature/container-temporary-credentials
branch
from
9c4e805 to
928bb18
Compare
|
To verify and validate, via Kura create a new container with the feature for passing credentials enabled and the right permissions set. For example the rest.inventory. Then try to interact with Kura REST via the container just started for example using: |
| * @return a temporary authentication token that can be used to authenticate as this identity. | ||
| * @throws KuraException if a failure occurs in creating the temporary identity. | ||
| */ | ||
| public String createTemporaryIdentity(final String identityName, final Set<Permission> permissions) throws KuraException; |
There was a problem hiding this comment.
I would remove 'Temporary' from the names of all methods. 'Temporary' is implied by the service name.
|
This PR introduces a new concept of temporary identity and a new token based authentication mechanism, in my opinion the two topics are orthogonal. In my opinion the only difference between regular and temporary identities should be that the latter are not persisted in the snapshot and have a fixed lifetime period, but I would expect them to behave like the regular ones for all other aspects (for example being able to use the existing auth methods for them). We could for example create a separate in-memory store for them that contains the same informations that we store in the UserAdmin that is used by the IdentityService in addition to the UserAdmin. A blocker for implementing this approach is the fact that the REST service is currently using the UserAdmin directly for authentication, and has not ported to use the new IdentityService yet. We could also implement a new token based authentication method that works the same both for regular and temporary identities, here we could use a standard solution like JWT. I would proceed with the following steps, maybe with separate contributions that can be probably done in parallel:
|
But would make sense then to use password based auth or certificate auth from the containers? |
MMaiero
force-pushed
the
feature/container-temporary-credentials
branch
from
1844d56 to
19e6427
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 51 out of 51 changed files in this pull request and generated 13 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Represents a temporary identity that exists only in memory. | ||
| * Used for container authentication without persistent storage. | ||
| */ | ||
| public class TemporaryIdentity { | ||
|
|
||
| private final String name; | ||
| private final Set<Permission> permissions; | ||
| private final long creationTime; | ||
| private final String token; | ||
|
|
||
| public TemporaryIdentity(String name, Set<Permission> permissions, String token) { | ||
| this.name = Objects.requireNonNull(name, "name cannot be null"); | ||
| this.permissions = Objects.requireNonNull(permissions, "permissions cannot be null"); | ||
| this.token = Objects.requireNonNull(token, "token cannot be null"); | ||
| this.creationTime = System.currentTimeMillis(); | ||
| } | ||
|
|
||
| public String getName() { | ||
| return name; | ||
| } | ||
|
|
||
| public Set<Permission> getPermissions() { | ||
| return permissions; | ||
| } | ||
|
|
||
| public long getCreationTime() { | ||
| return creationTime; | ||
| } | ||
|
|
||
| public String getToken() { | ||
| return token; | ||
| } | ||
|
|
||
| public boolean hasPermission(Permission permission) { | ||
| return permissions.contains(permission); | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(name, token); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object obj) { | ||
| if (this == obj) { | ||
| return true; | ||
| } | ||
| if (!(obj instanceof TemporaryIdentity)) { | ||
| return false; | ||
| } | ||
| TemporaryIdentity other = (TemporaryIdentity) obj; | ||
| return Objects.equals(name, other.name) && Objects.equals(token, other.token); | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return "TemporaryIdentity{" + "name='" + name + '\'' + ", permissions=" + permissions.size() + ", creationTime=" | ||
| + creationTime + '}'; | ||
| } |
There was a problem hiding this comment.
The TemporaryIdentity class is defined but never used in the implementation. The temporary identity functionality uses IdentityConfiguration directly instead. This class should either be removed or integrated into the design if it serves a purpose.
| * Represents a temporary identity that exists only in memory. | |
| * Used for container authentication without persistent storage. | |
| */ | |
| public class TemporaryIdentity { | |
| private final String name; | |
| private final Set<Permission> permissions; | |
| private final long creationTime; | |
| private final String token; | |
| public TemporaryIdentity(String name, Set<Permission> permissions, String token) { | |
| this.name = Objects.requireNonNull(name, "name cannot be null"); | |
| this.permissions = Objects.requireNonNull(permissions, "permissions cannot be null"); | |
| this.token = Objects.requireNonNull(token, "token cannot be null"); | |
| this.creationTime = System.currentTimeMillis(); | |
| } | |
| public String getName() { | |
| return name; | |
| } | |
| public Set<Permission> getPermissions() { | |
| return permissions; | |
| } | |
| public long getCreationTime() { | |
| return creationTime; | |
| } | |
| public String getToken() { | |
| return token; | |
| } | |
| public boolean hasPermission(Permission permission) { | |
| return permissions.contains(permission); | |
| } | |
| @Override | |
| public int hashCode() { | |
| return Objects.hash(name, token); | |
| } | |
| @Override | |
| public boolean equals(Object obj) { | |
| if (this == obj) { | |
| return true; | |
| } | |
| if (!(obj instanceof TemporaryIdentity)) { | |
| return false; | |
| } | |
| TemporaryIdentity other = (TemporaryIdentity) obj; | |
| return Objects.equals(name, other.name) && Objects.equals(token, other.token); | |
| } | |
| @Override | |
| public String toString() { | |
| return "TemporaryIdentity{" + "name='" + name + '\'' + ", permissions=" + permissions.size() + ", creationTime=" | |
| + creationTime + '}'; | |
| } | |
| * @deprecated This class is no longer used. Temporary identity functionality | |
| * relies on {@code IdentityConfiguration} directly. This type is | |
| * retained only for backwards compatibility and should not be used | |
| * in new code. | |
| */ | |
| @Deprecated | |
| public final class TemporaryIdentity { | |
| private TemporaryIdentity() { | |
| throw new AssertionError("TemporaryIdentity is deprecated and should not be used"); | |
| } |
| // Create temporary identity with very long lifetime (365 days) | ||
| // The identity lifetime matches the container lifecycle - cleanup happens when container stops | ||
| // The duration is a safety net for cases where cleanup fails | ||
| ContainerInstance.this.identityService.createTemporaryIdentity(identityName, configuration, | ||
| Duration.ofDays(365)); |
There was a problem hiding this comment.
The duration of 365 days for temporary identities is extremely long for something called "temporary". This undermines the security benefit of temporary credentials. Consider using a much shorter default lifetime (e.g., hours or days) or making this configurable. The comment mentions it's a "safety net for cases where cleanup fails", but this is too long and increases the attack surface if credentials are compromised.
| } catch (KuraException e) { | ||
| logger.error("Failed to create temporary identity for container {}", options.getContainerName(), e); | ||
| ContainerInstance.this.currentTemporaryIdentityName.set(null); | ||
| ContainerInstance.this.currentTemporaryToken.set(null); | ||
| } |
There was a problem hiding this comment.
The method catches all exceptions and sets both identity name and token to null. This could hide legitimate errors during identity creation. Consider logging the exception at error level before nulling out the values, or re-throwing certain types of exceptions that shouldn't be silently caught.
| private boolean isSamePassword(final String username, final String newPassword) { | ||
| try { | ||
| final Optional<PasswordHash> existingHash = this.identityHelper.getPasswordHash(username); | ||
| if (!existingHash.isPresent()) { | ||
| return false; | ||
| } | ||
|
|
||
| final PasswordHash newHash = this.identityHelper.computePasswordHash(newPassword.toCharArray()); | ||
| return existingHash.get().equals(newHash); | ||
| } catch (final Exception e) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
The method isSamePassword compares password hashes by computing a new hash of the new password and comparing it with the existing hash. However, password hashes typically include a salt, and calling computePasswordHash with the same password twice will generate different hashes due to different salts. This comparison will always return false, making the check ineffective. The proper way is to verify the password against the existing hash using the password verification functionality.
| logger.info("Created temporary identity {} for container {} with {} permissions", | ||
| identityName, options.getContainerName(), permissions.size()); |
There was a problem hiding this comment.
Security concern: The temporary token is logged in plain text. Consider masking or redacting the token value in log messages to prevent exposure of credentials in log files.
| .setDeviceList(baseConfig.getContainerDevices()) | ||
| .setFrameworkManaged(baseConfig.isFrameworkManaged()) | ||
| .setLoggingType(baseConfig.getContainerLoggingType()) | ||
| .setContainerNetowrkConfiguration(baseConfig.getContainerNetworkConfiguration()) |
There was a problem hiding this comment.
Typo in method name: setContainerNetowrkConfiguration should be setContainerNetworkConfiguration (missing 'r' in Network).
| .setContainerNetowrkConfiguration(baseConfig.getContainerNetworkConfiguration()) | |
| .setContainerNetworkConfiguration(baseConfig.getContainerNetworkConfiguration()) |
| .map(Permission::new) | ||
| .collect(Collectors.toSet()); | ||
|
|
||
| final String identityName = "container_" + options.getContainerName().replace("-", "_"); |
There was a problem hiding this comment.
Missing validation: The container name is converted from kebab-case to snake_case to create the identity name, but there's no validation that the resulting name is valid. If the container name contains special characters that aren't hyphens, the identity name could be invalid. Consider adding validation or sanitization of the identity name.
| final String identityName = "container_" + options.getContainerName().replace("-", "_"); | |
| final String rawContainerName = options.getContainerName(); | |
| String sanitizedContainerName = rawContainerName == null ? "" : rawContainerName; | |
| // Replace any character that is not a letter, digit or hyphen with underscore | |
| sanitizedContainerName = sanitizedContainerName.replaceAll("[^A-Za-z0-9\\-]", "_"); | |
| // Preserve existing behavior of converting hyphens to underscores | |
| sanitizedContainerName = sanitizedContainerName.replace("-", "_"); | |
| // Ensure we don't end up with an empty identity suffix | |
| if (sanitizedContainerName.isEmpty()) { | |
| sanitizedContainerName = "default"; | |
| } | |
| final String identityName = "container_" + sanitizedContainerName; |
|
Password authentication currently does not work in the case when the user provides the password using the PasswordConfiguration.newPassword field In this case the identity service should compute the hash of the given password, and store that one in the passwordHash field, the newPassword fileld should not be populated in the Part of the logic performed by the Consider also further unifying the identity configuration validation logic |
| * with the given name already exists (either regular or temporary). | ||
| * @since 2.8.0 | ||
| */ | ||
| public void createTemporaryIdentity(final String identityName, final IdentityConfiguration configuration, |
There was a problem hiding this comment.
The identityName argument is redundant since it is included in IdentityConfiguration, for symmetry with createIdentity() we could remove the configuration argument
|
The implementation of the IdentityService is becoming more and more complicated since it contains different code paths for handling reguar and temporary identities. A possible approach for simplifying it could be creating a |
| * or if deletion fails. | ||
| * @since 2.8.0 | ||
| */ | ||
| public boolean deleteTemporaryIdentity(final String identityName) throws KuraException; |
There was a problem hiding this comment.
Is it necessary to have a dedicated deleteTemporaryIdentity method or is it enough to extend the existing deleteIdentity method?
MMaiero
force-pushed
the
feature/container-temporary-credentials
branch
from
adfa91d to
a5b4020
Compare
|
Signed-off-by: MMaiero <matteo.maiero@eurotech.com>
Signed-off-by: MMaiero <matteo.maiero@eurotech.com>
Signed-off-by: MMaiero <matteo.maiero@eurotech.com>
Signed-off-by: Nicola Timeus <nicola.timeus@eurotech.com>
Signed-off-by: MMaiero <matteo.maiero@eurotech.com>
….eclipse.kura.container.provider.ContainerInstance.xml Co-authored-by: nicolatimeus <nicola.timeus@eurotech.com>
MMaiero
force-pushed
the
feature/container-temporary-credentials
branch
from
b1c8a0c to
cda9d08
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 38 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| final IdentityConfiguration configuration = new IdentityConfiguration(candidateName, | ||
| Arrays.asList(passwordConfiguration, assignedPermissions)); | ||
|
|
||
| ContainerInstance.this.identityService.createTemporaryIdentity(candidateName, Duration.ofDays(365)); | ||
| ContainerInstance.this.identityService.updateIdentityConfiguration(configuration); |
There was a problem hiding this comment.
The temporary identity lifetime is hard-coded to Duration.ofDays(365). If Kura crashes or the cleanup path doesn’t run, the credentials can remain valid far longer than intended. Consider making the lifetime configurable and/or using a short default tied to container lifecycle (with renewal if needed).
| private void givenExistingIdentity() { | ||
| try { | ||
| this.identityService.createIdentity(this.identityName); | ||
| this.createdIdentities.add(this.identityName); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
givenExistingIdentity ignores the boolean return value from createIdentity. If the identity already exists, it still gets added to createdIdentities and will be deleted in @After, which can remove pre-existing identities. Only track identities for cleanup when createIdentity(...) returns true.
| configuration.update(configurationProperties); | ||
|
|
||
| } catch (Exception e) { | ||
| fail("cannot update configuration"); | ||
| } |
There was a problem hiding this comment.
The catch-all in givenConfiguration drops the underlying exception details, which makes test failures harder to debug. Include the exception message (or chain it) in the fail(...) message.
| * if the provided permissio is not currently assigned to | ||
| * the given identity or if occurs while performing the |
There was a problem hiding this comment.
Typo in Javadoc: "permissio" → "permission".
| * if the provided permissio is not currently assigned to | |
| * the given identity or if occurs while performing the | |
| * if the provided permission is not currently assigned to | |
| * the given identity or if a failure occurs while performing the |
| if (options.isIdentityIntegrationEnabled() && password != null && identityName != null) { | ||
| final List<String> envVars = new ArrayList<>(baseConfig.getContainerEnvVars()); | ||
| envVars.add("KURA_IDENTITY_NAME=" + identityName); | ||
| envVars.add("KURA_IDENTITY_PASSWORD=" + new String(password)); |
There was a problem hiding this comment.
KURA_IDENTITY_PASSWORD is built from new String(password) which creates an extra immutable String copy of the secret in heap memory (cannot be wiped). Since you already keep the password as char[], consider only converting to String at the last possible moment (and avoid redundant copies), or pass it through in a way that minimizes additional String allocations.
| envVars.add("KURA_IDENTITY_PASSWORD=" + new String(password)); | |
| final StringBuilder identityPasswordEnv = new StringBuilder("KURA_IDENTITY_PASSWORD="); | |
| identityPasswordEnv.append(password); | |
| envVars.add(identityPasswordEnv.toString()); |
| private void givenExistingPermissions(final String... permissionNames) { | ||
| for (String permissionName : permissionNames) { | ||
| try { | ||
| this.identityService.createPermission(new Permission(permissionName)); | ||
| this.createdPermissions.add(permissionName); |
There was a problem hiding this comment.
givenExistingPermissions unconditionally adds permissions to createdPermissions even when createPermission returns false (already existed). The @After cleanup would then delete permissions that pre-existed, potentially breaking other tests/runs. Only add to createdPermissions when createPermission(...) returns true.
| private void givenTemporaryIdentity(final String name) { | ||
| callVoid(() -> { | ||
| this.identityService.createTemporaryIdentity(name, Duration.ofHours(1)); | ||
| return null; | ||
| }); |
There was a problem hiding this comment.
givenTemporaryIdentity uses callVoid, which records exceptions but doesn’t fail the test immediately. If temporary identity creation fails, the failure can be overwritten by the next call(...), allowing the test to pass with an invalid setup. Prefer doing this setup in a try/catch that calls fail(...) on error, or assert thenNoExceptionIsThrown() right after this setup step.
| for (int i = 0; i < 100; i++) { | ||
| final Response response = restTransport.runRequest("/ping", new MethodSpec("GET")); | ||
|
|
||
| if (response.getStatus() != 404 && response.getStatus() != 503) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
The service-startup wait loop breaks when the endpoint becomes available, but if it never becomes available the test proceeds without failing here (making failures later less diagnosable/flakier). Track whether startup succeeded and fail(...) after the loop if it didn’t.
Signed-off-by: Nicola Timeus <nicola.timeus@eurotech.com>
Signed-off-by: Nicola Timeus <nicola.timeus@eurotech.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 39 changed files in this pull request and generated 20 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| final User user = (User) this.userAdmin.getRole(KURA_USER_PREFIX + requestUser.getName()); | ||
|
|
||
| return containsBasicMember(this.userAdmin.getRole(KURA_PERMISSION_REST_PREFIX + role), user) | ||
| || containsBasicMember(this.userAdmin.getRole(KURA_PERMISSION_PREFIX + "kura.admin"), user); | ||
|
|
||
| this.identityService.checkPermission(requestUser.getName(), new Permission("rest." + role)); | ||
| return true; | ||
| } catch (final Exception e) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| private static boolean containsBasicMember(final Role group, final User user) { | ||
| if (!(group instanceof Group)) { | ||
| return false; | ||
| } | ||
|
|
||
| final Group asGroup = (Group) group; | ||
|
|
||
| final Role[] members = asGroup.getMembers(); | ||
|
|
||
| if (members == null) { | ||
| return false; | ||
| } | ||
|
|
||
| for (final Role member : members) { | ||
| if (member.getName().equals(user.getName())) { | ||
| try { | ||
| this.identityService.checkPermission(requestUser.getName(), new Permission(KURA_ADMIN_PERMISSION)); | ||
| return true; | ||
| } catch (final Exception ignored) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
The nested try-catch blocks on lines 130-140 for checking permissions have several issues. First, catching all Exceptions (line 133 and 137) is too broad and could hide unexpected errors. Second, the kura.admin permission check on line 135 is performed as a fallback for every permission check failure, which could mask genuine authorization issues and impact performance. Third, the exception 'ignored' on line 137 means authorization failures are silently suppressed. Consider logging at debug level when permission checks fail, and being more specific about which exceptions to catch (e.g., KuraException).
| @@ -1,17 +1,20 @@ | |||
| /******************************************************************************* | |||
There was a problem hiding this comment.
The PR description is incomplete and lacks important details. It only contains the template structure without filling in the actual information about the changes, related issues, solution description, manual tests, or side notes. This makes it difficult to understand the context and scope of the changes without inspecting the code.
…ipse/kura/container/provider/ContainerInstance.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ity/IdentityService.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ity/IdentityService.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Brief description of the PR. [e.g. Added
nullcheck onobjectto avoidNullPointerException]Related Issue: This PR fixes/closes {issue number}
Description of the solution adopted: A more detailed description of the changes made to solve/close one or more issues. If the PR is simple and easy to understand this section can be skipped
Screenshots: If applicable, add screenshots to help explain your solution
Manual Tests: Optional description of the tests performed to check correct functioning of changes, useful for an efficient review
Any side note on the changes made: Description of any other change that has been made, which is not directly linked to the issue resolution [e.g. Code clean up/Sonar issue resolution]