iframe-proxy

williammartin · 2023-10-18T15:39:54Z

Description

In preparation for changing our configuration schema to support multiple accounts, I want gain confidence in our Config, particularly the pieces that relate to the AuthConfig structure and reach down into the config and keyring. I actually couldn't find any tests that provided coverage for this even as an integration.

There will need to also be some tests added to the go-gh/auth package which is called through here and reaches into the config.

I've left some comments on areas I want to call out.

williammartin · 2023-10-18T15:53:57Z

+
+	// When the user has logged in insecurely
+	authCfg := newTestAuthConfig()
+	ghConfig.Read = func() (*ghConfig.Config, error) {


A bit sad 😬

I dont think this is necessary:

cfg, err := NewConfig() require.NoError(t, err) authCfg := cfg.Authentication()

I don't think what you're proposing here actually resolves my sadness so just to be clear about why I'm doing this NewConfig calls to ghConfig.Read which uses sync.Once to load config from disk. That means that any test from that point onward that calls ghConfig.Read will not be isolated.

Ah I see. I misunderstood the purpose. That makes sense to me.

I think what I'll do is actually put it inside the newTestAuthConfig function or another new function and comment it so that the reason can be discovered from any test.

williammartin · 2023-10-18T15:54:38Z

+	require.ErrorIs(t, err, keyring.ErrNotFound)
+}
+
+// Note that I'm not sure this test enforces particularly desirable behaviour


Calling this out for attention.

The reasoning behind this behavior was that if logout fails what actions can the user actually take? The thought was that there really was none and thus we shouldn't show them an error. Not saying I agree, just some context.

Depends on the user I suppose. A user that cares that their token is still hanging around on a shared machine could care.

I don't feel a pressing need to change it but I wanted to leave a note for any maintainer that comes across this in future. In fact, I'll update the comment to capture your added context.

williammartin · 2023-10-18T15:55:02Z

 		return val, err
 	}

-	if defaultExists(key) {


I dunno, feel free to tell me to move it back but it felt pretty analogous to a map.

I like the change.

williammartin · 2023-10-18T15:55:11Z


 // GitProtocol will retrieve the git protocol for the logged in user at the given hostname.
 // If none is set it will return the default value.
+// TODO: although this returns an error, it actually has no path to error.


Calling this out for attention.

Let's change the function signature then, it is only used in two places. Interestingly enough this function should replace the majority of places GetOrDefault is used. Would love to clean that up as I find GetOrDefault to be a bit of an eyesore.

williammartin · 2023-10-18T15:55:16Z

 // Logout will remove user, git protocol, and auth token for the given hostname.
 // It will remove the auth token from the encrypted storage if it exists there.
 func (c *AuthConfig) Logout(hostname string) error {
+	// TODO: I can't find anywhere that expects to call this function without a hostname


Calling this out for attention.

I think this was in place for legacy reasons, probably okay to remove now.

williammartin · 2023-10-18T15:56:09Z

 		c.cfg.Set([]string{hosts, hostname, oauthToken}, token)
 		insecureStorageUsed = true
 	}
+	// TODO: I can't find anywhere that calls this function without a username, so that would seem like


Calling this out for attention.

I think this was in place for legacy reasons, probably okay to remove now.

williammartin · 2023-10-18T16:12:10Z

@@ -0,0 +1,302 @@
+package config


Note that I have not written tests for KnownHosts (because it has no extra behaviour on top of go-gh/auth right now so it was low priority) nor the AliasConfig because it is low risk for the multi account changes.

I have written tests for Hosts and DefaultHost now but I will leave AliasConfig out of this PR and do it in a follow up since this is already getting pretty hefty.

williammartin · 2023-10-18T16:13:24Z

+}
+
+func TestHasEnvTokenWithNoEnvTokenButAConfigVar(t *testing.T) {
+	t.Skip("this test is explicitly breaking some implementation assumptions")


Calling this out for attention.

williammartin · 2023-10-18T16:13:32Z

 			return true
 		}
 	}
+	// TODO: This is _extremely_ knowledgable about the implementation of TokenFromEnvOrConfig


Calling this out for attention.

Agreed. We should create a function HasEnvToken in go-gh that abstracts away this knowledge.

I'm going to punt that from this PR since it's a bigger change and this is already pretty hefty.

williammartin · 2023-10-19T07:41:46Z

+	requireKeyWithValue(t, authCfg.cfg, []string{hosts, "github.com", "user"}, "test-user")
+	requireKeyWithValue(t, authCfg.cfg, []string{hosts, "github.com", "git_protocol"}, "ssh")


I should test these using GitProtocol and User instead. That would be a better black box test.

samcoe

@williammartin This is excellent, thanks for being diligent about improving our test coverage! I left inline code comments, let me know if you have any questions about them.

samcoe · 2023-10-19T08:04:40Z

+
+	// When the user has logged in insecurely
+	authCfg := newTestAuthConfig()
+	ghConfig.Read = func() (*ghConfig.Config, error) {


I dont think this is necessary:

cfg, err := NewConfig() require.NoError(t, err) authCfg := cfg.Authentication()

samcoe · 2023-10-19T08:12:56Z

+
+	// When the user is authenticated via env var
+	authCfg := newTestAuthConfig()
+	ghConfig.Read = func() (*ghConfig.Config, error) {


Same comment as above.

samcoe · 2023-10-19T08:21:21Z

+	require.ErrorIs(t, err, keyring.ErrNotFound)
+}
+
+// Note that I'm not sure this test enforces particularly desirable behaviour


The reasoning behind this behavior was that if logout fails what actions can the user actually take? The thought was that there really was none and thus we shouldn't show them an error. Not saying I agree, just some context.

samcoe · 2023-10-19T08:22:08Z

 		return val, err
 	}

-	if defaultExists(key) {


I like the change.

samcoe · 2023-10-19T08:23:14Z

-	}
-
-	val, err = c.cfg.Get([]string{key})
+	val, err := c.Get(hostname, key)


samcoe · 2023-10-19T08:25:41Z

 			return true
 		}
 	}
+	// TODO: This is _extremely_ knowledgable about the implementation of TokenFromEnvOrConfig


Agreed. We should create a function HasEnvToken in go-gh that abstracts away this knowledge.

samcoe · 2023-10-19T08:27:00Z

+	// can guarantee that tokens will only be returned from a set env var.
+	// Discussed here, but maybe worth revisiting: https://github.com/cli/cli/pull/7169#discussion_r1136979033
+	//
+	// By providing example.com. it's also _only_ looking for GH_ENTERPRISE_TOKEN or GITHUB_ENTERPRISE_TOKEN


It checks GH_TOKEN and GITHUB_TOKEN as well.

Ohhh sorry, I missed that the isEnterprise branch of tokenForHost would fall through. Thanks for correcting me!

samcoe · 2023-10-19T08:31:56Z


 // GitProtocol will retrieve the git protocol for the logged in user at the given hostname.
 // If none is set it will return the default value.
+// TODO: although this returns an error, it actually has no path to error.


Let's change the function signature then, it is only used in two places. Interestingly enough this function should replace the majority of places GetOrDefault is used. Would love to clean that up as I find GetOrDefault to be a bit of an eyesore.

samcoe · 2023-10-19T08:33:27Z

 		c.cfg.Set([]string{hosts, hostname, oauthToken}, token)
 		insecureStorageUsed = true
 	}
+	// TODO: I can't find anywhere that calls this function without a username, so that would seem like


I think this was in place for legacy reasons, probably okay to remove now.

samcoe · 2023-10-19T08:33:41Z

 // Logout will remove user, git protocol, and auth token for the given hostname.
 // It will remove the auth token from the encrypted storage if it exists there.
 func (c *AuthConfig) Logout(hostname string) error {
+	// TODO: I can't find anywhere that expects to call this function without a hostname


I think this was in place for legacy reasons, probably okay to remove now.

This is to avoid the impression that the keyring matters, when the test is actually about general auth behaviour

williammartin · 2023-10-19T11:00:19Z

I believe I've addressed all comments @samcoe and have explicitly excluded:

Testing AliasConfig
Moving HasEnvToken into gh-auth

In both cases, I felt like this PR was already pretty hefty and they were both larger distractions from the multi account confidence that I wanted to get out of this.

samcoe

Wonderful! Thanks for making the requested changes. This looks ready to ship. Glad we are one step closer to removing GetOrDefault.

samcoe · 2023-10-19T12:54:25Z

+
+	// Then the token is successfully fetched
+	// and the source is set to oauth_token but this isn't great
+	// but I can't find the issue # that references this.


I think cli/go-gh#94 is what you are looking for.

Yes! I just could not find it anywhere

williammartin added 7 commits October 18, 2023 15:51

Add initial tests for config Get

b94f458

Add tests for config GetOrDefault

5f645ea

Add initial AuthConfig tests

34b0b33

Add tests for AuthConfig Login and Logout

614e492

Update AuthConfig to use default fn

52ba99e

Add tests for AuthConfig Token

07110ec

Mod tidy go-md2man

465474e

williammartin force-pushed the wm/add-tests-for-existing-config branch from b80aca5 to 465474e Compare October 18, 2023 15:52

Add tests for AuthConfig HasEnvToken

e011633

williammartin commented Oct 18, 2023

View reviewed changes

williammartin requested a review from samcoe October 18, 2023 16:14

williammartin commented Oct 19, 2023

View reviewed changes

samcoe reviewed Oct 19, 2023

View reviewed changes

williammartin added 12 commits October 19, 2023 12:05

Rewrite AuthConfig User and GitProtocol tests

3380831

Add additional comment on Logout ignoring errors

cb7672e

Add test that host is added on Login

e0bc2ff

Share config.Read setup in AuthConfig tests

07f5ca4

Add tests for AuthConfig DefaultHost

fa80802

Avoid using keyring unnecessarily in AuthConfig tests

c9de817

This is to avoid the impression that the keyring matters, when the test is actually about general auth behaviour

Ensure all AuthConfig tests are isolated on disk

c430da7

Remove Logout branch that handles empty hostname

aa88484

Remove Login branch that handles empty username

3c3fbcd

Remove incorrect comment about HasEnvToken

b9cbee0

Fix GitProtocol signature and rework test descriptions

7d8c1af

Remove GetOrDefault uses in favour of GitProtocol

6dc04bb

williammartin marked this pull request as ready for review October 19, 2023 10:58

williammartin requested a review from a team as a code owner October 19, 2023 10:58

cliAutomation assigned williammartin Oct 19, 2023

samcoe approved these changes Oct 19, 2023

View reviewed changes

Add link to token source issue

4fc1999

williammartin merged commit 26f3601 into trunk Oct 19, 2023

williammartin deleted the wm/add-tests-for-existing-config branch October 19, 2023 13:59

samcoe mentioned this pull request Oct 25, 2023

Globally set git protocol is ignored in some places #8250

Closed

		requireKeyWithValue(t, authCfg.cfg, []string{hosts, "github.com", "user"}, "test-user")
		requireKeyWithValue(t, authCfg.cfg, []string{hosts, "github.com", "git_protocol"}, "ssh")

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

williammartin commented Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

williammartin Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

samcoe left a comment

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!

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

williammartin commented Oct 18, 2023 •

edited

Loading

williammartin Oct 19, 2023 •

edited

Loading

samcoe left a comment •

edited

Loading