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

[ENG-5196] addon capabilities (in api and implementers' toolkit) #12

Closed
wants to merge 20 commits into from
8 changes: 6 additions & 2 deletions addon_service/authorized_storage_account/models.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
# from django.contrib.postgres.fields import ArrayField
from django.contrib.postgres.fields import ArrayField
from django.db import models

from addon_service.capability.models import IntStorageCapability
from addon_service.common.base_model import AddonsServiceBaseModel


class AuthorizedStorageAccount(AddonsServiceBaseModel):
# TODO: authorized_capabilities = ArrayField(...)
authorized_capabilities = ArrayField(
models.IntegerField(choices=IntStorageCapability.as_django_choices()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: would rather enforce this differently, since setting it in choices requires migrations when options change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair enough; how about a validator?

)

default_root_folder = models.CharField(blank=True)

external_storage_service = models.ForeignKey(
Expand Down
5 changes: 5 additions & 0 deletions addon_service/authorized_storage_account/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
)
from rest_framework_json_api.utils import get_resource_type_from_model

from addon_service.capability.serializers import StorageCapabilityListField
from addon_service.models import (
AuthorizedStorageAccount,
ConfiguredStorageAddon,
Expand Down Expand Up @@ -43,6 +44,7 @@ def __init__(self, *args, **kwargs):
url = serializers.HyperlinkedIdentityField(
view_name=f"{RESOURCE_NAME}-detail", required=False
)
authorized_capabilities = StorageCapabilityListField()
account_owner = AccountOwnerField(
many=False,
queryset=UserReference.objects.all(),
Expand Down Expand Up @@ -74,6 +76,7 @@ def __init__(self, *args, **kwargs):

def create(self, validate_data):
account_owner = validate_data["account_owner"]
authorized_capabilities = validate_data["authorized_capabilities"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can we change validate_data -> validated_data here? feels weird to have a variable with a verb clause as a name (I think my brain just autocorrected this when it first made it in)

external_storage_service = validate_data["external_storage_service"]
# TODO(ENG-5189): Update this once credentials format is finalized
credentials, created = ExternalCredentials.objects.get_or_create(
Expand All @@ -90,6 +93,7 @@ def create(self, validate_data):
return AuthorizedStorageAccount.objects.create(
external_storage_service=external_storage_service,
external_account=external_account,
authorized_capabilities=authorized_capabilities,
)

class Meta:
Expand All @@ -102,4 +106,5 @@ class Meta:
"external_storage_service",
"username",
"password",
"authorized_capabilities",
]
Empty file.
10 changes: 10 additions & 0 deletions addon_service/capability/models.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
from addon_service.common.enums import IntEnumForEnum
from addon_toolkit.storage import StorageCapability


__all__ = ("IntStorageCapability",)


class IntStorageCapability(IntEnumForEnum, base_enum=StorageCapability):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose between the dual enums? Feels very complicated. On OSF, I was able to handle the translation from int to string for the API with a single enum here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm maybe it is too complicated, it sure did get finicky... bloomed from a conviction that addon_toolkit should be completely unaware of addon_service's database details like storing enum as int, but also that addon_toolkit.storage should define the source-of-truth StorageCapability enum, and then maybe got overexcited about having a use for __init_subclass__ kwargs

also seemed nice code-aroma to use enums as explicit source-controlled maps between a python-name and both api and database representations -- the single-enum requires the api representation to be the python name... which may be fine, really

for the operations api i did further complicate the idea, but may have better expressed intent?

ACCESS = 1
UPDATE = 2
11 changes: 11 additions & 0 deletions addon_service/capability/serializers.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
from addon_service.capability.models import IntStorageCapability
from addon_service.common.enums.serializers import DualEnumsListField
from addon_toolkit.storage import StorageCapability


class StorageCapabilityListField(
DualEnumsListField,
external_enum=StorageCapability,
internal_enum=IntStorageCapability,
):
pass
1 change: 1 addition & 0 deletions addon_service/common/base_model.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ def save(self, *args, **kwargs):
if not self.id:
self.created = timezone.now()
self.modified = timezone.now()
self.full_clean()
super().save(*args, **kwargs)

def __str__(self):
Expand Down
29 changes: 29 additions & 0 deletions addon_service/common/enums/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import enum
from typing import ClassVar


def same_enum_names(enum_a: type[enum.Enum], enum_b: type[enum.Enum]) -> bool:
# ensure enums have same names
_names_a = {_item.name for _item in enum_a}
_names_b = {_item.name for _item in enum_b}
return _names_a == _names_b


class IntEnumForEnum(enum.IntEnum):
__base_enum: ClassVar[type[enum.Enum]]

def __init_subclass__(cls, /, base_enum: type[enum.Enum], **kwargs):
super().__init_subclass__(**kwargs)
assert same_enum_names(base_enum, cls)
cls.__base_enum = base_enum

@classmethod
def to_int(cls, base_enum_member):
return cls[base_enum_member.name]

@classmethod
def as_django_choices(cls):
return [(int(_item), _item.name) for _item in cls]

def to_base_enum(self) -> enum.Enum:
return self.__base_enum[self.name]
50 changes: 50 additions & 0 deletions addon_service/common/enums/serializers.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import enum
from typing import ClassVar

from rest_framework_json_api import serializers

from addon_service.common.enums import same_enum_names


class DualEnumsListField(serializers.MultipleChoiceField):
"""use one enum in your database and another in your api!"""

__internal_enum: ClassVar[type[enum.Enum]]
__external_enum: ClassVar[type[enum.Enum]]

def __init__(self, **kwargs):
super().__init__(
**kwargs,
choices={ # valid serialized values come from the external enum
_external_member.value for _external_member in self.__external_enum
},
)

def __init_subclass__(
cls,
/,
internal_enum: type[enum.Enum],
external_enum: type[enum.Enum],
**kwargs,
):
super().__init_subclass__(**kwargs)
assert same_enum_names(internal_enum, external_enum)
cls.__internal_enum = internal_enum
cls.__external_enum = external_enum

def to_internal_value(self, data) -> list[enum.Enum]:
_names: set = super().to_internal_value(data)
return [self._to_internal_enum_member(_name) for _name in _names]

def to_representation(self, value):
_member_list = super().to_representation(value)
return {self._to_external_enum_value(_member) for _member in _member_list}

def _to_internal_enum_member(self, external_value) -> enum.Enum:
_external_member = self.__external_enum(external_value)
return self.__internal_enum[_external_member.name]

def _to_external_enum_value(self, internal_value: enum.Enum):
_internal_member = self.__internal_enum(internal_value)
_external_member = self.__external_enum[_internal_member.name]
return _external_member.value
21 changes: 20 additions & 1 deletion addon_service/configured_storage_addon/models.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,17 @@
from django.contrib.postgres.fields import ArrayField
from django.core.exceptions import ValidationError
from django.db import models

from addon_service.capability.models import IntStorageCapability
from addon_service.common.base_model import AddonsServiceBaseModel


class ConfiguredStorageAddon(AddonsServiceBaseModel):
root_folder = models.CharField()
root_folder = models.CharField(blank=True)

connected_capabilities = ArrayField(
models.IntegerField(choices=IntStorageCapability.as_django_choices()),
)

base_account = models.ForeignKey(
"addon_service.AuthorizedStorageAccount",
Expand All @@ -28,3 +35,15 @@ class JSONAPIMeta:
@property
def account_owner(self):
return self.base_account.external_account.owner

def clean(self):
_connected_caps = set(self.connected_capabilities)
if not _connected_caps.issubset(self.base_account.authorized_capabilities):
_unauthorized_caps = _connected_caps.difference(
self.base_account.authorized_capabilities
)
raise ValidationError(
{
"connected_capabilities": f"capabilities not authorized on account: {_unauthorized_caps}",
}
)
3 changes: 3 additions & 0 deletions addon_service/configured_storage_addon/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from rest_framework_json_api.relations import ResourceRelatedField
from rest_framework_json_api.utils import get_resource_type_from_model

from addon_service.capability.serializers import StorageCapabilityListField
from addon_service.models import (
AuthorizedStorageAccount,
ConfiguredStorageAddon,
Expand All @@ -23,6 +24,7 @@ def to_internal_value(self, data):
class ConfiguredStorageAddonSerializer(serializers.HyperlinkedModelSerializer):
root_folder = serializers.CharField(required=False)
url = serializers.HyperlinkedIdentityField(view_name=f"{RESOURCE_NAME}-detail")
connected_capabilities = StorageCapabilityListField()
base_account = ResourceRelatedField(
queryset=AuthorizedStorageAccount.objects.all(),
many=False,
Expand All @@ -48,4 +50,5 @@ class Meta:
"root_folder",
"base_account",
"authorized_resource",
"connected_capabilities",
]
4 changes: 0 additions & 4 deletions addon_service/external_account/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,6 @@


class ExternalAccount(AddonsServiceBaseModel):
# The user's ID on the remote service
remote_account_id = models.CharField()
remote_account_display_name = models.CharField()

credentials_issuer = models.ForeignKey(
"addon_service.CredentialsIssuer",
on_delete=models.CASCADE,
Expand Down
2 changes: 0 additions & 2 deletions addon_service/management/commands/fill_garbage.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@ def handle_label(self, label, **options):
)
_ec = db.ExternalCredentials.objects.create()
_ea = db.ExternalAccount.objects.create(
remote_account_id=label,
remote_account_display_name=label,
credentials_issuer=_ci,
owner=_iu,
credentials=_ec,
Expand Down
31 changes: 24 additions & 7 deletions addon_service/migrations/0001_initial.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# Generated by Django 4.2.7 on 2023-12-11 20:02
# Generated by Django 4.2.7 on 2024-02-07 22:04

import django.contrib.postgres.fields
import django.db.models.deletion
from django.db import (
migrations,
Expand Down Expand Up @@ -27,6 +28,15 @@ class Migration(migrations.Migration):
),
("created", models.DateTimeField(editable=False)),
("modified", models.DateTimeField()),
(
"authorized_capabilities",
django.contrib.postgres.fields.ArrayField(
base_field=models.IntegerField(
choices=[(1, "ACCESS"), (2, "UPDATE")]
),
size=None,
),
),
("default_root_folder", models.CharField(blank=True)),
],
options={
Expand Down Expand Up @@ -167,8 +177,6 @@ class Migration(migrations.Migration):
),
("created", models.DateTimeField(editable=False)),
("modified", models.DateTimeField()),
("remote_account_id", models.CharField()),
("remote_account_display_name", models.CharField()),
(
"credentials",
models.ForeignKey(
Expand Down Expand Up @@ -215,19 +223,28 @@ class Migration(migrations.Migration):
("modified", models.DateTimeField()),
("root_folder", models.CharField()),
(
"base_account",
"connected_capabilities",
django.contrib.postgres.fields.ArrayField(
base_field=models.IntegerField(
choices=[(1, "ACCESS"), (2, "UPDATE")]
),
size=None,
),
),
(
"authorized_resource",
models.ForeignKey(
on_delete=django.db.models.deletion.CASCADE,
related_name="configured_storage_addons",
to="addon_service.authorizedstorageaccount",
to="addon_service.resourcereference",
),
),
(
"authorized_resource",
"base_account",
models.ForeignKey(
on_delete=django.db.models.deletion.CASCADE,
related_name="configured_storage_addons",
to="addon_service.resourcereference",
to="addon_service.authorizedstorageaccount",
),
),
],
Expand Down
8 changes: 5 additions & 3 deletions addon_service/tests/_factories.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from factory.django import DjangoModelFactory

from addon_service import models as db
from addon_service.capability.models import IntStorageCapability


class UserReferenceFactory(DjangoModelFactory):
Expand All @@ -22,6 +23,8 @@ class CredentialsIssuerFactory(DjangoModelFactory):
class Meta:
model = db.CredentialsIssuer

name = factory.Faker("word")


class ExternalCredentialsFactory(DjangoModelFactory):
class Meta:
Expand All @@ -32,9 +35,6 @@ class ExternalAccountFactory(DjangoModelFactory):
class Meta:
model = db.ExternalAccount

remote_account_id = factory.Faker("word")
remote_account_display_name = factory.Faker("word")

credentials_issuer = factory.SubFactory(CredentialsIssuerFactory)
owner = factory.SubFactory(UserReferenceFactory)
credentials = factory.SubFactory(ExternalCredentialsFactory)
Expand All @@ -59,6 +59,7 @@ class Meta:
model = db.AuthorizedStorageAccount

default_root_folder = "/"
authorized_capabilities = factory.List([IntStorageCapability.ACCESS])
external_storage_service = factory.SubFactory(ExternalStorageServiceFactory)
external_account = factory.SubFactory(ExternalAccountFactory)
# TODO: external_account.credentials_issuer same as
Expand All @@ -70,5 +71,6 @@ class Meta:
model = db.ConfiguredStorageAddon

root_folder = "/"
connected_capabilities = factory.List([IntStorageCapability.ACCESS])
base_account = factory.SubFactory(AuthorizedStorageAccountFactory)
authorized_resource = factory.SubFactory(ResourceReferenceFactory)
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,13 @@ def test_get(self):
set(_content["data"]["attributes"].keys()),
{
"default_root_folder",
"authorized_capabilities",
},
)
self.assertEqual(
_content["data"]["attributes"]["authorized_capabilities"],
["access"],
)
self.assertEqual(
set(_content["data"]["relationships"].keys()),
{
Expand Down Expand Up @@ -187,6 +192,7 @@ def test_post(self):
"attributes": {
"username": "<placeholder-username>",
"password": "<placeholder-password>",
"authorized_capabilities": ["access", "update"],
},
"relationships": {
"external_storage_service": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,13 @@ def test_get(self):
set(_content["data"]["attributes"].keys()),
{
"root_folder",
"connected_capabilities",
},
)
self.assertEqual(
_content["data"]["attributes"]["connected_capabilities"],
["access"],
)
self.assertEqual(
set(_content["data"]["relationships"].keys()),
{
Expand Down Expand Up @@ -155,6 +160,9 @@ def setUpTestData(cls):
cls.default_payload = {
"data": {
"type": "configured-storage-addons",
"attributes": {
"connected_capabilities": ["access"],
},
"relationships": {
"base_account": {
"data": {
Expand Down
Loading