[CLI-2360] Revoking Auth0 refresh token on cloud logout #2216

kevin-wu24 · 2023-08-22T20:01:28Z

Release Notes

Bug Fixes

confluent logout revokes the refresh token when logging out of Confluent Cloud

Checklist

[CRUCIAL] Is the change for features that are already live in prod?
- yes: ok

What

Running confluent logout for cloud users now makes a request to DELETE /api/sessions.

References

https://confluentinc.atlassian.net/browse/CLI-2360
confluentinc/ccloud-sdk-go-v1-public#24

Test & Review

Unit and integration tests still pass
Request returns a reply with code 200 OK.

internal/command.go

internal/login/command_login_test.go

brianstrauch · 2023-08-22T21:55:42Z

pkg/auth/auth_token_handler.go

+
+func (a *AuthTokenHandlerImpl) RevokeRefreshToken(clientFactory CCloudClientFactory, url string, credentials *Credentials) error {
+	req := &ccloudv1.AuthenticateRequest{IdToken: credentials.AuthToken}
+	client := clientFactory.JwtHTTPClientFactory(context.Background(), credentials.AuthToken, url)


Why can't we just use the default c.Client in login/command.go?

I mentioned this in slack, but that was the reason I was getting Error 401. I was using the AnonHTTPClientFactory before, which I assume is what you mean by default client. However, the backend handler expects to have a userId in the header, so using the JwtHTTPClient worked.

Hmm, seems like this is due to confluent logout being an unauthenticated command. Maybe we should switch it to an authenticated command (we should be able to assume that the user is logged in before using it)

MuweiHe · 2023-08-23T05:25:59Z

pkg/config/mock.go

@@ -20,7 +20,7 @@ var (
 	mockURL                = "http://test"
 	usernameCredentialName = fmt.Sprintf("username-%s-%s", mockEmail, mockURL)
 	apiKeyCredentialName   = fmt.Sprintf("api-key-%s", kafkaAPIKey)
-	mockContextName        = fmt.Sprintf("login-%s-%s", mockEmail, mockURL)
+	MockContextName        = fmt.Sprintf("login-%s-%s", mockEmail, mockURL)


where else is this var being used?

In a logout unit test.

MuweiHe · 2023-08-23T05:38:08Z

internal/logout/command.go

+		loginCredentialsManager:  loginCredentialsManager,
+		loginOrganizationManager: loginOrganizationManager,
+		authTokenHandler:         authTokenHandler,
+	}
 	if cfg.IsCloudLogin() {
 		context = "Confluent Cloud"


Suggested change

context = "Confluent Cloud"

context = "Confluent Cloud"

AuthenticatedCLICommand: pcmd.NewAuthenticatedCLICommand(cmd, prerunner),

move it down for readibility

If we're not logged in, we still want to have this struct field set. I originally had the code this way, but it resulted in a panic rather than an error when the user was not logged in.

MuweiHe · 2023-08-23T05:42:25Z

Have you manually tested, that IsSSO field in get ccloud credentials is always correct?

kevin-wu24 · 2023-08-23T12:04:40Z

Have you manually tested, that IsSSO field in get ccloud credentials is always correct?

yup! It's the same code that used to exist in login/command.go, but I just moved it to pkg since it is shared.

EDIT: did some refactoring and it turns out we don't even need to call the getCCloudCredentials function, since logout an authenticated command now.

brianstrauch · 2023-08-23T15:26:59Z

internal/login/command_test.go

@@ -811,10 +810,10 @@ func TestLoginWithExistingContext(t *testing.T) {
 		ctx.KafkaClusterContext.SetActiveKafkaCluster(kafkaCluster.ID)

 		// Executing logout
-		logoutCmd, _ := newLogoutCmd(cfg, mockNetrcHandler)
+		logoutCmd, cfg1 := newLogoutCmd(auth, userInterface, s.isCloud, req, mockNetrcHandler, AuthTokenHandler, mockLoginCredentialsManager, LoginOrganizationManager, ctx.Name)


Suggested change

logoutCmd, cfg1 := newLogoutCmd(auth, userInterface, s.isCloud, req, mockNetrcHandler, AuthTokenHandler, mockLoginCredentialsManager, LoginOrganizationManager, ctx.Name)

logoutCmd, cfg := newLogoutCmd(auth, userInterface, s.isCloud, req, mockNetrcHandler, AuthTokenHandler, mockLoginCredentialsManager, LoginOrganizationManager, ctx.Name)

This needs to be changed so that the newLogoutCmd doesn't generate a new config, but use the same config as above.

brianstrauch · 2023-08-23T15:27:19Z

internal/login/command_test.go

@@ -903,7 +902,7 @@ func newLoginCmd(auth *ccloudv1mock.Auth, userInterface *ccloudv1mock.UserInterf
 	if !isCloud {
 		mdsConfig := mdsv1.NewConfiguration()
 		mdsClient = mdsv1.NewAPIClient(mdsConfig)
-		mdsClient.TokensAndAuthenticationApi = &mdsmock.TokensAndAuthenticationApi{
+		mdsClient.TokensAndAuthenticationApi = &mdsMock.TokensAndAuthenticationApi{


Package names should always be lowercase.

Suggested change

mdsClient.TokensAndAuthenticationApi = &mdsMock.TokensAndAuthenticationApi{

mdsClient.TokensAndAuthenticationApi = &mdsmock.TokensAndAuthenticationApi{

brianstrauch · 2023-08-23T15:28:03Z

internal/login/command_test.go

@@ -937,8 +936,38 @@ func newLoginCmd(auth *ccloudv1mock.Auth, userInterface *ccloudv1mock.UserInterf
 	return loginCmd, cfg
 }

-func newLogoutCmd(cfg *config.Config, netrcHandler netrc.NetrcHandler) (*cobra.Command, *config.Config) {
-	logoutCmd := logout.New(cfg, climock.NewPreRunnerMock(nil, nil, nil, nil, cfg), netrcHandler)
+func newLogoutCmd(auth *ccloudv1mock.Auth, userInterface *ccloudv1mock.UserInterface, isCloud bool, req *require.Assertions, netrcHandler netrc.NetrcHandler, authTokenHandler pauth.AuthTokenHandler, loginCredentialsManager pauth.LoginCredentialsManager, loginOrganizationManager pauth.LoginOrganizationManager, contextName string) (*cobra.Command, *config.Config) {


This is a crazy number of arguments. Do we really need all of them?

Yeah I forgot to remove some of these, as they aren't used anymore.

brianstrauch · 2023-08-23T15:28:26Z

internal/login/command_test.go

+		mdsConfig := mdsv1.NewConfiguration()
+		mdsClient = mdsv1.NewAPIClient(mdsConfig)
+		mdsClient.TokensAndAuthenticationApi = &mdsMock.TokensAndAuthenticationApi{
+			GetTokenFunc: func(ctx context.Context) (mdsv1.AuthenticationResponse, *http.Response, error) {


Suggested change

GetTokenFunc: func(ctx context.Context) (mdsv1.AuthenticationResponse, *http.Response, error) {

GetTokenFunc: func(_ context.Context) (mdsv1.AuthenticationResponse, *http.Response, error) {

brianstrauch · 2023-08-23T15:28:42Z

internal/login/command_test.go

+				return mdsv1.AuthenticationResponse{
+					AuthToken: testToken1,
+					TokenType: "JWT",
+					ExpiresIn: 100,
+				}, nil, nil


Suggested change

return mdsv1.AuthenticationResponse{

AuthToken: testToken1,

TokenType: "JWT",

ExpiresIn: 100,

}, nil, nil

res := mdsv1.AuthenticationResponse{

AuthToken: testToken1,

TokenType: "JWT",

ExpiresIn: 100,

}

return res, nil, nil

brianstrauch · 2023-08-23T15:30:01Z

pkg/auth/auth_token_handler.go

@@ -169,3 +170,11 @@ func login(client *ccloudv1.Client, req *ccloudv1.AuthenticateRequest) (*ccloudv
 		return client.Auth.Login(req)
 	}
 }
+
+func (a *AuthTokenHandlerImpl) RevokeRefreshToken(client *ccloudv1.Client, req *ccloudv1.AuthenticateRequest) (*ccloudv1.AuthenticateReply, error) {


Does this function need to be inside AuthTokenHandlerImpl? (I don't see a being used anywhere here) It looks like you're introducing a dependency for no reason.

brianstrauch · 2023-08-23T17:29:43Z

internal/login/command_test.go

+			},
+		}
+	}
+	ccloudClientFactory := &climock.CCloudClientFactory{


Seems like this should only run when isCloud is true?

It looks like the NewPreRunnerMock function that takes in this variable runs whether or not isCloud is true.

Sounds like we should change that too?

brianstrauch · 2023-08-23T17:30:33Z

internal/login/command_test.go

+	ccloudClientFactory := &climock.CCloudClientFactory{
+		AnonHTTPClientFactoryFunc: func(baseURL string) *ccloudv1.Client {
+			req.Equal("https://confluent.cloud", baseURL)
+			return &ccloudv1.Client{Params: &ccloudv1.Params{HttpClient: new(http.Client)}, Auth: auth, User: userInterface}


Suggested change

return &ccloudv1.Client{Params: &ccloudv1.Params{HttpClient: new(http.Client)}, Auth: auth, User: userInterface}

return &ccloudv1.Client{

Params: &ccloudv1.Params{HttpClient: new(http.Client)},

Auth: auth,

User: userInterface,

}

brianstrauch · 2023-08-23T17:31:44Z

internal/logout/command.go

@@ -64,3 +78,18 @@ func (c *Command) logout(cmd *cobra.Command, _ []string) error {
 	output.Println(errors.LoggedOutMsg)
 	return nil
 }
+
+func (c *command) revokeCCloudRefreshToken() (*ccloudv1.AuthenticateReply, error) {
+	ctx := c.Config.Config.Context()


Do you know why we need to do this? It looks wrong- can it be shortened?

Yeah we can just pass ctx into the function.

brianstrauch · 2023-08-23T17:32:23Z

internal/logout/command.go

+	}
+
+	req := &ccloudv1.AuthenticateRequest{IdToken: contextState.AuthToken}
+	if sso.IsOkta(c.Client.BaseURL) {


Can we get the URL from the configuration instead? This seems hacky

brianstrauch · 2023-08-23T17:33:19Z

internal/logout/command_test.go

+	config.SetTempHomeDir()
+	cfg := config.AuthenticatedConfigMockWithContextName(contextName)
+	var mdsClient *mdsv1.APIClient
+	if !isCloud {


This looks like duplicate code. Can we de-duplicate?

Is there a place in the code base where shared unit test helpers already exist? Looks like the ccloudClientFactory and mdsClient are used in both the login and logout unit tests.

Not yet, but since these tests span two packages, I'd expect it to go somewhere in pkg/

I tried putting this in existing packages that made sense in pkg/ (auth and mock) but both result in circular imports, so I had to create a new directory unfortunately.

brianstrauch · 2023-08-23T22:12:26Z

internal/logout/command_test.go

@@ -21,6 +18,7 @@ import (
 	"github.com/confluentinc/cli/v3/pkg/config"
 	pmock "github.com/confluentinc/cli/v3/pkg/mock"
 	"github.com/confluentinc/cli/v3/pkg/netrc"
+	testhelp "github.com/confluentinc/cli/v3/pkg/test-helpers"


Suggested change

testhelp "github.com/confluentinc/cli/v3/pkg/test-helpers"

testhelpers "github.com/confluentinc/cli/v3/pkg/test-helpers"

brianstrauch · 2023-08-23T22:13:32Z

pkg/test-helpers/mock.go

+			return mdsv1.AuthenticationResponse{
+				AuthToken: token,
+				TokenType: "JWT",
+				ExpiresIn: 100,
+			}, nil, nil


Suggested change

return mdsv1.AuthenticationResponse{

AuthToken: token,

TokenType: "JWT",

ExpiresIn: 100,

}, nil, nil

res := mdsv1.AuthenticationResponse{

AuthToken: token,

TokenType: "JWT",

ExpiresIn: 100,

}

return res, nil, nil

brianstrauch · 2023-08-23T22:13:56Z

pkg/test-helpers/mock.go

+	var mdsClient *mdsv1.APIClient
+	mdsConfig := mdsv1.NewConfiguration()
+	mdsClient = mdsv1.NewAPIClient(mdsConfig)


Suggested change

var mdsClient *mdsv1.APIClient

mdsConfig := mdsv1.NewConfiguration()

mdsClient = mdsv1.NewAPIClient(mdsConfig)

mdsConfig := mdsv1.NewConfiguration()

mdsClient := mdsv1.NewAPIClient(mdsConfig)

brianstrauch · 2023-08-23T22:16:05Z

pkg/test-helpers/mock.go

@@ -0,0 +1,55 @@
+package test_helpers


test_helpers is a really generic name and has the potential to get really big the more things we move into here. Why not put this in pkg/mock?

I get a circular import unfortunately.

try to fix it another way?

I can move it to mock, just not pkg/mock.

brianstrauch · 2023-08-24T20:44:20Z

internal/login/command_test.go

 	}
+
 	mdsClientManager := &climock.MDSClientManager{


Likewise, this looks like it should only be declared inside the if

brianstrauch · 2023-08-24T20:44:43Z

internal/login/command_test.go

+	logoutCmd := logout.New(cfg, prerunner, netrcHandler, authTokenHandler)
+	return logoutCmd


Suggested change

logoutCmd := logout.New(cfg, prerunner, netrcHandler, authTokenHandler)

return logoutCmd

return logout.New(cfg, prerunner, netrcHandler, authTokenHandler)

brianstrauch · 2023-08-24T20:45:26Z

internal/logout/command.go

 		username, err := c.netrcHandler.RemoveNetrcCredentials(c.cfg.IsCloudLogin(), c.Config.Config.Context().GetNetrcMachineName())
 		if err == nil {
 			log.CliLogger.Warnf(errors.RemoveNetrcCredentialsMsg, username, c.netrcHandler.GetFileName())
 		} else if !strings.Contains(err.Error(), "login credentials not found") && !strings.Contains(err.Error(), "keyword expected") {
 			// return err when other than NetrcCredentialsNotFoundErrorMsg or parsing error
 			return err
 		}
+
+		if isCCloud := ccloudv2.IsCCloudURL(ctx.Platform.Server, c.cfg.IsTest); isCCloud {


Suggested change

if isCCloud := ccloudv2.IsCCloudURL(ctx.Platform.Server, c.cfg.IsTest); isCCloud {

if ccloudv2.IsCCloudURL(ctx.Platform.Server, c.cfg.IsTest) {

brianstrauch · 2023-08-24T20:45:56Z

internal/logout/command_test.go

 	contextName := cfg.Context().Name
-	logoutCmd, cfg := newLogoutCmd(cfg, mockNetrcHandler)
+	// run login command


This comment seems wrong

internal/login/command_test.go

brianstrauch · 2023-08-25T00:53:23Z

pkg/cmd/prerunner.go

@@ -190,6 +190,10 @@ func (r *PreRun) Authenticated(command *AuthenticatedCLICommand) func(*cobra.Com
 		setContextErr := r.setAuthenticatedContext(command)
 		if setContextErr != nil {
 			if _, ok := setContextErr.(*errors.NotLoggedInError); ok {
+				if cmd.Use == "logout" {


Are there any other errors that could surface and cause a breaking change? We may want to re-evaluate this approach. If not, let's keep as is, but definitely write a comment here explaining why (and open a ticket to remove this hack in v4)

I tried some other scenarios and it looks like other errors can surface. If the login token expires, logout throws an expired token error, and if you try logging out again after that you get the not logged in error again (which is because although our current context exists, but we're no longer considered logged into it). These errors result from the fact that we're trying to have logout work exactly the same as before when the user is not logged in, despite it now being an authenticatedCLICommand.

I think we should probably just wait until v4 to add this since it doesn't look like all the possible breaking changes from errors can be avoided without putting several of these hacky exceptions for the logout command in the prerunner.

I think we should probably just wait until v4 to add this since it doesn't look like all the possible breaking changes from errors can be avoided without putting several of these hacky exceptions for the logout command in the prerunner.

Labeling as v4!

Going to revert the last two commits since we want the breaking changes to be visible.

revoking Auth0 token for cloud logout

9e1607d

kevin-wu24 requested review from a team as code owners August 22, 2023 20:01

Merge branch 'main' into CLI-2360

437adbf

brianstrauch reviewed Aug 22, 2023

View reviewed changes

internal/command.go Outdated Show resolved Hide resolved

internal/login/command_login_test.go Outdated Show resolved Hide resolved

moving shared functions to pkg and moving logout back to its own package

d467f4e

kevin-wu24 requested a review from brianstrauch August 22, 2023 21:28

brianstrauch reviewed Aug 22, 2023

View reviewed changes

kevin-wu24 requested a review from brianstrauch August 22, 2023 22:40

using default client and making logout an authenticated cmd

6b663a6

MuweiHe reviewed Aug 23, 2023

View reviewed changes

removing unnecessary code

08c4894

brianstrauch reviewed Aug 23, 2023

View reviewed changes

code review and cleanup

9c6c823

kevin-wu24 requested a review from brianstrauch August 23, 2023 17:07

brianstrauch reviewed Aug 23, 2023

View reviewed changes

kevin-wu24 added 2 commits August 23, 2023 15:32

cleanup unit test code + review

ca363c0

more cleanup

2eab151

kevin-wu24 requested a review from brianstrauch August 23, 2023 21:34

brianstrauch reviewed Aug 23, 2023

View reviewed changes

moving unit test mocks to mock dir

7972951

kevin-wu24 requested a review from brianstrauch August 24, 2023 19:23

brianstrauch reviewed Aug 24, 2023

View reviewed changes

code review

5267de9

brianstrauch reviewed Aug 24, 2023

View reviewed changes

internal/login/command_test.go Outdated Show resolved Hide resolved

kevin-wu24 added 2 commits August 24, 2023 16:38

hide not logged in error if already logged out

b4a2b7c

fixing integration test

bb6764b

brianstrauch reviewed Aug 25, 2023

View reviewed changes

brianstrauch added the v4 label Aug 25, 2023

revert change that hid 'not logged in' error for logout

4dfe398

	context = "Confluent Cloud"
	context = "Confluent Cloud"
	AuthenticatedCLICommand: pcmd.NewAuthenticatedCLICommand(cmd, prerunner),

	logoutCmd, cfg1 := newLogoutCmd(auth, userInterface, s.isCloud, req, mockNetrcHandler, AuthTokenHandler, mockLoginCredentialsManager, LoginOrganizationManager, ctx.Name)
	logoutCmd, cfg := newLogoutCmd(auth, userInterface, s.isCloud, req, mockNetrcHandler, AuthTokenHandler, mockLoginCredentialsManager, LoginOrganizationManager, ctx.Name)

	mdsClient.TokensAndAuthenticationApi = &mdsMock.TokensAndAuthenticationApi{
	mdsClient.TokensAndAuthenticationApi = &mdsmock.TokensAndAuthenticationApi{

	GetTokenFunc: func(ctx context.Context) (mdsv1.AuthenticationResponse, *http.Response, error) {
	GetTokenFunc: func(_ context.Context) (mdsv1.AuthenticationResponse, *http.Response, error) {

	testhelp "github.com/confluentinc/cli/v3/pkg/test-helpers"
	testhelpers "github.com/confluentinc/cli/v3/pkg/test-helpers"

		logoutCmd := logout.New(cfg, prerunner, netrcHandler, authTokenHandler)
		return logoutCmd

	logoutCmd := logout.New(cfg, prerunner, netrcHandler, authTokenHandler)
	return logoutCmd
	return logout.New(cfg, prerunner, netrcHandler, authTokenHandler)

	if isCCloud := ccloudv2.IsCCloudURL(ctx.Platform.Server, c.cfg.IsTest); isCCloud {
	if ccloudv2.IsCCloudURL(ctx.Platform.Server, c.cfg.IsTest) {

[CLI-2360] Revoking Auth0 refresh token on cloud logout #2216

Are you sure you want to change the base?

[CLI-2360] Revoking Auth0 refresh token on cloud logout #2216

Conversation

kevin-wu24 commented Aug 22, 2023 • edited by brianstrauch

Release Notes

Checklist

What

References

Test & Review

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

MuweiHe commented Aug 23, 2023 • edited

kevin-wu24 commented Aug 23, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevin-wu24 Aug 23, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevin-wu24 Aug 23, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevin-wu24 commented Aug 22, 2023 •

edited by brianstrauch

MuweiHe commented Aug 23, 2023 •

edited

kevin-wu24 commented Aug 23, 2023 •

edited

kevin-wu24 Aug 23, 2023 •

edited

kevin-wu24 Aug 23, 2023 •

edited