[FEATURE] Update ACL Token `List` endpoint by MohamedM216 · Pull Request #592 · G-Research/consuldotnet

MohamedM216 · 2026-03-02T22:33:30Z

Description

[FEATURE] Update ACL Token List endpoint

Related Issues

#531

Additional Context

Checklist

Please make sure to check the following before submitting your pull request:

Did you read the contributing guidelines?
Did you update the docs?
Did you write any tests to validate this change?

…te-acl-token-list-endpoint

marcin-krystianc · 2026-03-23T11:33:35Z

Consul.Test/TokenTest.cs

        {
            Skip.If(string.IsNullOrEmpty(TestHelper.MasterToken));
+            var cutOffVersion = SemanticVersion.Parse("1.7.14");
+            Skip.If(AgentVersion == cutOffVersion, $"Node Identity is not supported in {AgentVersion}");


Generally, it is not a good idea to change (or remove) existing tests when implementing new features. It is easier to reason about backwards compatibility if we do not touch exisiting test code.

marcin-krystianc · 2026-03-23T11:41:47Z

Consul/Token.cs

        public TokenEntry()
-            : this(string.Empty, string.Empty, Array.Empty<PolicyLink>(), Array.Empty<RoleLink>(), Array.Empty<ServiceIdentity>())
+            : this(string.Empty, string.Empty, Array.Empty<PolicyLink>(), Array.Empty<RoleLink>(), Array.Empty<ServiceIdentity>(), false, Array.Empty<TemplatedPolicy>(), Array.Empty<NodeIdentity>(), null, null)
        {
        }

        public TokenEntry(string description, PolicyLink[] policies)
-            : this(string.Empty, description, policies, Array.Empty<RoleLink>(), Array.Empty<ServiceIdentity>())
+            : this(string.Empty, description, policies, Array.Empty<RoleLink>(), Array.Empty<ServiceIdentity>(), false, Array.Empty<TemplatedPolicy>(), Array.Empty<NodeIdentity>(), null, null)
        {
        }

        public TokenEntry(string description, RoleLink[] roles)
-            : this(string.Empty, description, Array.Empty<PolicyLink>(), roles, Array.Empty<ServiceIdentity>())
+            : this(string.Empty, description, Array.Empty<PolicyLink>(), roles, Array.Empty<ServiceIdentity>(), false, Array.Empty<TemplatedPolicy>(), Array.Empty<NodeIdentity>(), null, null)
        {
        }

        public TokenEntry(string description, ServiceIdentity[] serviceIdentities)
-            : this(string.Empty, description, Array.Empty<PolicyLink>(), Array.Empty<RoleLink>(), serviceIdentities)
+            : this(string.Empty, description, Array.Empty<PolicyLink>(), Array.Empty<RoleLink>(), serviceIdentities, false, Array.Empty<TemplatedPolicy>(), Array.Empty<NodeIdentity>(), null, null)
        {
        }

        public TokenEntry(string accessorId, string description)
-            : this(accessorId, description, Array.Empty<PolicyLink>(), Array.Empty<RoleLink>(), Array.Empty<ServiceIdentity>())
+            : this(accessorId, description, Array.Empty<PolicyLink>(), Array.Empty<RoleLink>(), Array.Empty<ServiceIdentity>(), false, Array.Empty<TemplatedPolicy>(), Array.Empty<NodeIdentity>(), null, null)
        {
        }

-        public TokenEntry(string accessorId, string description, PolicyLink[] policies, RoleLink[] roles, ServiceIdentity[] serviceIdentities)
+        public TokenEntry(string accessorId, string description, PolicyLink[] policies, RoleLink[] roles, ServiceIdentity[] serviceIdentities, bool local, TemplatedPolicy[] templatedPolicies, NodeIdentity[] nodeIdentities, DateTime? expirationTime, TimeSpan? expirationTTL)


I don't think there's any need to update the constructors here. Ideally, we would remove them entirely, but that should be done in another PR, if at all.

marcin-krystianc · 2026-03-23T11:48:34Z

Consul/Interfaces/ITokenEndpoint.cs

+        Task<QueryResult<TokenEntry[]>> List(string policy, string role, string serviceName, string authMethod, CancellationToken ct);
+        Task<QueryResult<TokenEntry[]>> List(string policy, string role, string serviceName, string authMethod);
+        Task<QueryResult<TokenEntry[]>> List(string policy, string role, string serviceName, string authMethod, QueryOptions q, CancellationToken ct = default);


I think a single overload would do:

Suggested change

Task<QueryResult<TokenEntry[]>> List(string policy, string role, string serviceName, string authMethod, CancellationToken ct);

Task<QueryResult<TokenEntry[]>> List(string policy, string role, string serviceName, string authMethod);

Task<QueryResult<TokenEntry[]>> List(string policy, string role, string serviceName, string authMethod, QueryOptions q, CancellationToken ct = default);

Task<QueryResult<TokenEntry[]>> List(string policy, string role, string serviceName, string authMethod, QueryOptions q = QueryOptions.Default, CancellationToken ct = default);

marcin-krystianc · 2026-03-23T11:49:43Z

Consul/TemplatedPolicy.cs

+{
+    public class TemplatedPolicy
+    {
+        [JsonProperty("TemplateName")]


I don't think that this adds any value:

Suggested change

[JsonProperty("TemplateName")]

marcin-krystianc · 2026-03-23T11:50:09Z

Consul/TemplatedPolicy.cs

+        [JsonProperty("TemplateName")]
+        public string TemplateName { get; set; }
+
+        [JsonProperty("TemplateVariables")]


I don't think that this adds any value:

Suggested change

[JsonProperty("TemplateVariables")]

…uched

…te-acl-token-list-endpoint

marcin-krystianc · 2026-03-24T10:21:57Z

Consul.Test/TokenTest.cs

+                if (matchingToken.Response != null)
+                    await _client.Token.Delete(matchingToken.Response.AccessorID);
+                if (nonMatchingToken.Response != null)
+                    await _client.Token.Delete(nonMatchingToken.Response.AccessorID);


If matchingToken or nonMatchingToken would be null, it would throw NRE. Thus, the if conditions don't make much sense here.
Let's just get rid of try/finally and complicated cleanup logic.

marcin-krystianc · 2026-03-24T10:27:08Z

Consul/TemplatedPolicy.cs

+//  <copyright file="TemplatedPolicy.cs" company="PlayFab Inc">
+//    Copyright 2015 PlayFab Inc.


Suggested change

// <copyright file="TemplatedPolicy.cs" company="PlayFab Inc">

// Copyright 2015 PlayFab Inc.

// <copyright file="TemplatedPolicy.cs" company="G-Research Limited">

 // Copyright 2020 G-Research Limited

marcin-krystianc · 2026-03-24T10:34:55Z

Consul/Token.cs

+        public bool ShouldSerializeTemplatedPolicies()
+        {
+            return TemplatedPolicies != null && TemplatedPolicies.Length > 0;
+        }
+
+        public bool ShouldSerializeNodeIdentities()
+        {
+            return NodeIdentities != null && NodeIdentities.Length > 0;
+        }
+


I don't think this methods are needed at all, please remove them:

Suggested change

public bool ShouldSerializeTemplatedPolicies()

{

return TemplatedPolicies != null && TemplatedPolicies.Length > 0;

}

public bool ShouldSerializeNodeIdentities()

{

return NodeIdentities != null && NodeIdentities.Length > 0;

}

marcin-krystianc · 2026-03-24T10:50:47Z

Consul.Test/TokenTest.cs

+                Assert.NotEmpty(nonMatchingToken.Response.Description);
+
+                // List tokens filtering by the specific Policy ID
+                var filteredList = await _client.Token.List(policy.Response.ID, null, null, null);


Only policyId is tested here, but other filters need to be tested as well.

marcin-krystianc · 2026-03-27T09:24:07Z

Filtering by AuthMethodTest is not yet tested. Look at the AuthMethodTest to see how to create and use auth methods

Ah, I realised that I pointed you to a broken test example. I've opened a PR to fix it #614. Please try using the code from that PR.

…te-acl-token-list-endpoint

MohamedM216 · 2026-03-27T09:57:23Z

I've opened a PR to fix it #614. Please try using the code from that PR.

I tried to fix it following your approach, and it worked well for creating a new AuthMethod instance, but now it throws an exception when creating a new token: Consul.ConsulRequestException : Unexpected response, status code InternalServerError: AuthMethod field is disallowed outside of login

what does this message mean?
`

marcin-krystianc · 2026-03-27T11:25:22Z

I've opened a PR to fix it #614. Please try using the code from that PR.

I tried to fix it following your approach, and it worked well for creating a new AuthMethod instance, but now it throws an exception when creating a new token: Consul.ConsulRequestException : Unexpected response, status code InternalServerError: AuthMethod field is disallowed outside of login

what does this message mean? `

Looking at the docs, it is not possible to set the authmethod, when creating the token -> https://developer.hashicorp.com/consul/api-docs/acl/tokens.

marcin-krystianc · 2026-03-27T12:15:12Z

I realised that we need to create the token with AuthMethod.Login(), but that one is implemented but not working properly (#615).

MohamedM216 · 2026-03-27T12:36:02Z

Yes, that is what I realized too. I tried to use it but it didn't work, throws: Consul.ConsulRequestException : Unexpected response, status code MethodNotAllowed: method PUT not allowed
Should we finish #615 first, then continue the work here?

marcin-krystianc · 2026-03-27T20:59:44Z

Yes, that is what I realized too. I tried to use it but it didn't work, throws: Consul.ConsulRequestException : Unexpected response, status code MethodNotAllowed: method PUT not allowed Should we finish #615 first, then continue the work here?

Yes, the #615 needs to be done first. I've pushed commit that kind of does this (just an attempt with an agentic tool). Feel free, use it as a starting point and open a PR to fix it properly.

…te-acl-token-list-endpoint

MohamedM216 · 2026-04-02T11:05:49Z

There is a small nit in the previous PR, inside AuthMethod_Login(), positions of expected and actual fields were swapped by mistake.

Assert.Equal(res.Response.AuthMethod, authMethodEntry.Name);

So do you recommend to fix it here or open a separate PR for it?

marcin-krystianc · 2026-04-02T11:42:19Z

There is a small nit in the previous PR, inside AuthMethod_Login(), positions of expected and actual fields were swapped by mistake.
Assert.Equal(res.Response.AuthMethod, authMethodEntry.Name);
So do you recommend to fix it here or open a separate PR for it?

Feel, free to open a PR with a fix for that.

marcin-krystianc · 2026-04-02T11:47:11Z

Consul.Test/TokenTest.cs

+            Assert.NotEmpty(token3.Response.AccessorID);
+            Assert.NotEmpty(token3.Response.SecretID);
+            Assert.Equal(authMethodEntry.Name, token3.Response.AuthMethod);
+#endif


Can you group all the code into a single conditional compilation block? it will make it more readable.

…te-acl-token-list-endpoint

Consul/Token.cs

…te-acl-token-list-endpoint

Consul.Test/TokenTest.cs

Co-authored-by: Marcin Krystianc <marcin.krystianc@gmail.com>

MohamedM216 added 6 commits March 3, 2026 00:04

update acl-token List endpoint

253472a

add test for token list filtering

0190224

Merge branch 'master' of github.com:G-Research/consuldotnet into upda…

4ef049d

…te-acl-token-list-endpoint

Merge branch 'master' of github.com:G-Research/consuldotnet into upda…

a819315

…te-acl-token-list-endpoint

Merge branch 'master' of github.com:G-Research/consuldotnet into upda…

fa25df3

…te-acl-token-list-endpoint

polishing + update list endpoints, but has RS0017 errors

d8a16dd

MohamedM216 mentioned this pull request Mar 20, 2026

[FEATURE] update ACL Token Read endpoint to add expanded parameter #601

Merged

3 tasks

MohamedM216 added 3 commits March 20, 2026 20:23

Merge branch 'master' of github.com:G-Research/consuldotnet into upda…

def1237

…te-acl-token-list-endpoint

fix publicapi.unshipped file population (fix RS0017)

3ae9ff8

fix stackoverflow in the test + polish the test unit

ce9d08c

MohamedM216 force-pushed the update-acl-token-list-endpoint branch from 7e56c96 to ce9d08c Compare March 20, 2026 20:06

marcin-krystianc reviewed Mar 23, 2026

View reviewed changes

MohamedM216 added 4 commits March 24, 2026 10:40

keep the old code as it is

407bd53

add a one single sufficient overload and keep token constructors unto…

d19fa35

…uched

remove unneeded code

4515e34

Merge branch 'master' of github.com:G-Research/consuldotnet into upda…

44a10c2

…te-acl-token-list-endpoint

MohamedM216 force-pushed the update-acl-token-list-endpoint branch from 0a31b2a to 44a10c2 Compare March 24, 2026 08:53

MohamedM216 requested a review from marcin-krystianc March 24, 2026 08:55

update unshipped.txt file

03cd119

MohamedM216 force-pushed the update-acl-token-list-endpoint branch from b03daab to 03cd119 Compare March 24, 2026 08:56

marcin-krystianc reviewed Mar 24, 2026

View reviewed changes

simplify test

9171c48

MohamedM216 force-pushed the update-acl-token-list-endpoint branch from 81c540d to 28b79a2 Compare March 24, 2026 11:03

cleanup

c936662

MohamedM216 force-pushed the update-acl-token-list-endpoint branch from 28b79a2 to c936662 Compare March 24, 2026 11:05

MohamedM216 added 2 commits March 24, 2026 15:04

test filter by roleid, authmethod and servicename

5b047ec

improve tests

526fe66

MohamedM216 force-pushed the update-acl-token-list-endpoint branch from 19a276f to 526fe66 Compare March 24, 2026 13:20

MohamedM216 added 2 commits March 27, 2026 11:35

Merge branch 'master' of github.com:G-Research/consuldotnet into upda…

f389c58

…te-acl-token-list-endpoint

test: fix filtering by authmethod

ed927a8

MohamedM216 force-pushed the update-acl-token-list-endpoint branch from b6fe02e to ed927a8 Compare March 27, 2026 09:52

MohamedM216 added 2 commits April 2, 2026 11:17

Merge branch 'master' of github.com:G-Research/consuldotnet into upda…

b229ee3

…te-acl-token-list-endpoint

test filtering by AuthMethod

8151478

marcin-krystianc reviewed Apr 2, 2026

View reviewed changes

MohamedM216 added 2 commits April 2, 2026 14:17

group all the code into a single conditional compilation block

347b9f0

Merge branch 'master' of github.com:G-Research/consuldotnet into upda…

8c4f4e0

…te-acl-token-list-endpoint

MohamedM216 force-pushed the update-acl-token-list-endpoint branch from 96754bb to 8c4f4e0 Compare April 2, 2026 12:33

MohamedM216 requested a review from marcin-krystianc April 2, 2026 16:55

marcin-krystianc reviewed Apr 3, 2026

View reviewed changes

Consul/Token.cs Outdated Show resolved Hide resolved

MohamedM216 added 2 commits April 3, 2026 18:52

Merge branch 'master' of github.com:G-Research/consuldotnet into upda…

9449c34

…te-acl-token-list-endpoint

remove unnecessary properties

7c3c29d

MohamedM216 force-pushed the update-acl-token-list-endpoint branch from d504119 to 7c3c29d Compare April 3, 2026 16:54

MohamedM216 requested a review from marcin-krystianc April 3, 2026 16:55

marcin-krystianc reviewed Apr 7, 2026

View reviewed changes

Consul.Test/TokenTest.cs Outdated Show resolved Hide resolved

Update Consul.Test/TokenTest.cs

c1543e6

Co-authored-by: Marcin Krystianc <marcin.krystianc@gmail.com>

MohamedM216 requested a review from marcin-krystianc April 7, 2026 09:05

marcin-krystianc approved these changes Apr 7, 2026

View reviewed changes

marcin-krystianc merged commit 5b53e3f into G-Research:master Apr 7, 2026
120 checks passed

marcin-krystianc mentioned this pull request Apr 7, 2026

feature: updated acl-token implementation #532

Closed

3 tasks

MohamedM216 deleted the update-acl-token-list-endpoint branch April 9, 2026 21:05

		// <copyright file="TemplatedPolicy.cs" company="PlayFab Inc">
		// Copyright 2015 PlayFab Inc.

Conversation

MohamedM216 commented Mar 2, 2026

Description

Related Issues

Additional Context

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcin-krystianc commented Mar 27, 2026

Uh oh!

MohamedM216 commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcin-krystianc commented Mar 27, 2026

Uh oh!

marcin-krystianc commented Mar 27, 2026

Uh oh!

MohamedM216 commented Mar 27, 2026

Uh oh!

marcin-krystianc commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MohamedM216 commented Apr 2, 2026

Uh oh!

marcin-krystianc commented Apr 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

MohamedM216 commented Mar 27, 2026 •

edited

Loading

marcin-krystianc commented Mar 27, 2026 •

edited

Loading