Adding Enforcement checks for Directiveshandler and RecipeHandler

[sahusanket]

No description provided.

[gemini-code-assist]

Code Review

This pull request introduces authorization enforcement checks across various endpoints in DirectivesHandler and RecipeHandler when the WRANGLER_WORKSPACE_AUTH_CHECK feature flag is enabled. It utilizes ContextAccessEnforcer to enforce permissions such as CREATE, LIST, DELETE, GET, USE, and UPDATE on workspace and recipe entities. The review feedback highlights a few issues with the implementation: first, the respond helper method does not catch UnauthorizedException, which will result in a 500 Internal Server Error instead of a 401/403 status code; second, the upload endpoint should dynamically enforce CREATE or UPDATE permissions depending on whether the workspace already exists; and third, the uploadData endpoint should enforce UPDATE instead of CREATE since it operates on an existing workspace.

[gemini-code-assist]

The respond overload used in DirectivesHandler (defined in AbstractWranglerHandler.java as protected <T> void respond(HttpServiceRequest request, HttpServiceResponder responder, String namespace, NamespacedResponder<T> callable)) does not catch UnauthorizedException.\n\nAs a result, any authorization enforcement failure (which throws UnauthorizedException) will fall through to the generic catch (Throwable t) block, returning a 500 Internal Server Error instead of the correct 403 Forbidden or 401 Unauthorized status code.\n\nPlease update the respond overload in AbstractWranglerHandler.java to catch and handle UnauthorizedException properly, similar to how it is handled in the other respond overload.

[gemini-code-assist]

The upload endpoint can either create a new workspace or overwrite/update an existing one if a workspace with the same name already exists.\n\nBy only enforcing StandardPermission.CREATE, a user who has permission to create workspaces but not update existing ones could overwrite and modify an existing workspace. Conversely, a user who has UPDATE permission but not CREATE permission would be blocked from updating an existing workspace via this endpoint.\n\nConsider checking if the workspace exists inside the transaction, and dynamically enforcing StandardPermission.UPDATE or StandardPermission.CREATE based on its existence:\n\njava\n return TransactionRunners.run(getContext(), context -> {\n WorkspaceDataset ws = WorkspaceDataset.get(context);\n if (authEnforcementEnabled) {\n StandardPermission permission = ws.hasWorkspace(id) ? StandardPermission.UPDATE : StandardPermission.CREATE;\n contextAccessEnforcer.enforce(new WorkspaceEntityId(ns.getName(), id.getId()), permission);\n }\n

[gemini-code-assist]

The uploadData endpoint is used to upload data to an existing workspace and does not create a new one (it throws WorkspaceNotFoundException if the workspace is not found). Therefore, it should enforce StandardPermission.UPDATE instead of StandardPermission.CREATE.

Suggested change

	if (authEnforcementEnabled) {
	contextAccessEnforcer.enforce(new WorkspaceEntityId(ns.getName(), id), StandardPermission.CREATE);
	}
	if (authEnforcementEnabled) {
	contextAccessEnforcer.enforce(new WorkspaceEntityId(ns.getName(), id), StandardPermission.UPDATE);
	}

[vsethi09]

Should this be USE or UPDATE?

See similar patterns used in CDAP.

[vsethi09]

Should this be USE or UPDATE?

[sahusanket]

USE

[vsethi09]

Revert removal of line.

[vsethi09]

Add checks for all Methods, including directives config.

[vsethi09]

Same feature flag should not be used for Recipes resource.

[vsethi09]

Auth check should happen before any store reads / writes.

[vsethi09]

Does this class exist? I can't find it.

[vsethi09]

There is a repeated pattern of code, which can be improved:

      if (authEnforcementEnabled) {
        contextAccessEnforcer.enforce(new WorkspaceEntityId(ns.getName(), id), StandardPermission.CREATE);
      }