Here are some Pulp 2 links regarding how we handle Content Protection:

https://github.com/pulp/pulp_rpm/blob/2-master/plugins/etc/httpd/conf.d/pulp_rpm.conf#L48
https://github.com/pulp/pulp/blob/2-master/server/usr/share/pulp/wsgi/repo_auth.wsgi
https://github.com/pulp/pulp/blob/2-master/repoauth/pulp/repoauth/wsgi.py#L10-L47

Underscores strike me as odd (and a pain) outside of programming languages. Hyphens in URLs seem more common. Any objection to using hyphens instead?

POST /pulp/api/v3/content-guards/<type>/

Instead of:

POST /pulp/api/v3/content_guards/<type>/

What mechanism do we want to use for contributing the guard logic?

Entry points? Like this?

    entry_points={
        'pulpcore.content_guard': [
            'name=something:callable',
        ]
    }

Where the name matches the ContentGuard.name associated with the Distribution
Where callable is a callable that is passed the Request.

We been using this convention where discovery happens here as a 2 step process for things like Viewsets for example.

1. Use Django to discover django "apps" normally
2. Look for files by concention or instance type and use them

Read that code and see if it makes any sense.

bmbouter wrote:

We been using this convention where discovery happens here as a 2 step process for things like Viewsets for example.

1. Use Django to discover django "apps" normally
2. Look for files by concention or instance type and use them

Read that code and see if it makes any sense.

The code in apps.py inspects modules/packages looking for classes with django bases like serializers and viewsets. It identifies the classes using isinstance(). The ContentGuard would already be loaded as a model and used only when fetched from the DB. The approach for how to load the content guard logic in core still isn't clear. This is similar to how things were with sync and publishing logic as it related to their respective models (before moving the logic to the tasks). Remember the circular import discussion.

A few approaches to discuss. At this point, I'm just brainstorming.

A The content guard logic is a function passed a request and raises an exception for denials. The decorator would load and catalog the guard logic and would be really straight forward to implement. Something like this:

contentguards.py

from .models import GreatContentGuard

@contentguard(model=GreatContentGuard)
def permit(request, content_guard):
   ...

B Just add a (something like) ContentGuard.permit(self, request) method to the model. This contradicts an earlier design decision to limit methods on models subclasses to only those pertaining to the data model (ORM layer). This decision provides a healthy separation of concerns. Note: we violate this with Remote.get_downloader().

I think option A is good, simple reusable pattern for all things contributed by plugins where separation of the models and contributed application logic is needed.

Thoughts?

I think option A is good, simple reusable pattern for all things contributed by plugins where separation of the models and contributed application logic is needed.

+1 from me.

+1 to option A, this looks legit.

Actually why are we separating the logic from the data model that goes with it? I think we want those things together.

The plugin would contribute the model and a CRUD viewset for it. Then the content app would get that instance and call permit() as a method. For example the model would be:

class GreatContentGuard(Model):
    ...

    def permit(self, request):
        # logic goes here

bmbouter wrote:

Actually why are we separating the logic from the data model that goes with it? I think we want those things together.

The plugin would contribute the model and a CRUD viewset for it. Then the content app would get that instance and call permit() as a method. For example the model would be:

The reasoning is touched on in option B in #3968-14. The ContentGuard IsA Model which is part of the ORM object model. A permit operation seems inappropriate in that it does not pertain to the concerns of the ORM layer. Django embraces the value of this separation as well. For example: the view layer is separated from the model (ORM layer). Rather than just adding serialization methods to the model, a separate Serializer object provides this functionality and is associated to the model. It would be convenient to just add a permit() method to the model but it's not the proper thing to do.

Separating the permit() function from its model makes sense if we plan to use them separately, but I don't think we want that for this feature. To paraphrase this thought as a question: will users or plugin writers "pair" a permit() function with a with ContentGuardA in some installations and ContentGuardB in others? I think the usefullness of that re-pairing and re-using is almost none, and that it makes the most sense for a content guard function, e.g. permit() to always be used with a specific model type that goes with it, e.g. ContentGuard.

