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

feat: restrict information_system from facets for unauthenticated #416

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nicobav
Copy link
Contributor

@nicobav nicobav commented Dec 20, 2024

There are also information system information in search facets which are to be restricted from the unauthenticated users.

Removes InformationSystem attributes from facets data for unauthenticated users.

Refs TIED-171

@nicobav nicobav requested a review from a team December 20, 2024 12:52
@terovirtanen
Copy link
Contributor

TIEDONOHJAUS-API branch is deployed to platta: https://tiedonohjaus-pr416.api.dev.hel.ninja 🚀🚀🚀

@@ -28,13 +28,23 @@ class FacetedAttributeBackend(BaseFilterBackend, FilterBackendMixin):
faceted_search_param = "facet_attribute"

@staticmethod
def get_attributes(documents: List[Type[Document]] = None) -> list:
def get_attributes(
documents: List[Type[Document]] = None, authenticated=True
Copy link
Contributor

Choose a reason for hiding this comment

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

List and Type have been deprecated since 3.9:
https://docs.python.org/3.9/library/typing.html#typing.Type
https://docs.python.org/3.9/library/typing.html#typing.List

Suggested change
documents: List[Type[Document]] = None, authenticated=True
documents: list[type[Document]] = None, authenticated=True

@voneiden voneiden self-requested a review December 20, 2024 13:09
There are also information system information in search facets which
are to be restricted from the unauthenticated users.

Removes InformationSystem attributes from facets data for
unauthenticated users.

Refs TIED-171
@nicobav nicobav force-pushed the TIED-171/infsysinfacets branch from d1335d6 to 03bf689 Compare December 20, 2024 13:17
@@ -28,13 +26,23 @@ class FacetedAttributeBackend(BaseFilterBackend, FilterBackendMixin):
faceted_search_param = "facet_attribute"

@staticmethod
def get_attributes(documents: List[Type[Document]] = None) -> list:
def get_attributes(
documents: list[type[Document]] = None, authenticated=True
Copy link
Contributor

Choose a reason for hiding this comment

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

authenticated=True

  • defaulting optional flag related to authentication to true is a bit meh
  • "authenticated" does not tell the caller what it does, compared for example to something like "include_information_system"

Copy link
Contributor

Choose a reason for hiding this comment

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

While you're at it, this is a boolean trap. Use def get_attributes(documents: ..., *, authenticated=True) instead (or whatever name/value you settle on)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Valid argument. I'd go with include_information_system

Comment on lines +88 to +90
self.attributes = FacetedAttributeBackend.get_attributes(
authenticated=False
)
Copy link
Contributor

Choose a reason for hiding this comment

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

If it doesn't cause any issues, would be nice to have if/else branch here for both authenticated and unauthenticated. I got a bit paranoid about this somehow ending up overwriting the class attribute BaseSearchDocumentViewSet.attributes, but yeah that shouldn't happen of course because the instance copies the class attributes as instance attributes.

@terovirtanen
Copy link
Contributor

TIEDONOHJAUS-API branch is deployed to platta: https://tiedonohjaus-pr416.api.dev.hel.ninja 🚀🚀🚀

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 this pull request may close these issues.

4 participants