Make OAuth prefix used for storing credentials configurable; use port… #15114

chenkins · 2023-09-12T16:32:27Z

Resolves #15113

core/src/main/java/ch/cyberduck/core/DefaultHostPasswordStore.java

AliveDevil · 2023-09-13T07:53:22Z

On Windows the OAuth Token is attached to the Credential Manager entry for a host in an additional "page"/attribute.

Basically:

protocol://user@hostname:port/
|- username: user
|- password: …
\  Metadata
    |-  OAuth Access Token: …
    |- OAuth Refresh Token: …
    \  OAuth Expiry: …

Thus the configurable oauth prefix isn't used there.
To make this work an additional credential manager entry for the OAuth connection has to be created.

https://github.com/iterate-ch/cyberduck/blob/master/core/src/main/csharp/ch/cyberduck/core/CredentialManagerPasswordStore.cs#L115

core/src/main/java/ch/cyberduck/core/DefaultHostPasswordStore.java

core/src/main/java/ch/cyberduck/core/OAuthPrefixService.java

chenkins · 2023-09-27T11:20:09Z

core/src/main/csharp/ch/cyberduck/core/CredentialManagerPasswordStore.cs

-                if (protocol.isPortConfigurable() && !Equals(protocol.getDefaultPort(), bookmark.getPort()))
+                isOAuth = true;
+                OAuthPrefixService oAuthPrefix = new OAuthPrefixServiceFactory().create(bookmark);
+                hostname = oAuthPrefix.getIdentifier();


@AliveDevil looks dodgy to assign identifier to hostname

Well, that's what I called the field, could change that to identifier, which holds either OAuthPrefix.getIdentifier() or bookmark.getHostname() (in non-OAuth path).
Which is then built to duck:identifier:port?user= (OAuth) or duck:fqdn:port?user= (Default).

This causes a regression as previously entries were saved using bookmark.getProtocol().getOAuthTokenUrl() when available.

Only using the protocol identifier and no token URL will cause entries to be saved with the same key for bookmarks using same protocol but different OAuth endpoint.

@AliveDevil The previous default was duck:identifier:fqdn:port?user= , not duck:fqdn:port?user= ?

core/src/main/java/ch/cyberduck/core/OAuthPrefixServiceFactory.java

core/src/main/java/ch/cyberduck/core/DefaultOAuthPrefixService.java

core/src/main/java/ch/cyberduck/core/preferences/Preferences.java

dkocher · 2023-10-30T09:15:36Z

Refactored to protocol service PasswordStorePrefixService

Default implementation implements current behaviour of DefaultHostPasswordStore
New specific implementation for Windows implements current behaviour of CredentialManagerPasswordStore

core/src/main/java/ch/cyberduck/core/DefaultHostPasswordStore.java

core/src/main/java/ch/cyberduck/core/AbstractProtocol.java

core/src/main/java/ch/cyberduck/core/PasswordStorePrefixService.java

core/src/main/java/ch/cyberduck/core/preferences/Preferences.java

core/src/main/java/ch/cyberduck/core/DefaultPasswordStorePrefixService.java

windows/src/main/csharp/ch/cyberduck/ui/core/preferences/ApplicationPreferences.cs

cli/src/main/csharp/ch/cyberduck/cli/WindowsTerminalPreferences.cs

core/src/main/java/ch/cyberduck/core/PasswordStorePrefixService.java

core/src/main/java/ch/cyberduck/core/DefaultHostPasswordStore.java

…okup. Signed-off-by: chenkins <chenkins44@gmail.com>

…word store.

dkocher · 2023-11-22T17:01:43Z

core/src/main/csharp/ch/cyberduck/core/CredentialManagerPasswordStore.cs

            if (!string.IsNullOrWhiteSpace(cred.Password))
            {
                return cred.Password;
            }
+
            return base.findLoginPassword(bookmark);


Because the base implementation will load the platform specific implementation of the prefix service this will cause a regression in failure finding deprecated store entries.

core/src/main/java/ch/cyberduck/core/CredentialManagerPasswordStoreDescriptorService.java