It seems simpler to put them together both conceptually and structurally. Conceptually, it's simpler for plugin writers to make them because it's easier to understand (just subclass and implement the permit() method). Structurally it's also simpler; since Django already gives you the model instance, just call .permit() on it and we no longer need to maintain any "mappings" at all which avoids the need to implement that aspect of it.

Brian, Jeff,

I think that the permit() is a function of multiple objects:

• the ContentGuard because in case of e.g an RBAC content guard, some data would be required from the RBACContentGuard model instance(s)
• the ContentView as it provides the end-point and the request for the ContentGuard to check and it is the spot where the response (permission denied) would be triggered.
• the Distribution as that's the source of the data concerned and different distributions might require different guards (levels of security) applied e.g different permissions for different organisations that consume their respective distributions.

...so that one can e.g assign the RBACContetGuard to two different organizations, consuming Rust crates of RustOrganisationDistribution().org_name = 'OrgA', RustOrganisationDistribution().org_name = 'OrgB' instances thru the RustContentView

I also think the guards can be quite generic, so not every pulp plug-in would have to sub-class e.g an TLSContetGuard.
I'd assume multiple chained guards might be needed in order to provide different constraints so one can e.g apply a generic TLSContentGuard together with a generic RBACContentGuard to different distributions, run-time, as admin posting to some distributor endpoint or so.

Does this make sense?

Jeff, I think the decorator would bind the guard "compile-time", reducing the flexibility here.
Brian, could you please elaborate on how the ContentGuard.permit() method would be bound to a distribution and a view?

Thanks,
milan

The ContentGuard and Distribution have a one-to-many relationship in the DB.

So the chaining of ContentGuards isn't an option, correct?
Could you please describing the lifecycle of a content guard with regards to particular repo/distribution so it's more clear what use the ContentGuard will have?

milan wrote:

So the chaining of ContentGuards isn't an option, correct?

Yes, that's the current plan.

Could you please describing the lifecycle of a content guard with regards to particular repo/distribution so it's more clear what use the ContentGuard will have?

The usage I imagine is that one ContentGuard configured on a Pulp system, e.g. a certificate guard which only hands out content if the cert is correct and attests the user should have access to that content. Then this would be configured to one or more distibutions, and as those ditributions over time expose different publications the ContentGuard would stay in-place and not be updated. One ContentGuard may be used to protect N distributions, but the current plan is to have 1 distribution only use 1 content guard.

milan wrote:

Brian, Jeff,

I think that the permit() is a function of multiple objects:

• the ContentGuard because in case of e.g an RBAC content guard, some data would be required from the RBACContentGuard model instance(s)
• the ContentView as it provides the end-point and the request for the ContentGuard to check and it is the spot where the response (permission denied) would be triggered.
• the Distribution as that's the source of the data concerned and different distributions might require different guards (levels of security) applied e.g different permissions for different organisations that consume their respective distributions.

...so that one can e.g assign the RBACContetGuard to two different organizations, consuming Rust crates of RustOrganisationDistribution().org_name = 'OrgA', RustOrganisationDistribution().org_name = 'OrgB' instances thru the RustContentView

I also think the guards can be quite generic, so not every pulp plug-in would have to sub-class e.g an TLSContetGuard.
I'd assume multiple chained guards might be needed in order to provide different constraints so one can e.g apply a generic TLSContentGuard together with a generic RBACContentGuard to different distributions, run-time, as admin posting to some distributor endpoint or so.

Does this make sense?

I don't understand what is described here. I also think we should first talk about the problem these layers and reusable components are solving first.

Jeff, I think the decorator would bind the guard "compile-time", reducing the flexibility here.
Brian, could you please elaborate on how the ContentGuard.permit() method would be bound to a distribution and a view?

Yes the Distribution has a GenericForeignKey to the ContentGuard model instance, which has the permit() implementation right on it. That's why we should put permit() on the model because it makes the whole design very simple.

Thanks,
milan

I've read through the comments here and thought about this some more and I am +1 to option B. I think it's the simplest solution even though it combines concerns (ORM and permit logic).

bmbouter wrote:

One ContentGuard may be used to protect N distributions, but the current plan is to have 1 distribution only use 1 content guard.

