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(api): authorization extended for soft-delete and suspend #12158

Merged

Conversation

david-leifker
Copy link
Collaborator

@david-leifker david-leifker commented Dec 18, 2024

feat(api): authorization extended for soft-delete and suspended

  • Key Aspects Always Returned
    • Fix AspectRetriever behavior with respect to key aspects always returned
    • Legacy use of the EntityClient and restli will by default continue to return key aspects regardless for backwards compatibility
  • Deprecated active boolean flag in corpUserInfo not supported
  • Fix Global exception handler
  • Add CachingAspectRetriever to OperationContext
  • Increase corp user aspect caching to 5 min (corpUserKey, status, corpUserStatus)

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
  • For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

@github-actions github-actions bot added product PR or Issue related to the DataHub UI/UX devops PR or Issue related to DataHub backend & deployment smoke_test Contains changes related to smoke tests labels Dec 18, 2024
@datahub-cyborg datahub-cyborg bot added the needs-review Label for PRs that need review from a maintainer. label Dec 18, 2024
@david-leifker david-leifker force-pushed the base-authorization-check branch from 8b74c9d to 45f00d5 Compare December 18, 2024 05:24
@david-leifker david-leifker force-pushed the base-authorization-check branch from 45f00d5 to 6d03e03 Compare December 18, 2024 16:08
* fix AspectRetriever behavior with respect to key aspects always returned
* Deprecated active boolean in corpUserInfo not supported
@david-leifker david-leifker force-pushed the base-authorization-check branch from 6d03e03 to 441003b Compare December 18, 2024 16:16
@@ -22,7 +22,7 @@

@Getter
@Builder
public class EntityServiceAspectRetriever implements CachingAspectRetriever {
public class EntityServiceAspectRetriever implements AspectRetriever {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This impl is not caching, corrected interface.

* @param aspectRetriever aspect retriever - ideally the SystemEntityClient backed one for caching
* @return active status
*/
public boolean isActive(AspectRetriever aspectRetriever) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Primary logic for evaluation of the actor's state.

@@ -177,7 +209,7 @@ public static OperationContext asSystem(
@Nonnull private final EntityRegistryContext entityRegistryContext;
@Nullable private final ServicesRegistryContext servicesRegistryContext;
@Nullable private final RequestContext requestContext;
@Nullable private final RetrieverContext retrieverContext;
@Nonnull private final RetrieverContext retrieverContext;
Copy link
Collaborator Author

@david-leifker david-leifker Dec 18, 2024

Choose a reason for hiding this comment

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

Due to the importance of this context for actor state validation, it is no longer optional.

@@ -90,7 +91,7 @@ public Task<EntityResponse> get(
? opContext.getEntityAspectNames(entityName)
: new HashSet<>(Arrays.asList(aspectNames));
try {
return _entityService.getEntityV2(opContext, entityName, urn, projectedAspects);
return _entityService.getEntityV2(opContext, entityName, urn, projectedAspects, alwaysIncludeKeyAspect == null || alwaysIncludeKeyAspect);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rest.li does have default values, but this works too 😄

@@ -522,12 +522,12 @@ cache:
entityAspectTTLSeconds:
# cache user aspects for 20s
corpuser:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Effects the CachingAspectRetriever

@@ -1,9 +1,11 @@
package io.datahubproject.openapi;
package io.datahubproject.openapi.config;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Important relocation here, previously it was not being picked up.

@@ -0,0 +1,54 @@
package io.datahubproject.openapi.operations.test;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added this for smoke-test

@@ -490,45 +492,3 @@ def getAccessTokenMetadata(session, token):
response.raise_for_status()

return response.json()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Move to common util

@@ -0,0 +1,173 @@
import os
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Primary test case for the feat.

@datahub-cyborg datahub-cyborg bot added pending-submitter-response Issue/request has been reviewed but requires a response from the submitter and removed needs-review Label for PRs that need review from a maintainer. labels Dec 18, 2024
@datahub-cyborg datahub-cyborg bot added pending-submitter-merge and removed pending-submitter-response Issue/request has been reviewed but requires a response from the submitter labels Dec 18, 2024
@david-leifker david-leifker merged commit 8c724db into datahub-project:master Dec 18, 2024
99 of 105 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devops PR or Issue related to DataHub backend & deployment pending-submitter-merge product PR or Issue related to the DataHub UI/UX smoke_test Contains changes related to smoke tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants