- Notifications
You must be signed in to change notification settings - Fork928
feat: Output username and password forcode server --dev#1193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecovbot commentedApr 27, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## main #1193 +/- ##==========================================+ Coverage 66.26% 66.28% +0.02%========================================== Files 265 265 Lines 16682 16681 -1 Branches 157 157 ==========================================+ Hits 11054 11057 +3+ Misses 4483 4481 -2+ Partials 1145 1143 -2
Continue to review full report at Codecov.
|
563b060 tod6410d9CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
🎉 Two in one day! Just a couple of minor things.
cli/server_test.go Outdated
| @@ -88,6 +92,9 @@ func TestServer(t *testing.T) { | |||
| require.NoError(t, err) | |||
| parsed, err := url.Parse(accessURL) | |||
| require.NoError(t, err) | |||
| // Verify that credentials were output to the terminal. | |||
| assert.Contains(t, stdoutBuf.String(), "email: admin@coder.com", "unexpected output") | |||
| assert.Contains(t, stdoutBuf.String(), "password: password", "unexpected output") | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nit: changemsgAndArgs to be different for both asserts
cli/server_test.go Outdated
| @@ -88,6 +92,9 @@ func TestServer(t *testing.T) { | |||
| require.NoError(t, err) | |||
| parsed, err := url.Parse(accessURL) | |||
| require.NoError(t, err) | |||
| // Verify that credentials were output to the terminal. | |||
| assert.Contains(t, stdoutBuf.String(), "email: admin@coder.com", "unexpected output") | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
minor: should we use the values fromdefaultDevUser instead of hard-coding? This would mean exporting it, but I don't see a huge issue there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sure, we can do that. I think hard-coding would be acceptable here too, as changes to the defaults may require rethinking some things, but either works for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm a fan of hard coding here too. Customers that use the product will likely depend on the credentials, so having tests fail if they change seems reasonable.
I don't think the |
e343748 to3cd7460Compare| // Verify that credentials were output to the terminal. | ||
| wantEmail := "email: admin@coder.com" | ||
| wantPassword := "password: password" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe we could exportdefaultDevUser so we could assert the output in tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That's definitely an option, wediscussed it a bit and I added a commit that this this but ultimately dropped it before the merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oops, I should've read back before I commented!
| var stdoutBuf bytes.Buffer | ||
| root.SetOutput(&stdoutBuf) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
small nit: if you're only ever callingbuffer.String() you can usestrings.Builder to avoid a copy on each call toString()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good point, I didn't considerstrings.Builder in this scenario although I did note the multiple calls toString() but thought it would be acceptable for a test. Would you like me to do a follow-up PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nope! Not important at all, just thought I'd mention.
@mafredri I think the constant password + the external access URL option poses a security risk. I suggest we randomly generate the password to be safe.
|
@ammario I'd be happy to look at fixing that. Should we move this to a new issue to narrow down the specs a bit? For instance, a user might want to override the random password with a self-defined one (thinking about the convenience of logging in). A way to achieve that could be an env variable, or perhaps storing the generated password in a dotfile or similar. |
It's a highly relevant one line fix so I don't think a new issue is necessary. I'm not sure what the best solution is... Writing to a dotfile makes sense as it achieves both stability and security. |
I'd probably be against a dotfile approach. Getting the initial login would be harder in ephemeral environments, plus we can't always assume we have a writable fs. |
That's fine... then we could default to a randomly generated one and let an environment variable override. Secure by default, stable if configured. |
Seems reasonable to me |
This PR allows the dev mode credentials (email and password) to be output to the terminal.
Fixes#825