Support persistent session in IntrospectToken method of GRPC client, PLTFRM-72444 #33
Conversation
idptoken/grpc_client.go
Outdated
| @@ -117,7 +122,11 @@ func (c *GRPCClient) IntrospectToken( | |||
| req.ScopeFilter[i] = &pb.IntrospectionScopeFilter{ResourceNamespace: scopeFilter[i].ResourceNamespace} | |||
| } | |||
|
|
|||
| ctx = metadata.AppendToOutgoingContext(ctx, grpcMetaAuthorization, makeBearerToken(accessToken)) | |||
| if c.sessionID != "" { | |||
| ctx = metadata.AppendToOutgoingContext(ctx, grpcMetaSessionID, c.sessionID) | |||
There was a problem hiding this comment.
What will happen if session id is already in context?
There was a problem hiding this comment.
It will be replaced. Now it is considers only 1 active session could be used by a client.
Semerokozlyat
force-pushed
the
feature/PLTFRM-72444-caching-idp-client-session-support
branch
2 times, most recently
from
fc3abda to
ca77c0a
Compare
idptoken/grpc_client.go
Outdated
| @@ -177,6 +195,18 @@ func (c *GRPCClient) IntrospectToken( | |||
| }, nil | |||
| } | |||
|
|
|||
| func (c *GRPCClient) GetSessionID() string { | |||
There was a problem hiding this comment.
Could you make GetSessionID and SetSessionID private? They appear to be used only internally, so exporting them may not be necessary.
There was a problem hiding this comment.
Unfortunately, it is not possible because "grpc_client.go" and "grpc_client_test.go" files are related to different go packages, and unit tests require checking and explicit setting of session id.
There was a problem hiding this comment.
Please consider my suggestion below.
idptoken/grpc_client.go
Outdated
| @@ -135,11 +144,20 @@ func (c *GRPCClient) IntrospectToken( | |||
| if err := c.do(ctx, "IDPTokenService/IntrospectToken", func(ctx context.Context) error { | |||
| var innerErr error | |||
| resp, innerErr = c.client.IntrospectToken(ctx, &req) | |||
| if errors.Is(innerErr, ErrUnauthenticated) { | |||
There was a problem hiding this comment.
ErrUnauthenticated is returned by the do() method, which means the session won’t be cleared. Could you fix this and add a corresponding unit test?
idptoken/introspector_test.go
Outdated
| @@ -162,6 +162,7 @@ func TestIntrospector_IntrospectToken(t *gotesting.T) { | |||
| introspectorOpts idptoken.IntrospectorOpts | |||
| tokenToIntrospect string | |||
| accessToken string | |||
| sessionID string | |||
There was a problem hiding this comment.
I couldn’t find where this field is used. Could you clarify? Ideally, we should verify that the mock gRPC server receives the correct session ID from the client.
Semerokozlyat
force-pushed
the
feature/PLTFRM-72444-caching-idp-client-session-support
branch
3 times, most recently
from
51053f9 to
e7d52f8
Compare
idptoken/grpc_client_test.go
Outdated
|
|
||
| for _, tc := range tCases { | ||
| t.Run(tc.name, func(t *gotesting.T) { | ||
| grpcClient.SetSessionID(tc.sessionID) |
There was a problem hiding this comment.
Unfortunately, this test does not fully validate the behavior of GRPCClient. Currently, it only verifies that if the sessionID is set, it can be retrieved after making an IntrospectToken call.
Instead of retrieving the session ID from the client, you should check GRPCServerTokenIntrospectorMock.LastSessionMeta. This approach allows us to remove the unnecessary SetSessionID and GetSessionID methods, which are currently used only for testing purposes.
An ideal test should cover the following cases:
- When the gRPC server returns the x-session-id metadata, the gRPC client should use it for subsequent requests.
- When the gRPC server returns an authentication error, the gRPC client should attempt to use the access token for the next request.
Semerokozlyat
force-pushed
the
feature/PLTFRM-72444-caching-idp-client-session-support
branch
6 times, most recently
from
139bde2 to
f838e2d
Compare
…partly PLTFRM-72444
Semerokozlyat
force-pushed
the
feature/PLTFRM-72444-caching-idp-client-session-support
branch
from
f838e2d to
c4a838c
Compare
Support persistent session in IntrospectToken method of GRPC client.
Partly PLTFRM-72444