No, the current plan is that one content guard can protect N distributions.

milan wrote:

Yes the Distribution has a GenericForeignKey to the ContentGuard model instance, which has the permit() implementation right on it. That's why we should put permit() on the model because it makes the whole design very simple.

There's no question that it's a little bit simpler but correct should always be more important than simple. This decision would mean one of two things:

1. That consensus is to trade separation of concerns and layering principals of good software design for convenience.
2. That consensus is to redefine the current data (object) model as a persistent application domain object model. This architecture shift is perfectly valid though less common. Especially in Django applications. It would mean that each Model would represent a Thing within the entire application problem domain not just an Entity in the relational DB. As the domain object, the permit() would not only be an acceptable operation on the ContentGuard but would be the most proper. With this shift, to be consistent, there would likely be a good bit of other functionality in other parts of the code base that should be moved to the appropriate domain object.

So, which of the two is being proposed by adding ContentGuard.permit()?

jortel@redhat.com wrote:

bmbouter wrote:

One ContentGuard may be used to protect N distributions, but the current plan is to have 1 distribution only use 1 content guard.

No, the current plan is that one content guard can protect N distributions.

I'm not sure if you're agreeing or disagreeing with my statement.

jortel@redhat.com wrote:

milan wrote:

Yes the Distribution has a GenericForeignKey to the ContentGuard model instance, which has the permit() implementation right on it. That's why we should put permit() on the model because it makes the whole design very simple.

There's no question that it's a little bit simpler but correct should always be more important than simple. This decision would mean one of two things:

1. That consensus is to trade separation of concerns and layering principals of good software design for convenience.

There is no sacrifice of correctness here; I point that out because the design suggested is built on the assumption that adding logic to models incorrect. Separation of concerns is great for reuse, but if components always are used together, like in this design, separating them adds complexity. This is a shorter response of what I've written in Comment 19.

1. That consensus is to redefine the current data (object) model as a persistent application domain object model. This architecture shift is perfectly valid though less common. Especially in Django applications. It would mean that each Model would represent a Thing within the entire application problem domain not just an Entity in the relational DB. As the domain object, the permit() would not only be an acceptable operation on the ContentGuard but would be the most proper. With this shift, to be consistent, there would likely be a good bit of other functionality in other parts of the code base that should be moved to the appropriate domain object.

So, which of the two is being proposed by adding ContentGuard.permit()?

bmbouter wrote:

jortel@redhat.com wrote:

milan wrote:

Yes the Distribution has a GenericForeignKey to the ContentGuard model instance, which has the permit() implementation right on it. That's why we should put permit() on the model because it makes the whole design very simple.

There's no question that it's a little bit simpler but correct should always be more important than simple. This decision would mean one of two things:

1. That consensus is to trade separation of concerns and layering principals of good software design for convenience.

There is no sacrifice of correctness here; I point that out because the design suggested is built on the assumption that adding logic to models incorrect.

The correctness objection made in (1) is that adding logic to a Model is fine so long as it falls within its concerns as an object which represents an Entity in the DB (ORM). We do this already in other models. In contrast to this, The proposed ContentGuard.permit() has nothing to do with an Entity and instead deals with authorizing a web Request.

Separation of concerns is great for reuse, but if components always are used together, like in this design, separating them adds complexity. This is a shorter response of what I've written in Comment 19.

I agree with the correctness objection with putting the permit() method on the ContentGuard model. That said, in my experience, strictly adhering to good software design rules tends to lead to complex and complicated solutions. In this case, the option to put the permit() method on ContentGuard doesn't require a registry and it doesn't require plugin writers to have to figure out where to put their permit logic. It keeps all the content protection stuff in one place that's easy to find. Are we trading off good software design for convenience and simplicity? Perhaps but I find this case to be a compelling reason to do so.

I agree that separating the ContentGuard model object from the 'permit' logic adds no value for the plugin writer. I don't know of a use case where the plugin writer would want to use the permit logic of one content guard type with an instance of a different content guard model. I do see a lot of value in keeping the plugin writer experience simple.

dkliban@redhat.com wrote:

