-
Notifications
You must be signed in to change notification settings - Fork 8.9k
feature: enforce account initialization and disable default credentials #7261
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Also, we need to consider how to handle this logic in the build workflow. That's why the workflow below does not terminate. |
I resolved the problem in 78dd208 by adding environment variables during the CI stage. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 2.x #7261 +/- ##
=========================================
Coverage 55.76% 55.77%
- Complexity 7468 7473 +5
=========================================
Files 1178 1178
Lines 41962 41962
Branches 4923 4923
=========================================
+ Hits 23399 23403 +4
+ Misses 16323 16320 -3
+ Partials 2240 2239 -1 🚀 New features to boost your workflow:
|
| user.setUsername(username); | ||
| user.setPassword(new BCryptPasswordEncoder().encode(password)); | ||
| public void init() throws IOException { | ||
| String envUsername = System.getenv("SEATA_CONSOLE_USERNAME"); |
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.
Requiring users to read credentials (username and password) directly from environment variables would impose significant changes to their workflow and create a poor user experience. Instead, we can default to a predefined username (e.g., seata) and remove any default password. If no password is configured, the system will automatically generate a random password, log it for user reference, and notify them:
"No password was configured. A random password has been generated for security purposes. You may either:
- Use the auto-generated password (see logs for details), or
- Set a custom password in the configuration."
This approach ensures backward compatibility while guiding users toward secure practices.
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.
Requiring users to read credentials (username and password) directly from environment variables would impose significant changes to their workflow and create a poor user experience. Instead, we can default to a predefined username (e.g., seata) and remove any default password. If no password is configured, the system will automatically generate a random password, log it for user reference, and notify them:
"No password was configured. A random password has been generated for security purposes. You may either:
- Use the auto-generated password (see logs for details), or
- Set a custom password in the configuration."
This approach ensures backward compatibility while guiding users toward secure practices.
Thank you for your comment!
As you pointed out, suddenly requiring users to provide credentials would lead to a poor user experience.
Following your suggestion, I will implement it so that if no password is set in the YAML configuration file, the system will generate a random password, log it, and provide guidance to the user.
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.
Done in a02be95 !
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.
The modified result is now being logged as shown below!
22:56:58.549 INFO --- [ main] [y.CustomUserDetailsServiceImpl] [ init] [] : No password was configured. A random password has been generated for security purposes. You may either:
1. Use the auto-generated password: [424e287f]
2. Set a custom password in the configuration.
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.
Well done
|
The problem currently occurs only with JDK 17, and I'm in the process of analyzing it. The most likely cause at this point appears to be a compatibility issue between |
See #7329 |
funky-eyes
left a comment
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.
LGTM @slievrly PTAL
|
I don't think this should be merged right now. I'll make the necessary changes and provide a more detailed explanation afterward. |
During the login process, the password is compared using This has now been corrected by updating the code to encrypt passwords using PTAL 🚀
@Test
public void loginSuccess_shouldReturnTokenAndAddToHeader(CapturedOutput output) throws Exception {
String logs = output.getOut();
Pattern pattern = Pattern.compile("Use the auto-generated password: \\[(.+?)\\]");
Matcher matcher = pattern.matcher(logs);
assertTrue(matcher.find(), "captured password not found in logs");
String extractedPassword = matcher.group(1);
User user = new User("seata", extractedPassword);
String userJson = objectMapper.writeValueAsString(user);
MvcResult result = mockMvc.perform(post("/api/v1/auth/login")
.contentType(MediaType.APPLICATION_JSON)
.content(userJson))
.andExpect(status().isOk())
.andExpect(jsonPath("$.success").value(true))
.andExpect(jsonPath("$.data").isNotEmpty())
.andExpect(header().exists(WebSecurityConfig.AUTHORIZATION_HEADER))
.andReturn();
String authHeader = result.getResponse().getHeader(WebSecurityConfig.AUTHORIZATION_HEADER);
assertNotNull(authHeader);
assert (authHeader.startsWith(WebSecurityConfig.TOKEN_PREFIX));
}
@Test
public void loginSuccess_shouldReturnTokenAndAddToHeader() throws Exception {
User user = new User("seata", "foo");
String userJson = objectMapper.writeValueAsString(user);
MvcResult result = mockMvc.perform(post("/api/v1/auth/login")
.contentType(MediaType.APPLICATION_JSON)
.content(userJson))
.andExpect(status().isOk())
.andExpect(jsonPath("$.success").value(true))
.andExpect(jsonPath("$.data").isNotEmpty())
.andExpect(header().exists(WebSecurityConfig.AUTHORIZATION_HEADER))
.andReturn();
String authHeader = result.getResponse().getHeader(WebSecurityConfig.AUTHORIZATION_HEADER);
assertNotNull(authHeader);
assert (authHeader.startsWith(WebSecurityConfig.TOKEN_PREFIX));
} |
|
Since an integration test for login has been added, I believe it's okay to resume work on the issue below. |
slievrly
left a comment
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.
LGTM
Ⅰ. Describe what this PR did
fix #6396
fix #7307
Ⅱ. Does this pull request fix one issue?
Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews