Suggested changes for Remote Bolus/Carbs #5717
Conversation
| @@ -1,4 +1,4 @@ | |||
| //'use strict'; | |||
| 'use strict'; | |||
There was a problem hiding this comment.
@josep1972 why was this commented out?
There was a problem hiding this comment.
Hm...to be honest I don't remember...I agree that this should be changed. Apologies for the typo.
|
Thanks @jasoncalabrese, I agree. Plugins shouldn't do anything unless they become enabled, especially for features that require additional configuration or change liability and risk created by using or publishing the software. |
|
I am not sure I agree @jasoncalabrese that loop needs to do anything other than have an OTP shared with the remote provider. For temp overrides, the plugin is available even if overridePresets is not received from loop, except no overrides can be chosen. The same reasoning is used with these new plugins. You can decide to make them available, but if you don't have a valid OTP the plugin will not work, thus not "enabled". In other words, there is a difference between available and enabled. By editing the environment variables you make the plugins available. By adding presets and sharing the OTP with a remote provider you make the plugins enabled. There is no current clear "enable remotes" setting in loop, so I would have to defer to @ps2 on this if he thinks there should be one now. But, you can argue that creating the actual override presets and a shared OTP are themselves the enable settings. |
| @@ -231,7 +231,7 @@ function init (ctx) { | |||
| , split: false | |||
| , targets: false | |||
| , reasons: reasonconf | |||
| , otp: true | |||
| , otp: false //TODO: was why was this set to true? I don't think we need that, maybe an option? | |||
There was a problem hiding this comment.
The intention is for loop to have the option to require an OTP even for remote overrides. It was set to true on purpose. The current loop code ignores it for now, but this makes the OTP available if the looper desires to use it in the future.
In fact, I firmly believe that the remote overrides should have used an extra level of authentication when they were first implemented via an OTP. Loopers that are already using the feature have come to expect no extra OTP, though. But, this code is meant to move into a more secure direction wrt doing things remotely.
There was a problem hiding this comment.
Here is a long thread about the feature: https://loop.zulipchat.com/#narrow/stream/144182-development/topic/Remote.20bolus.20.2F.20remote.20carbs
On my March 30 posting I talk about OTP for overrides too...
There was a problem hiding this comment.
I don't agree that this should be changed. Having the ability to enable more security is highly desirable.
|
Where do we sit with this and what needs to be done for merge and release? |
@bewest @sulkaharo @josep1972
cc @ps2
This PR is mostly to point out some issues I saw, and start some discussion, feel free to replace with a different/better PR.
I was looking at the recent merge for #5598, and saw a few things that I think need to be cleaned up and improved.
The main issue is that I don't think the Remote Bolus or Carbs options should be included in the careportal unless they are enabled in Loop. I think that could work in a similar way to how Loop uploads the available
overridePresets. I'm not sure when the Loop PR will be merged, but I assume it will take some time.