I agree that separating the ContentGuard model object from the 'permit' logic adds no value for the plugin writer. I don't know of a use case where the plugin writer would want to use the permit logic of one content guard type with an instance of a different content guard model. I do see a lot of value in keeping the plugin writer experience simple.

There is no part of the correctness objection that claims to support using the ContentGuard model and logic separately so I don't see the relevance of that argument. Putting the logic in the model is obviously simpler than decorating a function but difference in complexity is negligible. Having an ORM model object approve a web request is just plain wrong.

There seems to be a majority of opinion noted on this issue to put the logic in the model. This is not the best decision but the broader community had the opportunity to weigh-in and did not so they will have to live with it.

There are certain 'best practices' when writing software. The single responsibility principle would dictate that a data model's object should only contain functions that are relevant to working with that data model object.

https://en.wikipedia.org/wiki/Single_responsibility_principle

I'm also concerned here at setting the precedence of bypassing software engineering best practices for a concept such as keeping the plugin writer experience simple. While plugin simplicity should be a goal, it definitely shouldn't be the only metric. Pulp should also strive for having a good, quality code base that is easy for pulp core developers to work with.

Polluting the model space with other functionality would be something that would be considered a code smell, and could have an adverse effect, and deter contributors away from the project by decreasing faith in the code base.

From what I can glean from this issue, the permit function sounds more like it should be applied to something like a controller (borrowing from the MVC paradigm for a second), instead of the model, to ensure proper abstraction.

So, Dennis and I had a nice chat over IRC. And I figured this summary should help a) provide some more detail on what the technical aspects are, and b) clear up my opinion with respect to a future proposed technical implementation.

This comes down to just what will be put into the permit() function, if it's attached to the model itself.

Dennis expressed to me, that the intent here is to follow this statement of this document [0]:

Make ‘em Fat¶

A common pattern in MVC-style programming is to build thick/fat models and thin controllers. For Django this translates to building models with lots of small methods attached to them and views which use those methods to keep their logic as minimal as possible. There are lots of benefits to this approach.

1. DRY: Rather than repeating the same logic in multiple views, it is defined once on the model.
2. Testable: Breaking up logic into small methods on the model makes your code easier to unit test.
3. Readable: By giving your methods friendly names, you can abstract ugly logic into something that is easily readable and understandable.

For a good example of a fat model in Django, look at the definition of django.contrib.auth.models.User.

He then went on to explain that the intent of a 'permit()' function on ContentGuard would be along the lines of check_password on the aforementioned User model [1] of the doc:

from django.contrib.auth.hashers import (
    check_password, make_password, is_password_usable, UNUSABLE_PASSWORD)

# ...{snip a lot of code}...

class User(models.Model):

# ...{snip a lot more code}...

    def check_password(self, raw_password):
        """
        Returns a boolean of whether the raw_password was correct. Handles
        hashing formats behind the scenes.
        """
        def setter(raw_password):
            self.set_password(raw_password)
            self.save()
        return check_password(raw_password, self.password, setter)

This example, not taking [0] into account, would still be, in my book, within the spirit of the single responsibility principle. I've usually seen and accepted such a practice for model design upon my internal logic:

     This example takes the model instance, and calls into outside code to 'validate' or manipulate data that exists on said model, and is relevant only this model instance.

It calls into the imported check_passsword function from django.contrib.auth.hashers, which does the heavy lifting, while providing said funciton input from the model-specific data.

As long as 'permit()' would continue to follow said intent, and not stray into implementing specific non-model dependent algorithm logic, I'm +1 for having such a function on the model.

If 'permit()' starts implementing the algorithm, then it would make more sense to refactor such algorithm into a separate location, and import it into the model for permit to take advantage of.

So, to restate, as long as ContentGuard.permit() offloads the heavy lifting to something external of the ContentGuard model, and focuses on calling such code with ContentGuard instance data, then it definitely makes sense to keep it on ContentGuard. If it strays outside that definition, then some more thought should be given to the design.

[0] https://django-best-practices.readthedocs.io/en/latest/applications.html#make-em-fat
[1] https://github.com/django/django/blob/ff6ee5f06c2850f098863d4a747069e10727293e/django/contrib/auth/models.py#L296-L304

