Hi Bastien, On Fri, Nov 20, 2020 at 2:33 AM Bastien Nocera <hadess@xxxxxxxxxx> wrote: > > On Thu, 2020-11-19 at 12:20 -0800, Sonny Sasaka wrote: > > Hi Bastien, > > > > On Thu, Nov 19, 2020 at 2:44 AM Bastien Nocera <hadess@xxxxxxxxxx> > > wrote: > > > > > > On Tue, 2020-11-17 at 14:16 -0800, Sonny Sasaka wrote: > > > > Hi Bastien, > > > > > > > > Thank you for the feedback. Please find my answers below. > > > > > > > > On Tue, Nov 17, 2020 at 2:51 AM Bastien Nocera > > > > <hadess@xxxxxxxxxx> > > > > wrote: > > > > > > > > > > Hey Sonny, > > > > > > > > > > On Tue, 2020-11-10 at 17:17 -0800, Sonny Sasaka wrote: > > > > > > This patch implements the BatteryProvider1 and > > > > > > BatteryProviderManager1 > > > > > > API. This is a means for external clients to feed battery > > > > > > information > > > > > > to > > > > > > BlueZ if they handle some profile and can decode battery > > > > > > reporting. > > > > > > > > > > > > The battery information is then exposed externally via the > > > > > > existing > > > > > > Battery1 interface. UI components can consume this API to > > > > > > display > > > > > > Bluetooth peripherals' battery via a unified BlueZ API. > > > > > > > > > > Was this patch reviewed for potential security problems? From > > > > > the > > > > > top > > > > > of my head, the possible problems would be: > > > > > - I don't see any filters on which user could register battery > > > > > providers, so on a multi user system, you could have a user > > > > > logged > > > > > in > > > > > via SSH squatting all the battery providers, while the user "at > > > > > the > > > > > console" can't have their own providers. Also, what happens if > > > > > the > > > > > user > > > > > at the console changes (fast user switching)? > > > > > - It looks like battery providers don't check for paired, > > > > > trusted > > > > > or > > > > > even connected devices, so I would be able to create nearly > > > > > unbound > > > > > number of battery providers depending on how big the cache for > > > > > "seen" > > > > > devices is. > > > > For security, the API can be access-limited at D-Bus level using > > > > D- > > > > Bus > > > > configuration files. For example, we can let only trusted UNIX > > > > users > > > > as the callers for this API. This D-Bus config file would be > > > > distribution-specific. In Chrome OS, for example, only the > > > > "audio" > > > > and > > > > "power" users are allowed to call this API. This way we can make > > > > sure > > > > that the callers do not abuse the API for denial-of-service kind > > > > of > > > > attack. > > > > > > That wouldn't solve it, the point is to avoid one user causing > > > problems > > > for another logged in user. If both users are in the audio group, > > > which > > > they'd likely be to be able to use the computer, they'd be able to > > > cause problems to each other. > > > > If I understand your case correctly, both users being in "audio" > > group > > still won't allow them both to become battery providers because the > > D-Bus policy only allows "audio" user and not "audio" group. > > OK, I guess that means that this is a separate daemon running as a > different user, not, say, PulseAudio running in the user's session > feeding information. Is that right? Yes, that's right. > > Either way, I guess we'll need to test this out once the feature is > merged. It will first be tested in Chrome OS with CRAS as the battery provider from HFP (I am working on it too). I guess PulseAudio can then follow along so Linux desktops can get headphones batteries in the UI. Then, as Luiz suggested, batteries from HID will be parsed directly from within bluetoothd (a TODO, not blocking the progress of the API). > > Apart from the concern about having to duplicate the exported > properties, the rest looks good. I've made some additional comments > about the architecture in the design document you shared, but those > should not have any impact on the implementation. Thanks for the review. I will update the patches based on your and Luiz's feedback. > > Good job :) >
