[provisioning-url] move matching to application layer

[deimi]

This way it would be possible to let every device join, no matter what kind of application layer provisioning is required later.

[deimi]

As far as I understand the code, the check for a correct provisioning url could also be completely deleted, because the provisioning url is anyway checked by the CommissioningHandler in the application code. So it makes no sense for me that the library is checking the url as well.

[wgtdkp]

Thanks! @deimi If the provisioning URL in mJoinerInfo is empty, it means the commissioner doesn't support vendor-specific provisioning for this device. So, if a joiner requires vendor-specific provisioning while the commissioner cannot handle it, the joiner should be rejected, which is defined behavior by Thread 1.1 specification.

I agree that it seems not necessary for the commissioner library to check the provisioning URL itself before delegating to application, while we made it there to just enforce that a commissioner app follows the specification. I am fine with moving the provisioning URL matching from commissioner library to application layer, but to compliant to Thread specification, we still should not accept a joiner if the provisioning URLs don't match at there. Thoughts?

[deimi]

@wgtdkp Ah ok, I understand your points for pre matching in the library.

Yes that would be my suggestion too. Remove the url matching from the library and let only decide the application via the CommissioningHandler if the provisioning url is accepted. The library should only check the return value of the CommissioningHandler. Nevertheless the library should still check if the joiner requests application layer provisioning, but the application has not set a CommissioningHandler. This is already in the code.

This way the library can also cover the use case where an application is capable of handling several different provisioning urls.

[wgtdkp]

@deimi

The library should only check the return value of the CommissioningHandler.

This totally works for me, and we don't need the mProvisioningURL field in the JoinerInfo class anymore. Could you make the change to that instead?

This way the library can also cover the use case where an application is capable of handling several different provisioning urls.

Please notice that the provisioning URL is per joiner. The application can specify different provisioning URL for each joiner, but don't support multiple provisioning URL of a single joiner device (Is there real use case of this?).

[deimi]

@wgtdkp

This totally works for me. Could you make the change to that instead?

:+1:

Please notice that the provisioning URL is per joiner. The application can specify different provisioning URL for each joiner, but don't support multiple provisioning URL of a single joiner device (Is there real use case of this?).

Especially during application development this could be handy. That's the reason why I stumbled across this, because I just used the cli app with joiner enableall meshcop XYZ :sweat_smile: Another situation which comes in my mind is that a joiner could run with different firmwares with different provisioning url configured. After this merge the matching lies in the hands of the application and can therefore handle different urls for the same joiner eui64.

[wgtdkp]

@deimi

joiner enableall meshcop XYZ

Enable a joiner without a provisioning URL is legal (but the joiner password is illegal), this means the commissioner doesn't support vendor-specific provisioning for this joiner.

Another situation which comes in my mind is that a joiner could run with different firmwares with different provisioning url configured. After this merge the matching lies in the hands of the application and can therefore handle different urls for the same joiner eui64.

For real product, I think the provisioning URL is not going to be changed but set when created from the factory...

[deimi]

@wgtdkp

For real product, I think the provisioning URL is not going to be changed but set when created from the factory...

Probably not, I was just thinking about a bug during update or something else. Nevertheless now this would be up to the application layer to handle this.

[codecov-io]

Codecov Report

Merging #73 into master will increase coverage by 0.01%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #73      +/-   ##
==========================================
+ Coverage   60.32%   60.34%   +0.01%     
==========================================
  Files          52       52              
  Lines        5379     5379              
==========================================
+ Hits         3245     3246       +1     
+ Misses       2134     2133       -1     

Impacted Files	Coverage Δ
src/library/commissioning_session.cpp	87.75% <100.00%> (+1.02%)	:arrow_up:

[deimi]

@wgtdkp please have a look