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

Support WOPI-Validator from M365 for the Web #182

Open
dviry opened this issue Feb 28, 2025 · 2 comments
Open

Support WOPI-Validator from M365 for the Web #182

dviry opened this issue Feb 28, 2025 · 2 comments
Assignees

Comments

@dviry
Copy link
Collaborator

dviry commented Feb 28, 2025

Motivation

  • Why is this feature required?

-> in order to officially submit a WOPI host to be used by "M365 for the Web", the WOPI host must succeed specified tests in Microsoft 365 for the web integration, partly using automated tests by the WOPI-Validator

  • Is your feature request related to a problem? What problem?

-> currently not all tests are passed successfully

Describe the solution you'd like

-> implement/fix WopiHost endpoints to pass the "interactive open-source CLI WOPI-Validator" tool

Additional context

-> will create sub-issues for each "test group"

@dviry dviry self-assigned this Feb 28, 2025
@dviry
Copy link
Collaborator Author

dviry commented Mar 1, 2025

While implementing this, a couple of topics popped up that I would like to discuss/implement:

  • async methods - IMO all the services should use Task methods: IWopiLockProvider, IWopiSecurityHandler (done) and IWopiStorageProvider
  • IMO the models IWopiFile, IWopiFolder should be concrete classes (or records) - only services (lockProvider, securityHandler and storageProvider) should be defined as interfaces:
    • would be easier to test without having to Mock all properties
    • would be possible to mark required properties
    • would be easier to set "default" properties' values
    • would be easier (and compiled faster) to extend (OOP)
  • now that we use IOptions<> everywhere, we need to "move" some to the correct place depending on responsibility; e.g. WopiHostOptions.StorageProviderAssemblyName and .LockProviderAssemblyName should not be within WopiHost.Core but rather in WopiHost sample project as loading/registering DI is a responsibility of the deployment and not of the Core library (e.g. what if I want to implement the IWopiLockProvider, IWopiSecurityHandler and IWopiStorageProvider in the same assembly?)
  • I would like to create more "samples" for the WopiHost (different providers and integrations) and the WopiHost.Web (minimal Razor pages instead of bloated MVC) - what do you think? Maybe rename the existing ones to WopiHost.Samples.* and/or move to a new subfolder (I know when I started I was having difficulties understanding the difference/usage betwen WopiHost and WopiHost.Web)
  • the WopiHost.Discovery project needs refactoring - searching through an in-memory xml is both slow and error-prone - I am wondering if this here would be a better approach
  • I would like to replace all authorizationService.AuthorizeAsync() calls in the Controllers to an attribute-based authorization to reduce code bloat (IMO security should be implemented as Infrastructure when possible and not be "in the way") - would also be easier to unit test them

Pending @petrsvihlik feedback, I would implement those gradually while solving the current sub-issues in this ticket.

@petrsvihlik
Copy link
Owner

  • async methods - agreed
  • IWopiFile, IWopiFolder - so you would init them from outside (differently per storage type), do I get that right? I'd be fine with that
  • WopiHostOptions - you are correct
  • WopiHost.Samples - agreed...and you are not alone; I myself got confused couple of times :D lol
  • WopiHost.Discovery - let's create a separate issue and discuss this async. the proposed approach could work but maybe we can figure out sth even better...like caching all the URL templates, etc. on init or sth like that
  • attribute-based authorization & authnz as infra - can't agree more. what would that require? a custom attribute? if so, go for it

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

No branches or pull requests

2 participants