#615 Use Casbin for the authorization layer

[mario-nt]

Resolves #615

[josecelano]

Hi @mario-nt, I have some questions:

Policy

The policy

p, true, AddCategory
p, true, DeleteCategory
p, true, GetSettings
p, true, GetSettingsSecret
p, true, AddTag
p, true, DeleteTag
p, true, DeleteTorrent
p, true, BanUser

I was expecting something like this:

p, admin, AddCategory
p, admin, DeleteCategory
p, admin, GetSettings
p, admin, GetSettingsSecret
p, admin, AddTag
p, admin, DeleteTag
p, admin, DeleteTorrent
p, admin, BanUser

Missing cases

There are still some authorization points not included in casbin, like the one to update a torrent metadata:

// Check if user is owner or administrator
// todo: move this to an authorization service.
if !(torrent_listing.uploader == updater.username || updater.administrator) {
    return Err(ServiceError::Unauthorized);
}  

Load casbin config from string

As we discussed some weeks ago, it would be good to load the model.conf and policy.csvfrom strings. Otherwise, we have to include those files in the deployment/distribution/containerization of the app. I mean, when you only have the app binary those files should be moved to the storage folder like the config toml files. That would be a good solution if we want to let users to change permissions, but for the time being, we are not planning to do that. It will be a nice feature to add for the next version (v3.1.0)

Make the other public actions permissions explicit

There are other public actions like:

• See the torrents list.
• See a torrent details.
• Etc.

They are now public, but they could require the user to be logged-in in the future.

Maybe we should include them in the policy:

p, guest, SearchTorrents
p, guest, SeeTorrentDetails
...

In fact, in the future we could implement the full-private mode by only changing the casbin model. We could allow the user to define their own model or we can keep casbin as a implementation detail and define to Index modes (public and private), and define a casbin model for each.

cc @da2ce7

[josecelano]

Hi @mario-nt It looks very good now. The only thing I'm not sure is the roles names. In the future we could have more "registered" users. In fact, the admin is also a registered user. I had proposed this in my last review:

enum Role {
   Unknown, // We don't know the role because the user is not authenticated (guest).
   Member, // It's an uploader in the current version
   Admin,
}

What do you think? Anyway, the name you have used are the current names we are using in the code. We can open a new discussion to redefine those names.

[josecelano]

ACK 389acc4