Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ public String createManager(@Context GraphManager manager,
AuthManager authManager = manager.authManager();
validUser(authManager, user);

String creator = HugeGraphAuthProxy.getContext().user().username();
String creator = HugeGraphAuthProxy.username();
Copy link
Member

Choose a reason for hiding this comment

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

⚠️ Important: 全局一致性检查

建议搜索整个代码库中所有 HugeGraphAuthProxy.getContext().user().username() 的调用点,确保都已替换为新的 username() 方法,避免遗漏其他潜在的 NPE 风险点。

可以使用以下命令检查:

grep -r "HugeGraphAuthProxy.getContext().user().username()" --include="*.java"

switch (type) {
case SPACE:
validGraphSpace(manager, graphSpace);
Expand Down Expand Up @@ -124,7 +124,7 @@ public void delete(@Context GraphManager manager,
AuthManager authManager = manager.authManager();
validType(type);
validUser(authManager, user);
String actionUser = HugeGraphAuthProxy.getContext().user().username();
String actionUser = HugeGraphAuthProxy.username();

switch (type) {
case SPACE:
Expand Down Expand Up @@ -193,7 +193,7 @@ public String checkRole(@Context GraphManager manager,

validType(type);
AuthManager authManager = manager.authManager();
String user = HugeGraphAuthProxy.getContext().user().username();
String user = HugeGraphAuthProxy.username();

boolean result;
switch (type) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ public Object create(@Context GraphManager manager,
}
}

String creator = HugeGraphAuthProxy.getContext().user().username();
String creator = HugeGraphAuthProxy.username();

if (StringUtils.isNotEmpty(clone)) {
// Clone from existing graph
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ public String create(@Context GraphManager manager,

jsonGraphSpace.checkCreate(false);

String creator = HugeGraphAuthProxy.getContext().user().username();
String creator = HugeGraphAuthProxy.username();
GraphSpace exist = manager.graphSpace(jsonGraphSpace.name);
E.checkArgument(exist == null, "The graph space '%s' has existed",
jsonGraphSpace.name);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,7 @@ public static Context setAdmin() {
public static Context getContext() {
// Return task context first
String taskContext = TaskManager.getContext();

User user = User.fromJson(taskContext);
if (user != null) {
return new Context(user);
Expand Down Expand Up @@ -953,6 +954,14 @@ public void updateTime(Date updateTime) {
this.hugegraph.updateTime(updateTime);
}

public static String username() {
Copy link
Member

Choose a reason for hiding this comment

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

⚠️ Important: 缺少单元测试

这是一个 NPE 修复,应该添加单元测试来验证:

  1. 非认证模式下调用 username() 的行为
  2. getContext() 返回 null 时的处理
  3. 各个调用方(ManagerAPI, GraphsAPI, GraphSpaceAPI)在非认证模式下的正确性

建议添加测试类或测试方法覆盖这些场景。

Context context = HugeGraphAuthProxy.getContext();
Copy link
Member

Choose a reason for hiding this comment

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

⚠️ Important: 空指针安全性不完整

虽然修复了 context 为 null 的情况,但没有处理 context.user 为 null 的场景。建议增加防御性检查:

Suggested change
Context context = HugeGraphAuthProxy.getContext();
public static String username() {
Context context = HugeGraphAuthProxy.getContext();
if (context == null || context.user == null) {
return "anonymous";
}
return context.user.username();
}

或者检查一下 Context 的构造函数是否保证 user 字段非空。

if (context == null) {
return "anonymous";
}
return context.user.username();
Copy link
Member

Choose a reason for hiding this comment

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

‼️ Critical: 返回值设计问题

新增的 username() 方法在非认证模式下返回 "anonymous",但需要考虑:

  1. 业务逻辑正确性: 在 ManagerAPI.createManager()GraphSpaceAPI.create() 中,这个 username 被用作 creator 字段。如果在非认证模式下所有创建者都是 "anonymous",会导致无法追踪真实的资源创建者。

  2. 建议方案:

    • 如果非认证模式下不需要追踪创建者,应该返回 null 而不是 "anonymous",让调用方明确处理
    • 或者在调用方检查认证模式,非认证模式下使用其他标识(如 IP、session ID 等)
Suggested change
return context.user.username();
public static String username() {
Context context = HugeGraphAuthProxy.getContext();
if (context == null || context.user == null) {
return null; // 让调用方明确处理非认证场景
}
return context.user.username();
}

请确认非认证模式下的业务预期行为。

Copy link

Copilot AI Jan 4, 2026

Choose a reason for hiding this comment

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

Direct field access context.user is inconsistent with the rest of the codebase. The Context class provides a public user() method that should be used instead. All other usages in the codebase access the user through the method call context.user() (e.g., lines 203, 1171, 1172, 1202, 1458, 1489). Change context.user.username() to context.user().username() to maintain consistency.

Copilot uses AI. Check for mistakes.
}
Comment on lines +957 to +963
Copy link

Copilot AI Jan 4, 2026

Choose a reason for hiding this comment

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

The username() method should be placed near other static utility methods (lines 155-204) rather than in the middle of instance methods. This improves code organization and makes it easier to locate static methods. Consider moving this method to be placed after the getContext() method around line 196, which it depends on.

Copilot uses AI. Check for mistakes.

private <V> Cache<Id, V> cache(String prefix, long capacity,
long expiredTime) {
String name = prefix + "-" + this.hugegraph.spaceGraphName();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import org.apache.hugegraph.core.RoleElectionStateMachineTest;
import org.apache.hugegraph.unit.api.filter.PathFilterTest;
import org.apache.hugegraph.unit.auth.HugeGraphAuthProxyTest;
import org.apache.hugegraph.unit.cache.CacheManagerTest;
import org.apache.hugegraph.unit.cache.CacheTest;
import org.apache.hugegraph.unit.cache.CachedGraphTransactionTest;
Expand Down Expand Up @@ -117,6 +118,7 @@
PageStateTest.class,
SystemSchemaStoreTest.class,
RoleElectionStateMachineTest.class,
HugeGraphAuthProxyTest.class,

/* serializer */
BytesBufferTest.class,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,198 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.hugegraph.unit.auth;

import java.lang.reflect.Method;

import org.apache.hugegraph.auth.HugeAuthenticator;
import org.apache.hugegraph.auth.HugeGraphAuthProxy;
import org.apache.hugegraph.auth.RolePermission;
import org.apache.hugegraph.task.TaskManager;
import org.apache.hugegraph.testutil.Assert;
import org.apache.hugegraph.unit.BaseUnitTest;
import org.junit.After;
import org.junit.Test;

public class HugeGraphAuthProxyTest extends BaseUnitTest {

private static HugeGraphAuthProxy.Context setContext(
HugeGraphAuthProxy.Context context) {
try {
Method method = HugeGraphAuthProxy.class.getDeclaredMethod(
"setContext",
HugeGraphAuthProxy.Context.class);
method.setAccessible(true);
return (HugeGraphAuthProxy.Context) method.invoke(null, context);
} catch (Exception e) {
throw new RuntimeException(e);
}
}

@After
public void tearDown() {
// Clean up contexts after each test
HugeGraphAuthProxy.resetContext();
TaskManager.resetContext();
}

@Test
public void testUsernameWithNullContext() {
// Ensure no context is set
HugeGraphAuthProxy.resetContext();
TaskManager.resetContext();

// When context is null, username() should return "anonymous"
String username = HugeGraphAuthProxy.username();
Assert.assertEquals("anonymous", username);
}

@Test
public void testUsernameWithValidContext() {
// Create a user with a specific username
HugeAuthenticator.User user = new HugeAuthenticator.User(
"test_user",
RolePermission.admin()
);

// Set context with this user
HugeGraphAuthProxy.Context context = new HugeGraphAuthProxy.Context(user);
setContext(context);

// username() should return the user's username
String username = HugeGraphAuthProxy.username();
Assert.assertEquals("test_user", username);
}

@Test
public void testUsernameWithAdminUser() {
// Test with ADMIN user
HugeAuthenticator.User adminUser = HugeAuthenticator.User.ADMIN;
HugeGraphAuthProxy.Context context = new HugeGraphAuthProxy.Context(
adminUser);
setContext(context);

String username = HugeGraphAuthProxy.username();
Assert.assertEquals("admin", username);
}

@Test
public void testGetContextReturnsNull() {
// Ensure both TaskManager context and CONTEXTS are null
HugeGraphAuthProxy.resetContext();
TaskManager.resetContext();

HugeGraphAuthProxy.Context context = HugeGraphAuthProxy.getContext();
Assert.assertNull(context);
}

@Test
public void testGetContextFromThreadLocal() {
// Set context via setContext (which sets CONTEXTS ThreadLocal)
HugeAuthenticator.User user = new HugeAuthenticator.User(
"thread_local_user",
RolePermission.admin()
);
HugeGraphAuthProxy.Context expectedContext = new HugeGraphAuthProxy.Context(
user);
setContext(expectedContext);

// Ensure TaskManager context is null
TaskManager.resetContext();

// getContext() should return the context from CONTEXTS ThreadLocal
HugeGraphAuthProxy.Context context = HugeGraphAuthProxy.getContext();
Assert.assertNotNull(context);
Assert.assertEquals("thread_local_user", context.user().username());
}

@Test
public void testGetContextFromTaskManager() {
// Clear CONTEXTS ThreadLocal
HugeGraphAuthProxy.resetContext();

// Create a user and set it in TaskManager context
HugeAuthenticator.User user = new HugeAuthenticator.User(
"task_user",
RolePermission.admin()
);
String userJson = user.toJson();
TaskManager.setContext(userJson);

// getContext() should return context from TaskManager
HugeGraphAuthProxy.Context context = HugeGraphAuthProxy.getContext();
Assert.assertNotNull(context);
Assert.assertEquals("task_user", context.user().username());
}

@Test
public void testGetContextPrioritizesTaskManager() {
// Set both TaskManager context and CONTEXTS ThreadLocal
HugeAuthenticator.User taskUser = new HugeAuthenticator.User(
"task_user",
RolePermission.admin()
);
String taskUserJson = taskUser.toJson();
TaskManager.setContext(taskUserJson);

HugeAuthenticator.User threadUser = new HugeAuthenticator.User(
"thread_user",
RolePermission.admin()
);
HugeGraphAuthProxy.Context threadContext = new HugeGraphAuthProxy.Context(
threadUser);
setContext(threadContext);

// getContext() should prioritize TaskManager context
HugeGraphAuthProxy.Context context = HugeGraphAuthProxy.getContext();
Assert.assertNotNull(context);
Assert.assertEquals("task_user", context.user().username());
}

@Test
public void testGetContextWithNullTaskManagerJson() {
// Clear CONTEXTS ThreadLocal
HugeGraphAuthProxy.resetContext();

// Set null in TaskManager
TaskManager.setContext(null);

// getContext() should return null
HugeGraphAuthProxy.Context context = HugeGraphAuthProxy.getContext();
Assert.assertNull(context);
}

@Test
public void testUsernameAfterResetContext() {
// Set a context first
HugeAuthenticator.User user = new HugeAuthenticator.User(
"temp_user",
RolePermission.admin()
);
HugeGraphAuthProxy.Context context = new HugeGraphAuthProxy.Context(user);
setContext(context);

// Verify it's set
Assert.assertEquals("temp_user", HugeGraphAuthProxy.username());

// Reset context
HugeGraphAuthProxy.resetContext();

// username() should now return "anonymous"
Assert.assertEquals("anonymous", HugeGraphAuthProxy.username());
}
}
Loading