[ISSUE #15476] Add plugin-owned AI visibility grant API#15513
[ISSUE #15476] Add plugin-owned AI visibility grant API#15513Zhengcy05 wants to merge 7 commits into
Conversation
|
Thanks for your this PR. 🙏 感谢您提交的PR。 🙏 |
|
@Zhengcy05 Thanks for the contribution. The overall direction—keeping visibility grant management inside the auth plugin and resolving domain resource metadata through an SPI—is reasonable. However, I think the RBAC mapping, permission lookup, database compatibility, and authentication scope need to be revised before this feature can be merged. Requesting changes for the following items. 1. Create one dedicated visibility role per userThe current implementation creates one internal role for every resource/action pair and encodes the namespace, resource type, resource name, and action into the role name. This causes several problems:
For this user-specific grant model, please create at most one dedicated internal visibility role for each user when the user receives their first visibility grant: The intended persistence model should be similar to: The dedicated role name must be deterministic, reserved for visibility grants, unique per user, and bounded to the existing 50-character column. Please do not attach these permissions to an arbitrary existing role. Existing roles may be shared by multiple users, so adding a user-specific visibility permission to such a role would grant access to every user bound to that role. Grant should:
Revoke should delete the corresponding permission. The empty internal role may either be retained or cleaned up. A partially created role without a permission must never grant visibility. 2. Keep the authorization chain as identity -> role -> permission
A role binding only establishes identity-to-role membership. It must not be treated as a permission by itself. Otherwise, an orphaned or manually created role can expose a private resource in list/search results even when the backing permission does not exist. Please derive visible resources from the actual permissions attached to the current user's dedicated visibility role: This keeps single-resource authorization and list/search/count authorization on the same permission source of truth. The resource grant-list API also needs the reverse lookup: Please add an exact persistence query and the required indexes for this path. It should not scan all roles and permissions or depend on decoding role names. Please also ensure that grant and revoke invalidate or refresh the relevant role/permission caches so that a successful API response has clearly defined visibility semantics. 3. Store the raw resource identifier and expand it to 512 charactersThe permission should store the original resource identifier: Using the current AI resource field limits, the complete identifier can require approximately 431 characters: The current built-in schemas are inconsistent:
For this PR, expanding Please update:
The MySQL With This fits the 3072-byte InnoDB index limit only under a compatible configuration, such as: The field was previously reduced from 255 to 128 because the unique index exceeded the historical 767-byte limit: #11306 Therefore, the migration documentation must include preflight checks such as: SELECT VERSION();
SHOW VARIABLES LIKE 'innodb_page_size';
SHOW VARIABLES LIKE 'innodb_default_row_format';
SHOW CREATE TABLE permissions;The MySQL migration should explicitly rebuild the table with a compatible row format while changing the column, for example: ALTER TABLE permissions
ROW_FORMAT=DYNAMIC,
MODIFY COLUMN resource VARCHAR(512) NOT NULL;The exact charset and collation should also be explicitly defined instead of silently inheriting the database default. Please confirm whether permission resource comparison is intended to be case-sensitive before selecting the collation. Users whose MySQL configuration cannot support the resulting index must be warned before upgrading rather than encountering a runtime permission-insert failure. 4. Handle database-independent indexing as a follow-upA database-independent SHA-256 index is the preferred long-term design, but it does not need to expand the scope of this PR. Please create a follow-up issue/design for a model such as: The unique index would become: To reduce database-type differences, The raw resource must remain the final authorization value. After looking up by hash, the implementation should still compare the raw resource using exact equality. The follow-up design should define the canonicalization rules for:
5. Match visibility resources exactly in memoryVisibility grants target one exact resource. The current default RBAC matcher treats stored permission resources as regular expressions. A resource name containing Please keep the original raw resource string in storage, but use exact in-memory matching for the reserved For example: if (permissionResource.startsWith(VISIBILITY_RESOURCE_PREFIX)) {
matched = permissionResource.equals(targetResource);
} else {
matched = existingWildcardMatch(permissionResource, targetResource);
}The persisted resource must not be regex-escaped or otherwise transformed. Existing wildcard behavior for Config/Naming permissions should remain unchanged. 6. Explicitly select the authentication scopeThe visibility grant endpoints rely on the service layer to resolve the authenticated However, the current With the distribution defaults: nacos.core.auth.enabled=false
nacos.core.auth.admin.enabled=truethe open API filter skips identity validation, while Please explicitly assign the intended authentication scope, likely
This behavior must be covered by auth-enabled integration tests. The current auth-disabled IT cannot validate the most important authorization semantics of this API. 7. Keep Prompt visibility integration in a separate issue/PRPrompt has not yet fully joined the standard visibility flow. Its read/detail/version/download and list paths do not consistently apply standard readable checks and pre-pagination visibility conditions. Refactoring Prompt visibility is outside the scope of this PR and should be handled by a separate issue and PR. However, this PR must not accept a Prompt visibility grant and return success when that permission cannot affect Prompt runtime behavior. Please introduce or validate a supported-resource-type capability so that the grant API only accepts resource types whose single-resource and list/search/count paths already support the standard visibility flow. Unsupported resource types should return a clear unsupported error until their visibility integration is complete. The currently supported resource types must also be documented in the visibility/auth specs. The same rule should apply to any other AI resource type that has not yet joined the standard visibility flow. Grant cleanup when a resource is deleted is not required in this PR and can remain a later lifecycle enhancement. 8. Align the API and specification textPlease use the same endpoint consistently in:
The implementation currently uses: but some text still refers to 9. Required testsIn addition to the current API contract tests, please cover:
Once these items are addressed, the plugin-owned API and locator SPI should provide a reasonable base for explicit visibility grants without coupling the generic resource controllers directly to the default auth implementation. |
|
One more need todo after this PR: Follow-up proposal: use a SHA-256 resource index for database-independent permission storageThe current PR can use For the long term, I suggest creating a separate issue/design to replace the raw resource string in permission indexes with a Java-computed SHA-256 digest. This would remove the index-length dependency on database charset, collation, page size, and database-specific index limits while retaining the original resource value for authorization. Proposed data modelThe A cross-database logical schema could use: resource VARCHAR(512) NOT NULL,
resource_hash CHAR(64) NOT NULLUsing lowercase hexadecimal The hash must be calculated in Java, not with database-specific SQL functions. Canonical hash inputThe implementation should first build the canonical raw resource identifier: It should then calculate: The canonicalization rules must be defined in the visibility/auth specification:
The hash should always be calculated from the exact canonical string that is stored in IndexesThe current unique index: should eventually be replaced with: The grant-list API also needs efficient reverse lookup from a resource to users, so an additional index should be considered: This supports both directions: |
|
And @Zhengcy05 Would you like provide your dingtalk number? |
|
Understood. I will focus on this PR first, which might take a bit longer. 🫡 |
My DingTalk account : 1ex-76mml35v5b |
Special note: Since there are many design changes this time, I sincerely hope someone will carefully review this PR. Thx😘
fixes #15476
What is the purpose of the change
This PR adds plugin-owned AI visibility grant management to the default auth plugin so resource owners or global admins can grant, list, and revoke explicit visibility permissions for AI resources.
It also wires those explicit grants into AI visibility queries, while keeping the API ownership and persistence model inside the auth plugin boundary.
Brief changelog
/v3/auth/ai/visibilitygrant/list/revoke APIs in the default auth pluginVisibilityResourceLocatorso the auth plugin can resolve AI resources and verify owner-based management authorityVerifying this change
mvn -pl plugin-default-impl/nacos-default-auth-plugin,test/openapi-test spotless:applymvn -pl plugin-default-impl/nacos-default-auth-plugin,test/openapi-test spotless:checkmvn -pl plugin-default-impl/nacos-default-auth-plugin -Dtest=DefaultAiVisibilityGrantServiceTest,NacosAuthPluginControllerConfigTest testmvn -pl test/openapi-test test-compileNote:
AiVisibilityGrantAuthApiITCasewas added for the new auth API flow.Follow this checklist to help us incorporate your contribution quickly and easily:
[ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.mvn -B clean package apache-rat:check spotbugs:check -DskipTeststo make sure basic checks pass. Runmvn clean installto make sure unit-test pass. Runmvn clean test-compile failsafe:integration-testto make sure integration-test pass.