[Koha-devel] Koha::* business objects authorization check

[Jonathan Druart]

2016-08-30 21:23 GMT+01:00 Chris Cormack <chrisc at catalyst.net.nz>:
> * Tomas Cohen Arazi (tomascohen at gmail.com) wrote:
>> Hi everyone, adding to the discussion about Koha::Objects I have a subject that
>> needs broad (an urgent) discussion.
>>
>> The REST api patches are shaping up, and I'd like to focus on an important one:
>> 14868 [1].
>>
>> This bug introduces a way of specifying on the Swagger files, the required
>> privileges to do stuff on objects. For example:
>>
>>
>> +      "x-koha-authorization": {
>> +        "allow-owner": true,
>> +        "allow-guarantor": true,
>> +        "permissions": {
>> +          "borrowers": "1"
>> +        }
>>        }
>>
>> specifies that to get a patron object you need to either have the 'borrowers'
>> permission, be the object owner, or the guarantor of the object's owner.
>>
>> This is really nice, because permissions definition are now higher level
>> entities (just a configuration thing), and they are part of the API
>> documentation. There's even a bug with patches that introduce required
>> permissions in the error messages.
>>
>> The current implementation introduces checks for each object class on the
>> controller code.
>>
>> My feeling is that each business object should be responsible for such a check.
>> The first step I made was to extend Koha::Object so it exposes an ->owner
>> method that returns the Koha::Patron object that owns the object (because we
>> can get the guarantees from there too).
>
> I like it, it is only when we do things like this that the Objects become useful,
> otherwise they are a shim over db access (and a slow shim at that).
> Once they do more things like this, they will be a lot more useful.
>
> Chris

I think it's also quite useful when we manage to migrate our legacy
code to OO quite easily (reducing the number of lines and adding test
coverage).
Note that Koha::Object[s] does not reduce speed compare to using
DBIx::Class directly, I have already proved that previously. Your turn
to prove the contrary if you think you are right :)

Cheers,
Jonathan

>
>> I did this to eliminate the need of class-specific checks on the controller.
>> But at the end of the day, this still didn't feel it was good enough. I belive
>> such a business matter should not be so tied to the controller/routing code and
>> it is still there.
>>
>> Talking with Jonathan we thought we could make Koha::Objects->search accept
>> more parameters (current userid, required permissions) and have the code
>> checking authorization there, basically creating a filter on the search. The
>> main problem is how to propagate the swagger-specified required permissions
>> down to the Koha::Object in a clean way.
>>
>> I'd like to hear how people belive we should handle on this new+fresh code
>> permissions check, so we have good foundations for the future of the project.
>> Cleaning untested business logic from controller scripts is a good start! And
>> this is the time to do it!
>>
>> Thanks in advance
>>
>> PS. I'm sure Jonathan will explain the discussion better once he comes back
>> from the pub :-D
>>
>> [1] https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14868
>> --
>> Tomás Cohen Arazi
>> Theke Solutions (https://theke.io)
>> ✆ +54 9351 3513384
>> GPG: B2F3C15F
>
>> _______________________________________________
>> Koha-devel mailing list
>> Koha-devel at lists.koha-community.org
>> http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel
>> website : http://www.koha-community.org/
>> git : http://git.koha-community.org/
>> bugs : http://bugs.koha-community.org/
>
>
> --
> Chris Cormack
> Catalyst IT Ltd.
> +64 4 803 2238
> PO Box 11-053, Manners St, Wellington 6142, New Zealand
>
> _______________________________________________
> Koha-devel mailing list
> Koha-devel at lists.koha-community.org
> http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel
> website : http://www.koha-community.org/
> git : http://git.koha-community.org/
> bugs : http://bugs.koha-community.org/