ylangisc · 2023-11-23T07:36:45Z

core/src/main/java/ch/cyberduck/core/DefaultHostPasswordStore.java

-            return null;
-        }
+        final PasswordStoreDescriptorService service = bookmark.getProtocol().getFeature(PasswordStoreDescriptorService.class);
+        final String prefix = service.getDescriptor(bookmark);


ylangisc · 2023-11-23T07:44:53Z

core/src/main/java/ch/cyberduck/core/DefaultHostPasswordStore.java

        final Credentials credentials = bookmark.getCredentials();
-        if(StringUtils.isEmpty(credentials.getUsername())) {


How is this case handled in the new implementation?

ylangisc · 2023-11-23T07:54:22Z

core/src/main/java/ch/cyberduck/core/DefaultPasswordStoreDescriptorService.java

Introduce variables, e.g. URI.create(bookmark.getProtocol().getOAuthTokenUrl()) to avoid duplicate code which is not very readable as you always have to check if there is any difference to the previous usage.

ylangisc · 2023-11-23T08:14:11Z

core/src/main/csharp/ch/cyberduck/core/CredentialManagerPasswordStore.cs

-            var credentials = bookmark.getCredentials();
-
-            var targetBuilder = new UriBuilder(PreferencesFactory.get().getProperty("application.container.name"), string.Empty);
+            PasswordStoreDescriptorService service = (PasswordStoreDescriptorService)bookmark.getProtocol().getFeature(typeof(PasswordStorePrefixService));


Wrong type PasswordStorePrefixService.

ylangisc · 2023-11-23T08:15:05Z

core/src/main/csharp/ch/cyberduck/core/CredentialManagerPasswordStore.cs

+            string hostname = service.getHostname(bookmark);
+            if (!string.IsNullOrWhiteSpace(hostname))
+            {
+                pathBuilder.Append(hostname);


Missing : between prefix and hostname?

ylangisc · 2023-11-23T08:16:36Z

core/src/main/csharp/ch/cyberduck/core/CredentialManagerPasswordStore.cs

@@ -211,27 +221,37 @@ public override void save(Host bookmark)

        private static Uri ToUri(Host bookmark)


Missing test coverage for such methods.

ylangisc · 2023-11-23T08:22:20Z

core/src/main/csharp/ch/cyberduck/core/CredentialManagerPasswordStore.cs

            var pathBuilder = new StringBuilder();
-            pathBuilder.Append(protocol.getIdentifier());
-            if (protocol.isHostnameConfigurable() || !(protocol.isTokenConfigurable() || protocol.isOAuthConfigurable()))


Is this equivalent to the new checks in CredentialManagerPasswordStoreDescriptorService which are different.

chenkins requested a review from a team as a code owner September 12, 2023 16:32

chenkins self-assigned this Sep 12, 2023

chenkins requested a review from dkocher September 12, 2023 16:32

dkocher requested changes Sep 13, 2023

View reviewed changes

core/src/main/java/ch/cyberduck/core/DefaultHostPasswordStore.java Outdated Show resolved Hide resolved

core/src/main/java/ch/cyberduck/core/DefaultHostPasswordStore.java Outdated Show resolved Hide resolved

dkocher marked this pull request as draft September 13, 2023 07:55

chenkins commented Sep 13, 2023

View reviewed changes

core/src/main/java/ch/cyberduck/core/DefaultHostPasswordStore.java Outdated Show resolved Hide resolved

chenkins force-pushed the feature/GH-15113-oauth-prefix-factory branch 2 times, most recently from ac17560 to ec72ed9 Compare September 26, 2023 13:11

chenkins commented Sep 27, 2023

View reviewed changes

chenkins marked this pull request as ready for review October 20, 2023 20:54

chenkins requested a review from dkocher October 20, 2023 20:54

dkocher marked this pull request as draft October 23, 2023 07:23

chenkins force-pushed the feature/GH-15113-oauth-prefix-factory branch from ec72ed9 to 10c08c1 Compare October 23, 2023 11:59

dkocher force-pushed the feature/GH-15113-oauth-prefix-factory branch from 247781e to fb98028 Compare October 27, 2023 08:42

dkocher assigned dkocher and unassigned chenkins Oct 27, 2023

dkocher force-pushed the feature/GH-15113-oauth-prefix-factory branch from fb98028 to 507c612 Compare October 27, 2023 18:25

dkocher modified the milestones: 8.7.1, 9.0 Oct 27, 2023

dkocher marked this pull request as ready for review October 27, 2023 18:41

dkocher force-pushed the feature/GH-15113-oauth-prefix-factory branch 3 times, most recently from 6b5dbe3 to f311e4f Compare October 29, 2023 19:55

chenkins commented Oct 30, 2023

View reviewed changes

dkocher mentioned this pull request Nov 1, 2023

Shared OAuth refresh token not accepted #15284

Closed

dkocher linked an issue Nov 1, 2023 that may be closed by this pull request

Shared OAuth refresh token not accepted #15284

Closed

dkocher marked this pull request as draft November 1, 2023 16:46

chenkins and others added 14 commits November 22, 2023 17:25

Use java.net.URI.create because java.net.URL is prone to do a name lo…

2e4b00d

…okup. Signed-off-by: chenkins <chenkins44@gmail.com>

Add try-implementation for WinCredentialManager to get credentials

c6f7825

Support persisting OAuth credentials for each OAuth token url host

53ce4d1

Review OAuth prefix service

b5bd5dc

Add docs

1f07de2

Always save port regardless of default.

142ccb7

Rename method.

62c8368

Rename interface.

e8e46c4

Revert using hostname and port from token URL when OAuth.

b0e70f7

Register platform specific implementation to determine prefix in pass…

9016fba

…word store.

Javadoc.

b560803

Javadoc.

884a14f

Javadoc.

775e19a

Formatting.

fe35500

dkocher force-pushed the feature/GH-15113-oauth-prefix-factory branch from a30e21e to fe35500 Compare November 22, 2023 16:26

Javadoc.

3a2b565

dkocher reviewed Nov 22, 2023

View reviewed changes

core/src/main/java/ch/cyberduck/core/CredentialManagerPasswordStoreDescriptorService.java Show resolved Hide resolved

dkocher added 2 commits November 22, 2023 21:20

Read scheme from token URL.

24df9a7

Rename interface.

8650ca9

dkocher force-pushed the feature/GH-15113-oauth-prefix-factory branch from 23accf1 to 8650ca9 Compare November 22, 2023 20:24

ylangisc reviewed Nov 23, 2023

View reviewed changes

dkocher modified the milestones: 8.7.2, 9.0 Nov 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make OAuth prefix used for storing credentials configurable; use port… #15114

Make OAuth prefix used for storing credentials configurable; use port… #15114

chenkins commented Sep 12, 2023

AliveDevil commented Sep 13, 2023 •

edited

chenkins Sep 27, 2023

AliveDevil Sep 27, 2023

dkocher Oct 27, 2023

dkocher Oct 27, 2023

dkocher Oct 27, 2023

dkocher commented Oct 30, 2023

dkocher Nov 22, 2023

ylangisc Nov 23, 2023

ylangisc Nov 23, 2023

ylangisc Nov 23, 2023

ylangisc Nov 23, 2023

ylangisc Nov 23, 2023

ylangisc Nov 23, 2023

ylangisc Nov 23, 2023

		final Credentials credentials = bookmark.getCredentials();
		if(StringUtils.isEmpty(credentials.getUsername())) {

		@@ -211,27 +221,37 @@ public override void save(Host bookmark)

		private static Uri ToUri(Host bookmark)

Make OAuth prefix used for storing credentials configurable; use port… #15114

Are you sure you want to change the base?

Make OAuth prefix used for storing credentials configurable; use port… #15114

Conversation

chenkins commented Sep 12, 2023

AliveDevil commented Sep 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkocher commented Oct 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AliveDevil commented Sep 13, 2023 •

edited