EventStore / EventStoreDB-Client-Java

Official Asynchronous Java 8+ Client Library for EventStoreDB 20.6+
https://eventstore.com
Apache License 2.0
63 stars 19 forks source link

Per-operation credentials (passed via OptionsBase instance) ignored. #220

Closed jezovuk closed 1 year ago

jezovuk commented 1 year ago

Problem description

It seems that credentials passed via com.eventstore.dbclient.OptionsBase.authenticated(UserCredentials) are ignored and global client-level credentials (configured via com.eventstore.dbclient.EventStoreDBClientSettings.defaultCredentials) will always be used.

EventStoreDB servder version: 22.10.1 OSS EventStoreDB-Client-Java version: 4.1.1

Demo code

    // create non-admin test/test user on target ES server
    @Test
    public void testPerOperationAuth() throws Exception {
        final EventStoreDBClient client = eventStoreDBClient();
        final StreamMetadata md = new StreamMetadata();
        final StreamAcl acl = new StreamAcl();
        acl.addWriteRoles("$admins");
        md.setAcl(acl);
        client.setStreamMetadata("test", md);
        // the append should fail with auth error from server, since "test" user is not part of "$admins" group
        // instead it works?!
        EventStoreUtils.append(client,
                               new UserCredentials("test", "test"),
                               jacksonObjectMapper(),
                               xenaEventStream("test"),
                               ExpectedRevision.any(),
                               DateTimeUpdatedImpl.builder().now(Instant.now()).build());
    }

    private static EventStoreDBClient eventStoreDBClient() {
        final String host = EventStoreConsts.ES_DEFAULT_HOST;
        final EventStoreDBClientSettings settings =
            EventStoreDBClientSettings.builder()
                                      .addHost(new Endpoint(host, ES_DEFAULT_HTTP_PORT))
                                      .defaultCredentials("admin", "changeit")
                                      .keepAliveInterval(Duration.ofSeconds(10).toMillis())
                                      .keepAliveTimeout(Duration.ofSeconds(10).toMillis())
                                      .discoveryInterval((int)Duration.ofSeconds(10).toMillis())
                                      .maxDiscoverAttempts(3)
                                      .gossipTimeout((int)Duration.ofSeconds(15).toMillis())
                                      .defaultDeadline(Duration.ofMinutes(3).toMillis())
                                      .dnsDiscover(false)
                                      .tlsVerifyCert(false)
                                      .buildConnectionSettings();
        return EventStoreDBClient.create(settings);
    }

Suggested modifications

From code inspection, it seems that following places should be modified:

  1. com.eventstore.dbclient.GrpcUtils.configureStub(S, EventStoreDBClientSettings, OptionsBase<O>)

Instead of:

        UserCredentials credentials = null;

        if (options.hasUserCredentials()) {
            credentials = options.getCredentials();
        } else if (settings.getDefaultCredentials() != null) {
            credentials = settings.getDefaultCredentials();
        }

impl should be similar to this:

        UserCredentials credentials = options.getCredentials();
        if (credentials == null) {
            credentials = settings.getDefaultCredentials();
        }
  1. com.eventstore.dbclient.GrpcClient.WorkItemArgs.getHttpConnection(OptionsBase<A>, EventStoreDBClientSettings, String)

Instead of:

                String creds = options.getUserCredentials();

                if (creds == null && settings.getDefaultCredentials() != null) {
                    creds = settings.getDefaultCredentials().basicAuthHeader();
                }

                if (creds != null) {
                    conn.setRequestProperty("Authorization", creds);
                }

something like this:

                UserCredentials creds = options.getCredentials();
                if (creds == null) {
                    creds = settings.getDefaultCredentials();
                }
                if (creds != null) {
                    conn.setRequestProperty("Authorization", creds.basicAuthHeader());
                }
  1. com.eventstore.dbclient.OptionsBase.metadata

It seems to me that this field can be removed altogether. It is never actually used AFAICT, except in getUserCredentials() and hasUserCredentials(), which are only used in above-listed snippets. The only other use is in OperationBase.getMetadata(), which is called from com.eventstore.dbclient.DeleteStream.execute() but only to be stored in unused local var (headers).

The dichotomy between OptionsBase.hasUserCredentials() and OptionsBase.getUserCredentials() on one hand and OptionsBase.credentials on the other is the root cause of this entire issue so it seems cleanest to remove OptionsBase.metadata altogether.

Note that I'm not familiar with entire codebase in great detail (this is all from one afternoon of debugging), so it is of course possible that I'm missing something.

YoEight commented 1 year ago

Nice catch @jezovuk and thanks for reporting. PR is ready. As soon as the PR is merged, I'll cut a release on Maven Central.