Skip to content

Conversation

@YongGoose
Copy link
Member

@YongGoose YongGoose commented Mar 31, 2025

  • I have registered the PR changes.

Ⅰ. 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

@YongGoose
Copy link
Member Author

YongGoose commented Mar 31, 2025

@slievrly

Also, we need to consider how to handle this logic in the build workflow.
Currently, it expects username and password input, but input is not possible in a build workflow.

That's why the workflow below does not terminate.
https://github.com/apache/incubator-seata/actions/runs/14169655810/job/39690431239?pr=7261

@YongGoose
Copy link
Member Author

@slievrly

Also, we need to consider how to handle this logic in the build workflow. Currently, it expects username and password input, but input is not possible in a build workflow.

That's why the workflow below does not terminate. https://github.com/apache/incubator-seata/actions/runs/14169655810/job/39690431239?pr=7261

I resolved the problem in 78dd208 by adding environment variables during the CI stage.

@codecov
Copy link

codecov bot commented Apr 26, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 55.77%. Comparing base (ad07251) to head (2d81050).
Report is 1 commits behind head on 2.x.

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     

see 4 files with indirect coverage changes

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@funky-eyes funky-eyes changed the title Feature: enforce account initialization and disable default credentials eature: enforce account initialization and disable default credentials Apr 29, 2025
@funky-eyes funky-eyes changed the title eature: enforce account initialization and disable default credentials feature: enforce account initialization and disable default credentials Apr 29, 2025
user.setUsername(username);
user.setPassword(new BCryptPasswordEncoder().encode(password));
public void init() throws IOException {
String envUsername = System.getenv("SEATA_CONSOLE_USERNAME");
Copy link
Contributor

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:

  1. Use the auto-generated password (see logs for details), or
  2. Set a custom password in the configuration."

This approach ensures backward compatibility while guiding users toward secure practices.

Copy link
Member Author

@YongGoose YongGoose Apr 29, 2025

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:

  1. Use the auto-generated password (see logs for details), or
  2. 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in a02be95 !

Copy link
Member Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done

@funky-eyes funky-eyes added this to the 2.5.0 milestone Apr 29, 2025
@funky-eyes funky-eyes added module/console type: feature Category issues or prs related to feature request. labels Apr 29, 2025
@YongGoose YongGoose requested a review from funky-eyes April 29, 2025 13:56
@YongGoose
Copy link
Member Author

YongGoose commented Apr 29, 2025

The problem currently occurs only with JDK 17, and I'm in the process of analyzing it.

Caused by: java.lang.NoClassDefFoundError: Could not initialize class org.apache.catalina.startup.Tomcat
Caused by: java.lang.ExceptionInInitializerError

The most likely cause at this point appears to be a compatibility issue between JDK 17 and Tomcat version 9.0.99.
If this turns out to be the case, we plan to upgrade Tomcat to version 9.1.00 with separate PR.

@YongGoose
Copy link
Member Author

The problem currently occurs only with JDK 17, and I'm in the process of analyzing it.

Caused by: java.lang.NoClassDefFoundError: Could not initialize class org.apache.catalina.startup.Tomcat
Caused by: java.lang.ExceptionInInitializerError

The most likely cause at this point appears to be a compatibility issue between JDK 17 and Tomcat version 9.0.99. If this turns out to be the case, we plan to upgrade Tomcat to version 9.1.00 with separate PR.

See #7329

Copy link
Contributor

@funky-eyes funky-eyes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM @slievrly PTAL

@YongGoose
Copy link
Member Author

@funky-eyes @slievrly

I don't think this should be merged right now.
It could cause issues with login.

I'll make the necessary changes and provide a more detailed explanation afterward.

@YongGoose
Copy link
Member Author

YongGoose commented May 9, 2025

@funky-eyes @slievrly

I don't think this should be merged right now. It could cause issues with login.

I'll make the necessary changes and provide a more detailed explanation afterward.

During the login process, the password is compared using PasswordEncoder internally by the AuthenticationManager.
However, in the previous implementation, passwords were stored in plain text, which could have caused login failures.

This has now been corrected by updating the code to encrypt passwords using PasswordEncoder.
I ran the integration tests shown below, and since everything works as expected, I believe it's safe to proceed with the merge now.'

PTAL 🚀

Random password test

@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));
}

Property test

@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));
}

@YongGoose YongGoose requested a review from funky-eyes May 9, 2025 23:54
@YongGoose
Copy link
Member Author

Since an integration test for login has been added, I believe it's okay to resume work on the issue below.
What do you think?

Copy link
Member

@slievrly slievrly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@slievrly slievrly merged commit 50d7003 into apache:2.x May 11, 2025
9 checks passed
slievrly pushed a commit to slievrly/fescar that referenced this pull request Oct 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module/console type: feature Category issues or prs related to feature request.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

seata-server 需要配置加密 Prohibiting Default Username and Password Logins.

3 participants