pcreech, I agree although the only problem I see is that the permit() method is passed an HttpRequest which I think doesn't belong in the data/model domain. I wonder if we can instead pass in the necessary data to permit() instead of an HttpRequest?

pcreech, thanks for the clarification. I also agree with Fat Models for all the reason listed in the django documentation. Pulp has Fat Models already as I have already pointed out. The User.check_password() method provides an answer the the question "Does this password match what is stored in the DB?" It is not itself performing authorization. This method easily remains within the concerns of a database Entity (the model) because it is only encapsulating the hashing complexity re: how the password is stored and answering a question about a row in the table. Does it match. This is very different than what ContentGuard.permit() will likely do. Instead, we'd be handing it a web request and asking it to perform web request authorization. For example: an SSL guard will likely get both the certificate and the key paths from an attribute of the model (a row in the table) but would be answering the question. "Is this web request authorized?" It would likely delegate the decision to an SSL library because it cannot answer this question on its own. IMHO, this goes well outside the concerns of a database Entity (row in the table) and goes beyond what the django community considers a Fat Model.

@daviddavis Good thought on the 'pass in the necessary data to permit'. I must admit I didn't give too much thought to the actual input of the function at that time. Passing in an entire web request and expecting it to extract the data it needs from it could easily be considered outside of scope, since it would need special knowledge of how to get data out of a web request.

@jortel Yes, and I think you helped frame what the question should be for a method on the model. And if you look at the full `django.contrib.auth.models.User` model, every one of those functions utilize a similar format w/r to what data the model holds, and works directly with that model data, or on that model data.

So, let's take that idea, and look at it in the context of what you provided, a 'SslContentGuard'. I'm suspending the use of 'permit' here, to help clarify what the method is doing.

If it was done this way:

import ssl_check_library

class SslContentGuard(models.Model):
    def __init__(self, ca):
        self.my_ca = ca

    def check_x509_against_my_ca(self, x509):  # could be permit(self, x509)
        return ssl_check_library.valid_x509(self.my_ca, x509)

While possibly a little grey, It could be viewed as valid, since it's validating some data within it's context of 'my_ca'. This model's clear intent is to be a representation of a SslContentGuard for a specific CA, 'my_ca'.

But, if the method is something like this:

import ssl_check_library
from request_validation import ValidRequest, InvalidRequest

class SslContentGuard(models.Model):

    def check_web_request_x509_against_system_certificate_store(self, request):
        x509 = request.x509
        system_ca_store = "/etc/pki/some.store" #  Can't remember absolute proper path here
        ssl_check = ssl_check_library.valid_x509(self.my_ca, x509)
        if ssl_check:
            return ValidRequest()
        else:
            return InvalidRequest()

Then, I think that pollutes the intent here. This model now has to know how to find the system_ca_store, extract data from the web request, and decide if it's valid or invalid. This type of situation would lend itself to a more 'config object' situation, where you store the config data in the database, but have another class that takes that config data, and then performs an action. Said class would then be the code to be interacted with, and would be the arbiter of what's valid or not.

Another example would be, if I were reaching out to an authentication server. If the model has to know how to talk to an authentication server, such as an Ldap server, or get data out of that server in any way, that could be going against best practices.

So, to summarize, it really depends on what the intent of 'permit' is, and what code ends up in there. If it's the latter, then a 'config object' that is passed to a type of controller, service, or helper type class might be more preferable.

The benefits of allowing the plugin writer to provide the permit logic as part of the ContentGuard model are the following:

The 'permit' logic can be discovered using existing infrastructure.
Plugin writer can provide a single object to represent both the configuration options and behavior of a ContentGuard.
The design follows the best practices outlined here[0].

Can someone succinctly describe material benefits of keeping the permit logic outside the model?

[0] https://django-best-practices.readthedocs.io/en/latest/applications.html#make-em-fat

Throwing in some thoughts:

Jeff wrote:

I think option A is good, simple reusable pattern for all things contributed by plugins where separation of the models and contributed application logic is needed.

Do we have any planned or unplanned features for which this pattern might be applicable, or is this the only feature we currently forsee using it? I certainly see the value in reusable components and patterns, although I think it would be more a much more compelling argument if we could actually think of one.

Do you feel that Remote.get_downloader() is similarly flawed? If not, can you articulate how this case is different? My initial reaction is to see these examples as being a very similar sort of thing, albeit with two differences that don't invalidate the functional similarity. Those differences being that we don't expect Plugin Writers to overload Remote.get_downloader(), and that it is written only once instead of , thus making it more a less visible design element. But as I mentioned, I still believe it is functionally similar.

Patrick wrote:

I'm also concerned here at setting the precedence of bypassing software engineering best practices for a concept such as keeping the plugin writer experience simple. While plugin simplicity should be a goal, it definitely shouldn't be the only metric. Pulp should also strive for having a good, quality code base that is easy for pulp core developers to work with.

I don't think this exclusively makes things easier for plugin writers. A new, separate registry system for things like Content Guards are another component that core would have to maintain and another possible source of tricky bugs. And if we want to have the future flexibility to re-use this pattern for other things as Jeff mentioned, that means we're either dealing with multiple registries for new features (code duplication?), or we need to over-engineer it a bit to be generic enough that other features could use the same core registry code.

Jeff, let me know if I'm overthinking the complexity of such a thing in practice. I admit that it's possible, I have difficulty visualizing how the code would work so maybe it's simpler than I'm imagining.

My general opinion: I'm partial to Option B. I'm concerned about adding too much complexity (== possible sources of tricky bugs) just for the sake of purity. I could be convinced otherwise if the additional complexity would add a significant functional value, but I don't currently see that being the case.

Thanks, dalley.

dalley wrote:

Throwing in some thoughts:

Jeff wrote:

[...]

Do we have any planned or unplanned features for which this pattern might be applicable, or is this the only feature we currently forsee using it? I certainly see the value in reusable components and patterns, although I think it would be more a much more compelling argument if we could actually think of one.

Not that I am aware of. That said, had we not moved sync and publish to tasks, this pattern would be been a good fit.

Do you feel that Remote.get_downloader() is similarly flawed? If not, can you articulate how this case is different? My initial reaction is to see these examples as being a very similar sort of thing, albeit with two differences that don't invalidate the functional similarity. Those differences being that we don't expect Plugin Writers to overload Remote.get_downloader(), and that it is written only once instead of , thus making it more a less visible design element. But as I mentioned, I still believe it is functionally similar.

No, I don't see Remote.get_downloader() as being similarly flawed because the model is being asked to build an object that is configured using the information in the DB. In contrast, something like: Remote.download() would be more equivalent to ContentGuard.permit() and would be equally inappropriate because it would be asking a database Entity (model) to download a file.

Patrick wrote:

[...]

I don't think this exclusively makes things easier for plugin writers. A new, separate registry system for things like Content Guards are another component that core would have to maintain and another possible source of tricky bugs. And if we want to have the future flexibility to re-use this pattern for other things as Jeff mentioned, that means we're either dealing with multiple registries for new features (code duplication?), or we need to over-engineer it a bit to be generic enough that other features could use the same core registry code.

Jeff, let me know if I'm overthinking the complexity of such a thing in practice. I admit that it's possible, I have difficulty visualizing how the code would work so maybe it's simpler than I'm imagining.

The decorator/registration, IMHO, is somewhat trivial and low complexity. https://github.com/pulp/pulp/pull/3708/files#diff-ef5897da44d0b954c133f8ecbd101410

My general opinion: I'm partial to Option B. I'm concerned about adding too much complexity (== possible sources of tricky bugs) just for the sake of purity. I could be convinced otherwise if the additional complexity would add a significant functional value, but I don't currently see that being the case.

I want to check in on the status of this. It sounds like we have 5 people who are either in favor or leaning toward option B, putting the permit method on ContentGuard. Is that enough consensus to proceed with option B?

daviddavis wrote:

I want to check in on the status of this. It sounds like we have 5 people who are either in favor or leaning toward option B, putting the permit method on ContentGuard. Is that enough consensus to proceed with option B?

Yes.