Re: [Pulp-dev] AccessPolicy Model Advice ... input needed
Here's my next dilemma: Should we make the API to GET/PUT these access policies all at one endpoint, or nest them as sub-resources under each viewset they control? I'm outlining the two options below, please reply with your thoughts. Option "nested under each viewset". This would have each access policies live under the viewset it pairs with, e.g. the /pulp/api/v3/remotes/file/file/ would then have a sub-resource /pulp/api/v3/remotes/file/file/access_policy/ pros: it's simple for users cons: it's more complicated to develop because we have to try to automatically add this to lots of viewsets. Also it's complicated to see the policies all in one place. Also if there are other sub-resources that may want to use a name like "access_policy" this becomes a problem unless we allow plugin writers to "move" this resource to another area somehow. Option "all at one viewset". This would have all access policies live at /pulp/api/v3/access_policy/ with detail views like /pulp/api/v3/access_policy/:uuid/ pros: it avoids url conflicts described above. It's easy to implement, and you can see all the policies in one place cons: it requires users to "find" their access_policy and this could cause user mistakes also... What do you think we should do to create the best user experience first-and-foremost, and the best plugin writer experience second-most? Thanks! Brian On Tue, Jul 14, 2020 at 8:44 AM Tatiana Tereshchenko wrote: > +1 to idea 1 since the URLs seem to me more stable than class paths. We > should not change REST API much but potentially can refactor code in some > ways. > > One nitpick, maybe the `viewset` field should have a more generic name, > since we don't use viewsets exclusively, we also have separate views, e.g. > for orphan cleanup or status. > I don't have good ideas though... `endpoint`? (though it's not a full > endpoint), `guarded_view_obj`? `view_object`? `view`? :) > > On Tue, Jul 14, 2020 at 2:21 AM Dennis Kliban wrote: > >> Pulp 2 users would definitely be familiar with describing policies in >> terms of urls. The Pulp 3 REST API already uses the pulp_href field as the >> primary identifier. You should implement idea 1. >> >> On Mon, Jul 13, 2020, 5:45 PM Brian Bouterse wrote: >> >>> I need some design input. To store AccessPolicy data in the DB I think >>> we want one Model where each instance is the access policy for a given >>> viewset. I think this would be better than one Model per Viewset which >>> would generate N tables for N viewsets with 1 instance of each which would >>> be very strange and inefficient. >>> >>> So let's assume we have a simple definition like the one below. Each >>> instance stores the policy for one viewset. >>> >>> class AccessPolicy(BaseModel): >>> data = JSONField() >>> >>> So what second field can I add to this that would allow me to relate an >>> instance of this model to a viewset. For example the FileRemoteViewset >>> here: >>> https://github.com/pulp/pulp_file/blob/de638519fc02d588f403db4c5cfcca7552543c50/pulp_file/app/viewsets.py#L116 >>> >>> Idea 1: Add a viewset = CharField(). Have it store values as URLs, e.g. >>> /pulp/api/v3/remotes/file/file/. >>> Idea 2: Add a viewset = CharField(), and have it store values as >>> classpaths, e.g. 'pulp_file.app.viewsets.RemoteFileViewset'. >>> >>> I think Idea 1 makes the most sense because that's how our users think >>> of it. I can't think of a good alternative. What do you think makes the >>> most sense? What alternative ideas should we consider? >>> >>> If you have feedback please share it. I need to start into something to >>> get it going tomorrow even if it's just Idea 1 for lack of an alternate >>> proposal. >>> >>> Thanks, >>> Brian >>> ___ >>> Pulp-dev mailing list >>> Pulp-dev@redhat.com >>> https://www.redhat.com/mailman/listinfo/pulp-dev >>> >> ___ >> Pulp-dev mailing list >> Pulp-dev@redhat.com >> https://www.redhat.com/mailman/listinfo/pulp-dev >> > ___ Pulp-dev mailing list Pulp-dev@redhat.com https://www.redhat.com/mailman/listinfo/pulp-dev
Re: [Pulp-dev] AccessPolicy Model Advice ... input needed
+1 to idea 1 since the URLs seem to me more stable than class paths. We should not change REST API much but potentially can refactor code in some ways. One nitpick, maybe the `viewset` field should have a more generic name, since we don't use viewsets exclusively, we also have separate views, e.g. for orphan cleanup or status. I don't have good ideas though... `endpoint`? (though it's not a full endpoint), `guarded_view_obj`? `view_object`? `view`? :) On Tue, Jul 14, 2020 at 2:21 AM Dennis Kliban wrote: > Pulp 2 users would definitely be familiar with describing policies in > terms of urls. The Pulp 3 REST API already uses the pulp_href field as the > primary identifier. You should implement idea 1. > > On Mon, Jul 13, 2020, 5:45 PM Brian Bouterse wrote: > >> I need some design input. To store AccessPolicy data in the DB I think we >> want one Model where each instance is the access policy for a given >> viewset. I think this would be better than one Model per Viewset which >> would generate N tables for N viewsets with 1 instance of each which would >> be very strange and inefficient. >> >> So let's assume we have a simple definition like the one below. Each >> instance stores the policy for one viewset. >> >> class AccessPolicy(BaseModel): >> data = JSONField() >> >> So what second field can I add to this that would allow me to relate an >> instance of this model to a viewset. For example the FileRemoteViewset >> here: >> https://github.com/pulp/pulp_file/blob/de638519fc02d588f403db4c5cfcca7552543c50/pulp_file/app/viewsets.py#L116 >> >> Idea 1: Add a viewset = CharField(). Have it store values as URLs, e.g. >> /pulp/api/v3/remotes/file/file/. >> Idea 2: Add a viewset = CharField(), and have it store values as >> classpaths, e.g. 'pulp_file.app.viewsets.RemoteFileViewset'. >> >> I think Idea 1 makes the most sense because that's how our users think of >> it. I can't think of a good alternative. What do you think makes the most >> sense? What alternative ideas should we consider? >> >> If you have feedback please share it. I need to start into something to >> get it going tomorrow even if it's just Idea 1 for lack of an alternate >> proposal. >> >> Thanks, >> Brian >> ___ >> Pulp-dev mailing list >> Pulp-dev@redhat.com >> https://www.redhat.com/mailman/listinfo/pulp-dev >> > ___ > Pulp-dev mailing list > Pulp-dev@redhat.com > https://www.redhat.com/mailman/listinfo/pulp-dev > ___ Pulp-dev mailing list Pulp-dev@redhat.com https://www.redhat.com/mailman/listinfo/pulp-dev
Re: [Pulp-dev] AccessPolicy Model Advice ... input needed
On Tue, Jul 14, 2020 at 12:24 PM Brian Bouterse wrote: > > > > On Tue, Jul 14, 2020 at 4:37 AM Matthias Dellweg wrote: >> >> I think, it would help me to understand if you could reiterate what >> kind of policy this model holds. Is it the class level permission like >> "Only admins can create new repositories."? > > It holds a database equivalent of this policy for example: > https://github.com/pulp/pulp_file/compare/master...bmbouter:rbac-PoC#diff-b8a33258feb0183716da827efd0b4102R19-R54 > These would be loaded 100% from the DB and no longer in code. This is more or > less recommended by drf-access-policy here > https://rsinger86.github.io/drf-access-policy/loading_external_source/ ty > >> What kind of context does a lookup for these objects? Is it the >> PermissionClass that carries a context containing an instance to the >> view / viewset, or the request with the resolver_match? > > It only holds the policy, the context data is available on the request and in > the task, so only the "statements" live in the db. That kind of answers my question, that was: Does the code have enough contextual information to build the viewsets base_path or class_path. And is one of them significantly easier to derive / more canonic? Both solutions lead to equally unique keys, right? > >> >> If it is userfacing i would probably also go for option 1 with the hrefs. >> >> On Tue, Jul 14, 2020 at 2:21 AM Dennis Kliban wrote: >> > >> > Pulp 2 users would definitely be familiar with describing policies in >> > terms of urls. The Pulp 3 REST API already uses the pulp_href field as the >> > primary identifier. You should implement idea 1. >> > >> > On Mon, Jul 13, 2020, 5:45 PM Brian Bouterse wrote: >> >> >> >> I need some design input. To store AccessPolicy data in the DB I think we >> >> want one Model where each instance is the access policy for a given >> >> viewset. I think this would be better than one Model per Viewset which >> >> would generate N tables for N viewsets with 1 instance of each which >> >> would be very strange and inefficient. >> >> >> >> So let's assume we have a simple definition like the one below. Each >> >> instance stores the policy for one viewset. >> >> >> >> class AccessPolicy(BaseModel): >> >> data = JSONField() >> >> >> >> So what second field can I add to this that would allow me to relate an >> >> instance of this model to a viewset. For example the FileRemoteViewset >> >> here: >> >> https://github.com/pulp/pulp_file/blob/de638519fc02d588f403db4c5cfcca7552543c50/pulp_file/app/viewsets.py#L116 >> >> >> >> Idea 1: Add a viewset = CharField(). Have it store values as URLs, e.g. >> >> /pulp/api/v3/remotes/file/file/. >> >> Idea 2: Add a viewset = CharField(), and have it store values as >> >> classpaths, e.g. 'pulp_file.app.viewsets.RemoteFileViewset'. >> >> >> >> I think Idea 1 makes the most sense because that's how our users think of >> >> it. I can't think of a good alternative. What do you think makes the most >> >> sense? What alternative ideas should we consider? >> >> >> >> If you have feedback please share it. I need to start into something to >> >> get it going tomorrow even if it's just Idea 1 for lack of an alternate >> >> proposal. >> >> >> >> Thanks, >> >> Brian >> >> ___ >> >> Pulp-dev mailing list >> >> Pulp-dev@redhat.com >> >> https://www.redhat.com/mailman/listinfo/pulp-dev >> > >> > ___ >> > Pulp-dev mailing list >> > Pulp-dev@redhat.com >> > https://www.redhat.com/mailman/listinfo/pulp-dev >> ___ Pulp-dev mailing list Pulp-dev@redhat.com https://www.redhat.com/mailman/listinfo/pulp-dev
Re: [Pulp-dev] AccessPolicy Model Advice ... input needed
On Tue, Jul 14, 2020 at 4:37 AM Matthias Dellweg wrote: > I think, it would help me to understand if you could reiterate what > kind of policy this model holds. Is it the class level permission like > "Only admins can create new repositories."? > It holds a database equivalent of this policy for example: https://github.com/pulp/pulp_file/compare/master...bmbouter:rbac-PoC#diff-b8a33258feb0183716da827efd0b4102R19-R54 These would be loaded 100% from the DB and no longer in code. This is more or less recommended by drf-access-policy here https://rsinger86.github.io/drf-access-policy/loading_external_source/ What kind of context does a lookup for these objects? Is it the > PermissionClass that carries a context containing an instance to the > view / viewset, or the request with the resolver_match? > It only holds the policy, the context data is available on the request and in the task, so only the "statements" live in the db. > If it is userfacing i would probably also go for option 1 with the hrefs. > > On Tue, Jul 14, 2020 at 2:21 AM Dennis Kliban wrote: > > > > Pulp 2 users would definitely be familiar with describing policies in > terms of urls. The Pulp 3 REST API already uses the pulp_href field as the > primary identifier. You should implement idea 1. > > > > On Mon, Jul 13, 2020, 5:45 PM Brian Bouterse > wrote: > >> > >> I need some design input. To store AccessPolicy data in the DB I think > we want one Model where each instance is the access policy for a given > viewset. I think this would be better than one Model per Viewset which > would generate N tables for N viewsets with 1 instance of each which would > be very strange and inefficient. > >> > >> So let's assume we have a simple definition like the one below. Each > instance stores the policy for one viewset. > >> > >> class AccessPolicy(BaseModel): > >> data = JSONField() > >> > >> So what second field can I add to this that would allow me to relate an > instance of this model to a viewset. For example the FileRemoteViewset > here: > https://github.com/pulp/pulp_file/blob/de638519fc02d588f403db4c5cfcca7552543c50/pulp_file/app/viewsets.py#L116 > >> > >> Idea 1: Add a viewset = CharField(). Have it store values as URLs, e.g. > /pulp/api/v3/remotes/file/file/. > >> Idea 2: Add a viewset = CharField(), and have it store values as > classpaths, e.g. 'pulp_file.app.viewsets.RemoteFileViewset'. > >> > >> I think Idea 1 makes the most sense because that's how our users think > of it. I can't think of a good alternative. What do you think makes the > most sense? What alternative ideas should we consider? > >> > >> If you have feedback please share it. I need to start into something to > get it going tomorrow even if it's just Idea 1 for lack of an alternate > proposal. > >> > >> Thanks, > >> Brian > >> ___ > >> Pulp-dev mailing list > >> Pulp-dev@redhat.com > >> https://www.redhat.com/mailman/listinfo/pulp-dev > > > > ___ > > Pulp-dev mailing list > > Pulp-dev@redhat.com > > https://www.redhat.com/mailman/listinfo/pulp-dev > > ___ Pulp-dev mailing list Pulp-dev@redhat.com https://www.redhat.com/mailman/listinfo/pulp-dev
Re: [Pulp-dev] AccessPolicy Model Advice ... input needed
I think, it would help me to understand if you could reiterate what kind of policy this model holds. Is it the class level permission like "Only admins can create new repositories."? What kind of context does a lookup for these objects? Is it the PermissionClass that carries a context containing an instance to the view / viewset, or the request with the resolver_match? If it is userfacing i would probably also go for option 1 with the hrefs. On Tue, Jul 14, 2020 at 2:21 AM Dennis Kliban wrote: > > Pulp 2 users would definitely be familiar with describing policies in terms > of urls. The Pulp 3 REST API already uses the pulp_href field as the primary > identifier. You should implement idea 1. > > On Mon, Jul 13, 2020, 5:45 PM Brian Bouterse wrote: >> >> I need some design input. To store AccessPolicy data in the DB I think we >> want one Model where each instance is the access policy for a given viewset. >> I think this would be better than one Model per Viewset which would generate >> N tables for N viewsets with 1 instance of each which would be very strange >> and inefficient. >> >> So let's assume we have a simple definition like the one below. Each >> instance stores the policy for one viewset. >> >> class AccessPolicy(BaseModel): >> data = JSONField() >> >> So what second field can I add to this that would allow me to relate an >> instance of this model to a viewset. For example the FileRemoteViewset here: >> https://github.com/pulp/pulp_file/blob/de638519fc02d588f403db4c5cfcca7552543c50/pulp_file/app/viewsets.py#L116 >> >> Idea 1: Add a viewset = CharField(). Have it store values as URLs, e.g. >> /pulp/api/v3/remotes/file/file/. >> Idea 2: Add a viewset = CharField(), and have it store values as classpaths, >> e.g. 'pulp_file.app.viewsets.RemoteFileViewset'. >> >> I think Idea 1 makes the most sense because that's how our users think of >> it. I can't think of a good alternative. What do you think makes the most >> sense? What alternative ideas should we consider? >> >> If you have feedback please share it. I need to start into something to get >> it going tomorrow even if it's just Idea 1 for lack of an alternate proposal. >> >> Thanks, >> Brian >> ___ >> Pulp-dev mailing list >> Pulp-dev@redhat.com >> https://www.redhat.com/mailman/listinfo/pulp-dev > > ___ > Pulp-dev mailing list > Pulp-dev@redhat.com > https://www.redhat.com/mailman/listinfo/pulp-dev ___ Pulp-dev mailing list Pulp-dev@redhat.com https://www.redhat.com/mailman/listinfo/pulp-dev
Re: [Pulp-dev] AccessPolicy Model Advice ... input needed
Pulp 2 users would definitely be familiar with describing policies in terms of urls. The Pulp 3 REST API already uses the pulp_href field as the primary identifier. You should implement idea 1. On Mon, Jul 13, 2020, 5:45 PM Brian Bouterse wrote: > I need some design input. To store AccessPolicy data in the DB I think we > want one Model where each instance is the access policy for a given > viewset. I think this would be better than one Model per Viewset which > would generate N tables for N viewsets with 1 instance of each which would > be very strange and inefficient. > > So let's assume we have a simple definition like the one below. Each > instance stores the policy for one viewset. > > class AccessPolicy(BaseModel): > data = JSONField() > > So what second field can I add to this that would allow me to relate an > instance of this model to a viewset. For example the FileRemoteViewset > here: > https://github.com/pulp/pulp_file/blob/de638519fc02d588f403db4c5cfcca7552543c50/pulp_file/app/viewsets.py#L116 > > Idea 1: Add a viewset = CharField(). Have it store values as URLs, e.g. > /pulp/api/v3/remotes/file/file/. > Idea 2: Add a viewset = CharField(), and have it store values as > classpaths, e.g. 'pulp_file.app.viewsets.RemoteFileViewset'. > > I think Idea 1 makes the most sense because that's how our users think of > it. I can't think of a good alternative. What do you think makes the most > sense? What alternative ideas should we consider? > > If you have feedback please share it. I need to start into something to > get it going tomorrow even if it's just Idea 1 for lack of an alternate > proposal. > > Thanks, > Brian > ___ > Pulp-dev mailing list > Pulp-dev@redhat.com > https://www.redhat.com/mailman/listinfo/pulp-dev > ___ Pulp-dev mailing list Pulp-dev@redhat.com https://www.redhat.com/mailman/listinfo/pulp-dev
[Pulp-dev] AccessPolicy Model Advice ... input needed
I need some design input. To store AccessPolicy data in the DB I think we want one Model where each instance is the access policy for a given viewset. I think this would be better than one Model per Viewset which would generate N tables for N viewsets with 1 instance of each which would be very strange and inefficient. So let's assume we have a simple definition like the one below. Each instance stores the policy for one viewset. class AccessPolicy(BaseModel): data = JSONField() So what second field can I add to this that would allow me to relate an instance of this model to a viewset. For example the FileRemoteViewset here: https://github.com/pulp/pulp_file/blob/de638519fc02d588f403db4c5cfcca7552543c50/pulp_file/app/viewsets.py#L116 Idea 1: Add a viewset = CharField(). Have it store values as URLs, e.g. /pulp/api/v3/remotes/file/file/. Idea 2: Add a viewset = CharField(), and have it store values as classpaths, e.g. 'pulp_file.app.viewsets.RemoteFileViewset'. I think Idea 1 makes the most sense because that's how our users think of it. I can't think of a good alternative. What do you think makes the most sense? What alternative ideas should we consider? If you have feedback please share it. I need to start into something to get it going tomorrow even if it's just Idea 1 for lack of an alternate proposal. Thanks, Brian ___ Pulp-dev mailing list Pulp-dev@redhat.com https://www.redhat.com/mailman/listinfo/pulp-dev
