[#3608] Verify that auto-provisioned device ids match device id pattern #3641
Conversation
harism
force-pushed
the
auto_provision_device_id_checks
branch
11 times, most recently
from
9b85c1d
to
c76d2ad
Compare
harism
force-pushed
the
auto_provision_device_id_checks
branch
from
c76d2ad
to
c3ce2da
Compare
|
I added integration test for If not I will continue writing per adapter integration tests in a similar fashion too. |
| @@ -276,6 +284,15 @@ public final Future<OperationResult<Id>> createDevice( | |||
| Objects.requireNonNull(span); | |||
|
|
|||
| final String deviceIdValue = deviceId.orElseGet(() -> generateDeviceId(tenantId)); | |||
| final Matcher matcher = Objects.requireNonNull(serviceConfig.getDeviceIdPattern()).matcher(deviceIdValue); | |||
There was a problem hiding this comment.
getDeviceIdPattern() will never return null
| return Future.failedFuture(new ClientErrorException( | ||
| tenantId, | ||
| HttpURLConnection.HTTP_BAD_REQUEST, | ||
| "invalid device id \"" + deviceIdValue + "\"" |
There was a problem hiding this comment.
We do not use string concatenation like that. Can you replace this with e.g.
"invalid device ID: %s".formatted(deviceIdValue);| final ServiceConfigProperties serviceConfig = mock(ServiceConfigProperties.class); | ||
| when(serviceConfig.getDeviceIdPattern()) | ||
| .thenReturn(Pattern.compile(RegistryManagementConstants.DEFAULT_REGEX_DEVICE_ID)); |
There was a problem hiding this comment.
no need to mock ServiceConfigProperties. Simply instantiate it and set the pattern as required.
| @Inject | ||
| MongoDbBasedHttpServiceConfigOptions mongoDbBasedHttpServiceConfigOptions; | ||
|
|
There was a problem hiding this comment.
Now that these options are needed in multiple places, I'd rather have a corresponding producer in org.eclipse.hono.deviceregistry.mongodb.app.ConfigPropertiesProducer and adapt HttpServerFactory accordingly. You can then add a corresponding parameter to this class' deviceManagementService() method.
There was a problem hiding this comment.
I believe you can do the same for the JDBC registry correspondingly.
There was a problem hiding this comment.
Ok, I get back to this a bit later on better time.
There was a problem hiding this comment.
Done, hope I understood it correctly what you were requesting to change.
harism
force-pushed
the
auto_provision_device_id_checks
branch
6 times, most recently
from
018ffd6
to
a6bfae7
Compare
harism
force-pushed
the
auto_provision_device_id_checks
branch
from
a6bfae7
to
5be9237
Compare
This is initial implementation idea I had and thought it should cover all the paths device is created to prevent creating any deivce with id which does not conform to the device id pattern.
I think I need to look at integration tests also but need to do that bit later.