[#61178] Update RemoteIdentity after token obtained.

[ba1ash]

Ticket

https://community.openproject.org/wp/61178
https://community.openproject.org/wp/61291

What are you trying to accomplish?

With this PR I try to make RemoteIdentity more flexible and being able to store user identities for combination of auth_source: oauth_client or oidc provider and integration: storage

What approach did you choose and why?

The goal is accomplished by adding two polymorphic associations to RemoteIdentity: integration and auth_source.

Merge checklist

• Updated tests.
• Tested with Chromium

[NobodysNightmare]

Something that I just found during integration testing. We also need to update modules/storages/app/services/storages/nextcloud_managed_folder_sync_service.rb:

diff --git a/modules/storages/app/services/storages/nextcloud_managed_folder_sync_service.rb b/modules/storages/app/services/storages/nextcloud_managed_folder_sync_service.rb
index 78e27cc34ae..76e34852db6 100644
--- a/modules/storages/app/services/storages/nextcloud_managed_folder_sync_service.rb
+++ b/modules/storages/app/services/storages/nextcloud_managed_folder_sync_service.rb
@@ -318,7 +318,7 @@ def active_project_storages_scope
     end
 
     def remote_identities_scope
-      RemoteIdentity.includes(:user).where(oauth_client: @storage.oauth_client)
+      RemoteIdentity.includes(:user).where(integration: @storage)
     end
 
     def auth_strategy
@@ -326,7 +326,7 @@ def auth_strategy
     end
 
     def admin_remote_identities_scope
-      RemoteIdentity.includes(:user).where(oauth_client: @storage.oauth_client, user: User.admin.active)
+      RemoteIdentity.includes(:user).where(integration: @storage, user: User.admin.active)
     end
   end
 end

This makes sure that we add all users with a remote identity for the given storage to the Nextcloud group.

Probably the same for OneDriveManagedFolderSyncService, even though there we do not yet officially support anything else than oauth clients.

[NobodysNightmare]

One more:

diff --git a/modules/storages/app/common/storages/peripherals/storage_interaction/nextcloud/util.rb b/modules/storages/app/common/storages/peripherals/storage_interaction/nextcloud/util.rb
index 2888c25fd8e..08b99c4f18c 100644
--- a/modules/storages/app/common/storages/peripherals/storage_interaction/nextcloud/util.rb
+++ b/modules/storages/app/common/storages/peripherals/storage_interaction/nextcloud/util.rb
@@ -71,7 +71,7 @@ def origin_user_id(caller:, storage:, auth_strategy:)
               when :basic_auth
                 ServiceResult.success(result: storage.username)
               when :oauth_user_token, :sso_user_token
-                origin_user_id = RemoteIdentity.where(user_id: auth_strategy.user, oauth_client: storage.oauth_client)
+                origin_user_id = RemoteIdentity.where(user_id: auth_strategy.user, integration: storage)
                                                .pick(:origin_user_id)
                 if origin_user_id.present?
                   ServiceResult.success(result: origin_user_id)

[github-actions]

Caution

The provided work package version does not match the core version

Details:

• Work package URL: https://community.openproject.org/wp/61178
• Work package version: not set
• Core version: 15.4.0

Please make sure that:

• The work package version OR your pull request target branch is correct

[NobodysNightmare]

🟡 I am really not sure what's the right thing to do here.

I am pretty sure that there will be code (possibly touched in this PR), that will assume there is exactly one user per integration. Because uniqueness per storage is what we effectively had before.

Thus it would make sense to keep it that way:

Suggested change

	validates :user, uniqueness: { scope: %i[auth_source integration] }
	validates :user, uniqueness: { scope: :integration }

However, this would be a problem for users migrating to SSO if either of these is true:

• code relies on looking up the user with their current auth source
• the origin user id is different between SSO and the old authentication

On the other hand: Allowing to migrate has been extracted to a separate feature, so being more restrictive might be okay for now.

What do you think?

[ba1ash]

I think we should go with auth_source integration scope. I know one place to update where we should be explicit about auth_source. That is modules/storages/app/common/storages/peripherals/storage_interaction/nextcloud/util.rb. I suggest there we have something like:

              when :oauth_user_token, :sso_user_token
                auth_source = if auth_strategy.key == :oauth_user_token
                                storage.oauth_client
                              else
                                user.authentication_provider
                              end
                origin_user_id = RemoteIdentity.where(user_id: auth_strategy.user,
                                                      auth_source:,
                                                      integration: storage)
                                               .pick(:origin_user_id)
                if origin_user_id.present?
                  ServiceResult.success(result: origin_user_id)
                else
                  failure(code: :error,
                          data: StorageErrorData.new(source: caller),
                          log_message:
                            "No origin user ID or user token found. Cannot execute query without user context.")
                end

Other places should be fine with new constraint. But maybe I am missing something.

[NobodysNightmare]

Essentially I didn't think enough about edge cases right now, and I'd be happy to delay that.

But on a general level:

• uniqueness: { scope: %i[auth_source integration] } means that it's possible that user.remote_identities.where(storage:).count > 1
• This means there could be two different origin_user_id values for the same user at a storage (user.remote_identities.where(storage:).pluck(:origin_user_id).uniq.count > 1`)
• This means if we have code like RemoteIdentity.where(user:, integration:).pick(:origin_user_id) it could pick the wrong user id

However, you already properly addressed the one case I know of in your example above, by making sure we pick the correct auth_source. And other places I see in this PR actually work correctly by iterating over all identities, regardless of the auth source. So it's probably fine I guess?

[ba1ash]

That's my understanding as well.

[NobodysNightmare]

🟡 I am wondering whether this code won't be run every time that we fetch a token for the storage.

I see nothing limiting us to the create case here, so also all subsequent updates (possibly with no changes) will trigger the REMOTE_IDENTITY_CREATED event.

Am I missing something? Is this intended?

[ba1ash]

Yes, this code will run on every token fetch. I do not see a reason for emitting an event if there is no changes to existing model. Would you agree?

[NobodysNightmare]

Exactly. I think under normal circumstances the event should be fired once upon creation.

[NobodysNightmare]

🔴 See nextcloud_managed_folder_sync_service.rb.

[ba1ash]

Yes, the same.

[NobodysNightmare]

🟡 Shouldn't we be using changing values here? Otherwise all remote identities created through the factory would have colliding user names by default.

Suggested change

	origin_user_id: "33db2c84-275d-46af-afb0-c26eb786b194")
	origin_user_id: SecureRandom.uuid)

Alternatively: If there is a good reason to always use the same one, I'd go with the @mereghost approach and call it UNIVERSALLY-DUPLICATED-IDENTIFIER. This at least makes it transparent, that we are always dealing with the same thing and it's 5% funny ;-)

Alternatively: Just don't specify anything, because the factory for remote identities already takes care of generating sequential origin user ids.

[ba1ash]

This origin_user_id is used in a couple of vcr cassetes. I will try to move it specifically to the specs concerned with it.

[NobodysNightmare]

🔴 This will also not fly for sso_user_token (which is part of this when), because authentication through SSO will have a different auth_source than storage.oauth_client.

[ba1ash]

Yes, that is right. I suggested above(in the uniqueness thread) to have two separate branches here with explicit auth_source set. Wdyt?

[NobodysNightmare]

Yep, code suggested there will fix this. Alternatively (I didn't check what it would look like) we could also have two when cases, one for SSO and one for oauth_user_token. I am not sure what would be cleaner in the end.