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

[Jonathan Druart]

Hi devs,

Not sure I will manage to explain it better than you...
Basically we all agree that the permission check should not be done
from the controller. It would be good to let the Koha::Object-object
know if a given user can interact with it.
The problem is that the permissions can be different depending on the
action: Add/update/delete/list
Who is able to add a patron? => You need borrowers perm
Who is able to edit detail for this patron? => You need borrowers perm
or you are logged in with this user or the guarantor
Who is able to delete this patron? => same as above (unless you cannot
delete yourself)
Who is able to list patrons? It depends on IndependentBranches: we
want to filter on the search, not after the search (perf issues and
awful design).

And this is the easy example.
After the discussion of yesterday, Tomás was more willing to override
->store and raise an exception if the perms are not enough.
IMO ->store should store if we call it, it's the responsibility of the
developers to test if the logged in user is allowed to update/insert
the stuff:
  if ( $patron->can_be_updated( [$logged_in_user]) ) {
      $patron->store;
  } else {
      # Raise an exception
  }

We already have 2 examples of perms checking:
% git grep can_be Koha/Virtualshelf.pm
sub can_be_viewed {
sub can_be_deleted {
sub can_be_managed {

And, not pushed yet:
Bug 15758 - Move the C4::Branch related code to Koha::Libraries - part 4
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=54864

+sub search_filtered {
+    my ( $self, $params, $attributes ) = @_;
+
+    if (    C4::Context->preference('IndependentBranches')
+        and C4::Context->userenv
+        and not C4::Context->IsSuperLibrarian()
+        and C4::Context->userenv->{branch}
+    ) {
+        $params->{branchcode} = C4::Context->userenv->{branch};
+    }
+
+    return $self->SUPER::search( $params, $attributes );
+}

Hope that adds confusion ;)

Jonathan


2016-08-30 21:11 GMT+01:00 Tomas Cohen Arazi <tomascohen at gmail.com>:
> 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 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/