feat(OCM-invites): Implementation of invitation flow for OCM 1.1.0

[mickenordin]

• Resolves: 🤝 Implement OCM v1.1 + 1.2 #38886

Summary

This patchset implements the /invite-accepted endpoint

https://cs3org.github.io/OCM-API/docs.html?branch=v1.1.0&repo=OCM-API&user=cs3org#/paths/~1invite-accepted/post

A companion PR with UI for creating and otherwise lifecycle manage OCM invites is in the works for the Contacts app, but that work needs this first.

[nickvergessen]

Where is this table written?
I only see a single SELECT and an UPDATE, but no INSERT?

[mickenordin]

Where is this table written? I only see a single SELECT and an UPDATE, but no INSERT?

The ui part of this, where you can create and delete invites are coming in a companion pr for the contacts app.

[mickenordin]

When working on the front end part, I realized we need an event to react to, so I added that.

[mickenordin]

@ArtificialOwl @miaulalala who can merge the PR now, if all the requirements are met?

[AndyScherzinger]

@mickenordin @ArtificialOwl @sorbaugh

accordign to @SystemKeeper this breaks talk federation at the moment, as IOCMProvider was replaced by ICapabilityAwareOCMProvider. Can someone tell me if that is intended to break? I see IOCMProvider used in talk, circles and groupfolders and it is currently not documented at https://docs.nextcloud.com/server/latest/developer_manual/app_publishing_maintenance/app_upgrade_guide/upgrade_to_32.html

"replaced".. at least the alias was removed: https://github.com/nextcloud/server/pull/51113/files#diff-b868f3fc169b78b9222fb4822eba2940dc6eccb4b8ea9df464366be45f059ef8

Accordign to @st3iny the old alias should stay. Should be fine since the new interface extends the old one: interface ICapabilityAwareOCMProvider extends IOCMProvider {

Perhaps add a deprecation notice to IOCMProvider?

Also cc @icewind1991 and @nickvergessen given the impacted apps.

[mickenordin]

There was not an intention for it to break. So the proposed solution is to revert this change on https://github.com/mickenordin/server/blob/623f2f0240016bbc142387741a44864ecb0458e2/lib/private/Server.php#L1275?

-		$this->registerAlias(IOCMProvider::class, OCMProvider::class);
+		$this->registerAlias(ICapabilityAwareOCMProvider::class, OCMProvider::class);

The thinking was that this would work, since every ICapabilityAwareOCMProvider is an IOCMProvider, but maybe I thought about it backwards, and the above change should just be reverted?

[mickenordin]

Relevant comments are here:

#51113 (comment)
#51113 (comment)
#51113 (comment)

[st3iny]

The thinking was that this would work, since every ICapabilityAwareOCMProvider is an IOCMProvider, but maybe I thought about it backwards, and the above change should just be reverted?

That is true but some apps try to inject an IOCMProvider into contstructors. This will now fail because the server container does not know how to resolve IOCMProvider anymore. This can be considered a breaking change as apps need to manually adjust their code to inject a ICapabilityAwareOCMProvider instead and we try to prevent that.

If the new interface is supposed to replace the old one, a good way would be to deprecated the old interface and remove it later. To give apps some more time to react.

I'm no DI expert but it should be possible to register both aliases at the same time. To not break existing apps but also give the opportunity to make use of the new functionality.

---

Example from Talk/Spreed:

	public function __construct(
		protected Config $talkConfig,
		protected IOCMProvider $provider,
		protected CloudFederationProviderTalk $talkProvider,
	) {
	}

Will fail when ran against server master.

[mickenordin]

I'm no DI expert but it should be possible to register both aliases at the same time. To not break existing apps but also give the opportunity to make use of the new functionality.

Can they have the same name? I.e, can we do:

		$this->registerAlias(IOCMProvider::class, OCMProvider::class);
		$this->registerAlias(ICapabilityAwareOCMProvider::class, OCMProvider::class);

[st3iny]

Can they have the same name? I.e, can we do:

		$this->registerAlias(IOCMProvider::class, OCMProvider::class);
		$this->registerAlias(ICapabilityAwareOCMProvider::class, OCMProvider::class);

Yes, that should work.

I have another question: Is there a reason to split the logic in to two interfaces and keep both? Another possibility to just add the new methods from ICapabilityAwareOCMProvider to IOCMProvider. Do you see a reason why this would be a bad idea? (Asking because I lack a bit of context about the PR.)

[mickenordin]

Can they have the same name? I.e, can we do:

		$this->registerAlias(IOCMProvider::class, OCMProvider::class);
		$this->registerAlias(ICapabilityAwareOCMProvider::class, OCMProvider::class);

Yes, that should work.

I have another question: Is there a reason to split the logic in to two interfaces and keep both? Another possibility to just add the new methods from ICapabilityAwareOCMProvider to IOCMProvider. Do you see a reason why this would be a bad idea? (Asking because I lack a bit of context about the PR.)

It was on the advice of @miaulalala that we did that, originally that was the way it was done. Anna was worried that doing it that way would break backwards compatibility as you can see from the most recent comments above, before it was merged.

[mickenordin]

If we can quickly decide on the way we want to move forward, I can draft a PR for a fix.

[mickenordin]

Adding both seems the easiest thing to do, so I created a PR for that.

[nickvergessen]

This is a breaking change. The old and new alias need to be in place

[AndyScherzinger]

#53465