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

gateway(sharing-ng): Check for remaing space manager before removing grant #4585

Merged
merged 1 commit into from
Mar 22, 2024

Conversation

rhafer
Copy link
Contributor

@rhafer rhafer commented Mar 18, 2024

This check is already done by the ocs sharing implementaion, but for the move to the graph API base sharing implementation we'd want to have it in a more central place.

@rhafer rhafer marked this pull request as ready for review March 18, 2024 16:38
@rhafer rhafer requested review from labkode, glpatcern and a team as code owners March 18, 2024 16:38
@rhafer rhafer self-assigned this Mar 18, 2024
…grant

This check is already done by the ocs sharing implementaion, but for the move
to the graph API base sharing implementation we'd want to have it in a more
central place.
Copy link
Contributor

@kobergj kobergj left a comment

Choose a reason for hiding this comment

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

The code looks fine, but are we really sure we want to do this? This is moving business logic to the gateway service. Previously that was a no-go as gateway should be dumb. Did this change?

cc @butonic

@rhafer
Copy link
Contributor Author

rhafer commented Mar 20, 2024

The code looks fine, but are we really sure we want to do this? This is moving business logic to the gateway service. Previously that was a no-go as gateway should be dumb. Did this change?

I kind of discussed this already with @butonic . It's not optimal but it is for sure better than the checks duplicated at the higher levels (i.e. ocs and graph). Any idea what would be a better place for those checks. The usersharepovider in the gateway is already full of business logic because it needs to deal with the differences between space permissions and "real" shares. This change falls into the same category.

@butonic
Copy link
Contributor

butonic commented Mar 20, 2024

Services infront of CS3 should just be protocol adapters. The business logic should be behind the CS3 gateway facade. It can be in the gateway or in a driver. From the outside that should be irrelevant.

If we actually split the CS3 API into two parts:

  1. an actively managed space registry and
  2. a storage provider
    the gateway would be deployed twice. Actually, it might become obsolete and the functionality could move to middle wares and interceptors. 🤔 At least conceptually that should be the architecture. A real deployment would share the gateway and deploy it together with the space registry and storage provider.

Note that the CS3 API just defines Interfaces it does not say every API should be deployed in a dedicated service. It is meant to swap out implementations of clearly defined APIs.

@rhafer rhafer merged commit bca5e83 into cs3org:edge Mar 22, 2024
9 checks passed
rhafer added a commit to rhafer/ocis that referenced this pull request Mar 22, 2024
@micbar micbar mentioned this pull request Jun 19, 2024
24 tasks
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.

3 participants