Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an Abstract Model #34

Closed
curtismorte opened this issue May 24, 2019 · 7 comments · Fixed by #36
Closed

Add an Abstract Model #34

curtismorte opened this issue May 24, 2019 · 7 comments · Fixed by #36
Milestone

Comments

@curtismorte
Copy link

Is your feature request related to a problem? Please describe.
For a multi-tenant database, the original model may need to be extended to include some type of foreign key relationship.

Describe the solution you'd like
It would be nice to have a class like AbstractAPIKey where we can make our own customizations to the default model.

Describe alternatives you've considered
Having a mixin that can be used, along with some documentation for subclassing helpers, admin, and permissions.

Additional context
N/A

@florimondmanca
Copy link
Owner

Hi @curtismorte! Thanks for bringing this up. I’m a bit busy over this weekend but will try to get a closer look at this soon.

I agree it’s an interesting point though, in fact we already discussed the potential difficulties of having the APIKey model involved in a FK or 1-1 relationship in #20.

What makes you think we may need an abstract model? Do you have any snippets to share that show that the current model is not sufficient to implement FKs, and maybe how the multi tenant DB comes into play?

Also if you’ve got any clues yet (eg about how the abstract model could look), feel free to share them here. :-)

Thanks

@curtismorte
Copy link
Author

The need for an abstract models allows for the architect to really customize the integration specific to their data model. Fundamentally, there are risks associated with this, but the mitigation of the risk ultimately falls on the architect.

For something like an API Key, it's a means of association and not verification. Since "association" can be a variety flavors, allowing the association to be extensible ultimately makes the package more useful. A couple of ways this could be extended are by associating the API key with a user or an account.

I'll make the argument for two use cases:

  1. The default setup as it exists currently
  2. An abstract model that allows for extensibility

Extending the model comes with tradeoffs, and should be well thought through by the architect. However, this decision shouldn't be defined by the package.

Thoughts?

@florimondmanca
Copy link
Owner

Since "association" can be a variety flavors, allowing the association to be extensible ultimately makes the package more useful.

Indeed!

Does the following snippet (associating API keys to another model, e.g. the User model) represent well enough the extensibility use case you're thinking about?

# myapp/models.py
from django.db import models
from django.contrib.auth.models import get_user_model
from rest_framework_api_key.models import BaseAPIKey

User = get_user_model()

class APIKey(BaseAPIKey):
    owner = models.ForeignKey(User, on_delete=models.CASCADE)

Having a mixin that can be used, along with some documentation for subclassing helpers, admin, and permissions.

Yes, it seems there's some additional work (docs / helpers) associated with extensibility. To be more precise:

  • How to associate API keys to another model / BaseAPIKey.
  • How to register the concrete API key model in the admin panel / BaseAPIKeyAdmin.
  • How to define and use a permission class for the concrete API key model / BaseHasAPIKey.

Do you think of anything else?

To start tackling this, I think we can start with the abstract BaseAPIKey model, and then add the other extensibility-enabling features.

@florimondmanca
Copy link
Owner

florimondmanca commented May 29, 2019

@curtismorte I pushed the beginning of something in #36, feel free to take a look.

Out of curiosity — have you considered whether multi-table inheritance could solve the extensibility use case we're aiming at here?

@curtismorte
Copy link
Author

@florimondmanca, you asked:
Does the following snippet (associating API keys to another model, e.g. the User model) represent well enough the extensibility use case you're thinking about?

# myapp/models.py
from django.db import models
from django.contrib.auth.models import get_user_model
from rest_framework_api_key.models import BaseAPIKey

User = get_user_model()

class APIKey(BaseAPIKey):
    owner = models.ForeignKey(User, on_delete=models.CASCADE)

Yes, a model mixin just like the snippet you provided does exactly what's needed IMO. The default package can use the mixin within the generic model class or it can be mixed in with another application specific model.


You also asked:
Yes, it seems there's some additional work (docs / helpers) associated with extensibility. To be more precise:

  • How to associate API keys to another model / BaseAPIKey.
  • How to register the concrete API key model in the admin panel / BaseAPIKeyAdmin.
  • How to define and use a permission class for the concrete API key model / BaseHasAPIKey.

Do you think of anything else?

Not at this time. When I was first inspecting the package, I noticed model dependency within permissions.py and admin.py. It seems that you could take the same mixin approach for permissions and admin as you would need to do for the model.

For example, being able to declare an attribute that the permissions mixin would rely only can solve the model dependency problem there.

class ViewsetPermission(HasAPIKeyMixin):
    model = CustomModelWithBaseAPIKey

    def has_permission(self, request, view) -> bool:
        key = _get_key(request)

        if not key:
            return False

        return self.model.objects.is_valid(key)

    def has_object_permission(self, request, view, obj):
        return self.has_permission(request, view)

For what it's worth, I think the admin area is a bit of a different problem because there is likely not a common use case other than the generic use case you provide with the package by default. Model mixins place the implementation of various admin use cases on the developer. I think the solution here is a nice wiki page which explains everything a developer would need to know to customize their admin experience specific to the package codebase.

@curtismorte
Copy link
Author

curtismorte commented Jun 6, 2019

Out of curiosity — have you considered whether multi-table inheritance could solve the extensibility use case we're aiming at here?

It could solve the extensibility use case. However, it makes the assumption that you want relationships to each key as opposed to being able to add fields to the model itself.

Those are the two common use cases IMO.

This was referenced Jun 12, 2019
@florimondmanca florimondmanca added this to the 1.3 milestone Jun 14, 2019
@florimondmanca
Copy link
Owner

@curtismorte I tracked the customization of permissions in #43 and admin in #45. Updated the roadmap in #42 as well. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants