[pulseaudio-discuss] Access control

[Tanu Kaskinen]

On Tue, 2017-02-14 at 13:28 +0100, Wim Taymans wrote:
> On 13 February 2017 at 13:58, Tanu Kaskinen <tanuk at iki.fi> wrote:
> > On Fri, 2017-01-27 at 17:15 +0100, Wim Taymans wrote:
> > > Hi All,
> > > 
> > > I think (1) is quite ugly and requires you to modify many functions
> > > with a new parameter that
> > > is not at all related to what the function does.
> > 
> > I disagree with the characterization that the client parameter is not
> > related to what the function does. If the function does access control,
> > then the client parameter is very much related to that.
> 
> Together with things such as logging,configuration and allocation, security
> is typically a Cross-cutting concern
> (https://en.wikipedia.org/wiki/Cross-cutting_concern).
> The methods that operate on objects should not include the parameters for
> the cross-cutting concern, you would typically keep those stored in a global
> environment or singleton (like how we have the logging in a singleton, the
> mempool and configuration in pa_core, ..).
> We should to the same with the current client, IMHO.

The existing stuff in pa_core can't be removed and replaced with
function arguments, as far as I can tell. The current_client field can.

I think having current_client would be completely fine, though, if it
really was only used to store the client whose request is currently
being processed. However, in IRC we talked about needing to set
current_client before calling pa_core_get_sinks() when rescuing
streams, for example. That's quite clearly using a global variable for
just passing a function argument. (This particular problem could be
side-stepped by adding the client argument to pa_core_get_sinks() and
the other similar functions, but otherwise keep using current_client.
As I see it, pa_core_get_sinks() just provides a client-specific view
of the sink list, and it could as well be implemented by storing the
filtered sink list in pa_client, but maintaining that might be more
work than creating the filtered list on demand.)

Aren't there also going to be problems when one operation involves
another operation, if the first operation is allowed for the client but
second operation isn't? The first operation will fail even though it
was supposed to be allowed. For example, module-bluetooth-policy will
automatically switch the card profile from A2DP to HSP when a source
output appears with media.role=phone. I think that shouldn't be
prevented just because the client that created the source output
doesn't have the right to switch card profiles by itself. I guess
you'll end up setting current_client in module-bluetooth-policy to NULL
 before the profile switch while the client still has its stream
creation operation ongoing. (I now realize this wasn't a perfect
example of the problem I first stated, because the source output
creation doesn't fail completely. Maybe a better example would have
been a client that tries to set its stream volume, but it fails because
flat volumes are enabled and the client doesn't have the permission to
set sink volumes.)

Another example is loading a module: the client's permissions affect
what the module can do during the initialization, which from some
perspective may make sense, but in any case that causes
inconsistencies, because the client's access limitations stop affecting
the module as soon as the initialization is completed.

But if you think the current_client approach is the best one, I don't
want to override your preference.

-- 
Tanu

https://www.patreon.com/tanuk