Skip to content

read user.uuid from DB#12632

Open
DaanHoogland wants to merge 5 commits into4.20from
ghi12612-userUuidPreservation
Open

read user.uuid from DB#12632
DaanHoogland wants to merge 5 commits into4.20from
ghi12612-userUuidPreservation

Conversation

@DaanHoogland
Copy link
Contributor

Description

This PR implements the reporters suggestion as requested...

Fixes: #12612

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • Build/CI
  • Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

@codecov
Copy link

codecov bot commented Feb 11, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 4.15%. Comparing base (34f6f41) to head (8029ef3).
⚠️ Report is 1 commits behind head on 4.20.

❗ There is a different number of reports uploaded between BASE (34f6f41) and HEAD (8029ef3). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (34f6f41) HEAD (8029ef3)
unittests 1 0
Additional details and impacted files
@@              Coverage Diff              @@
##               4.20   #12632       +/-   ##
=============================================
- Coverage     16.25%    4.15%   -12.10%     
=============================================
  Files          5662      404     -5258     
  Lines        500141    32965   -467176     
  Branches      60728     5893    -54835     
=============================================
- Hits          81299     1370    -79929     
+ Misses       409759    31419   -378340     
+ Partials       9083      176     -8907     
Flag Coverage Δ
uitests 4.15% <ø> (ø)
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses issue #12612 where API-key authentication could yield a different (random) user UUID per request, by ensuring the user’s UUID is read from the database and propagated into the User object used in auth/context.

Changes:

  • Update the FIND_USER_ACCOUNT_BY_API_KEY query and result mapping to include and set u.uuid.
  • Adjust UserVO(long id) initialization to delegate to the no-arg constructor.
  • Extend the com.cloud.user.User interface to include setUuid(String).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
engine/schema/src/main/java/com/cloud/user/dao/AccountDaoImpl.java Adds UUID to the API-key lookup query and maps it into the User instance.
engine/schema/src/main/java/com/cloud/user/UserVO.java Changes the UserVO(long id) constructor to call this() before setting id.
api/src/main/java/com/cloud/user/User.java Adds setUuid(String uuid) to the User interface.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 40%)

See analysis details on SonarQube Cloud

Copy link
Collaborator

@abh1sar abh1sar left a comment

Choose a reason for hiding this comment

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

code LGTM

Copy link
Contributor

@sureshanaparti sureshanaparti left a comment

Choose a reason for hiding this comment

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

clgtm

@weizhouapache
Copy link
Member

@DaanHoogland
can we use this method UserAccountDao.getUserByApiKey instead ?

@DaanHoogland
Copy link
Contributor Author

@DaanHoogland can we use this method UserAccountDao.getUserByApiKey instead ?

no doubt, but can you be more specific? Do you mean, bypass findUserAccountByApiKey altogether? Or call UserAccountDao.getUserByApiKey from within it. I am sure you mean to suggest a cleanup refactor, but that is as far as my understanding goes.

Note findUserAccountByApiKey is called in three places. Happy to make this PR more elaborate.

@weizhouapache
Copy link
Member

@DaanHoogland can we use this method UserAccountDao.getUserByApiKey instead ?

no doubt, but can you be more specific? Do you mean, bypass findUserAccountByApiKey altogether? Or call UserAccountDao.getUserByApiKey from within it. I am sure you mean to suggest a cleanup refactor, but that is as far as my understanding goes.

Note findUserAccountByApiKey is called in three places. Happy to make this PR more elaborate.

@DaanHoogland
I had a quick look, I think remove the method accountDao.findUserAccountByApiKey(apiKey), and use this method instead

--- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java
+++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java
@@ -3051,7 +3051,9 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M
 
     @Override
     public Pair<User, Account> findUserByApiKey(String apiKey) {
-        return _accountDao.findUserAccountByApiKey(apiKey);
+        UserAccount userAccount = _userAccountDao.getUserByApiKey(apiKey);
+        // TODO: get User and Account from userAccount or from DB
+        return new Pair<>(user, account);
     }
 
     @Override

@DaanHoogland
Copy link
Contributor Author

@DaanHoogland can we use this method UserAccountDao.getUserByApiKey instead ?

no doubt, but can you be more specific? Do you mean, bypass findUserAccountByApiKey altogether? Or call UserAccountDao.getUserByApiKey from within it. I am sure you mean to suggest a cleanup refactor, but that is as far as my understanding goes.
Note findUserAccountByApiKey is called in three places. Happy to make this PR more elaborate.

@DaanHoogland I had a quick look, I think remove the method accountDao.findUserAccountByApiKey(apiKey), and use this method instead

--- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java
+++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java
@@ -3051,7 +3051,9 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M
 
     @Override
     public Pair<User, Account> findUserByApiKey(String apiKey) {
-        return _accountDao.findUserAccountByApiKey(apiKey);
+        UserAccount userAccount = _userAccountDao.getUserByApiKey(apiKey);
+        // TODO: get User and Account from userAccount or from DB
+        return new Pair<>(user, account);
     }
 
     @Override

ok, gave it a stab, AccountDoa.findUserAccountByApiKey(apiKey) could be removed like this. Some test cleanup and interface needed.

@weizhouapache
Copy link
Member

ok, gave it a stab, AccountDoa.findUserAccountByApiKey(apiKey) could be removed like this. Some test cleanup and interface needed.

good, look forward to your more changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AccountDaoImpl.findUserAccountByApiKey() generates random UUID instead of reading from database

4 participants