feat: Output username and password for code server --dev
#1193
Conversation
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 to
d6410d9
Compare
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.
nit: change msgAndArgs 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.
minor: should we use the values from defaultDevUser 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.
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.
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 to
3cd7460
Compare
|
|
||
| // Verify that credentials were output to the terminal. | ||
| wantEmail := "email: admin@coder.com" | ||
| wantPassword := "password: password" |
There was a problem hiding this comment.
Maybe we could export defaultDevUser so we could assert the output in tests
There was a problem hiding this comment.
That's definitely an option, we discussed 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.
Oops, I should've read back before I commented!
| var stdoutBuf bytes.Buffer | ||
| root.SetOutput(&stdoutBuf) |
There was a problem hiding this comment.
small nit: if you're only ever calling buffer.String() you can use strings.Builder to avoid a copy on each call to String()
There was a problem hiding this comment.
Good point, I didn't consider strings.Builder in this scenario although I did note the multiple calls to String() 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.
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