fix: rewrite instance access func with proper auth

[minottic]

Description

It changes from a model based on job payload fields on creation to solely relying on user groups. This fixes the bug that prevented users to submit mixed ownerGroup datasets (part of the user). It also refactors the god function to smaller units, split by responsability

Tests included

• Included for each change/fix?
• Passing?

Documentation

• swagger documentation updated (required for API changes)
• official documentation updated

official documentation info

Summary by Sourcery

Refine job creation authorization to rely on user group memberships, enforce dataset access consistently, and clarify error handling for job submission failures.

Bug Fixes:

• Allow job submission with datasets owned by multiple groups as long as the submitting user belongs to all relevant groups.
• Correct HTTP status codes and error messages for invalid or unauthorized job creation scenarios.
• Fix access group configuration parsing so privileged and delete-job groups are correctly interpreted from comma-separated environment variables.

Enhancements:

• Refactor job creation authorization logic into smaller, focused helper methods for initializing jobs, determining user privileges, and validating dataset access.

Tests:

• Update existing job authorization tests to match the new error semantics and status codes, and add coverage for mixed-owner dataset scenarios with multi-group users.

[sourcery-ai]

Hey - I've found 3 issues, and left some high level feedback:

• In processJobUser, when a privileged user does not specify ownerUser, jobUser is set to null, causing checkDatasetsAccess to use an empty requestUserGroups for privileged users; consider falling back to the current user's groups when jobUser is null so dataset access checks reflect the real requester.
• The helper method name isPriviligedUser contains a typo; renaming it to isPrivilegedUser (and updating all call sites) will improve readability and avoid confusion.
• initJobInstance takes jobConfiguration as an argument but does not use it; either remove the unused parameter or make use of it if it is intended to influence job initialization.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `processJobUser`, when a privileged user does not specify `ownerUser`, `jobUser` is set to `null`, causing `checkDatasetsAccess` to use an empty `requestUserGroups` for privileged users; consider falling back to the current user's groups when `jobUser` is `null` so dataset access checks reflect the real requester.
- The helper method name `isPriviligedUser` contains a typo; renaming it to `isPrivilegedUser` (and updating all call sites) will improve readability and avoid confusion.
- `initJobInstance` takes `jobConfiguration` as an argument but does not use it; either remove the unused parameter or make use of it if it is intended to influence job initialization.

## Individual Comments

### Comment 1
<location path="src/jobs/jobs.controller.utils.ts" line_range="291-300" />
<code_context>
-  async instanceAuthorizationJobCreate(
+  private initJobInstance(
     jobCreateDto: CreateJobDto,
-    user: JWTUser,
-  ): Promise<JobClass> {
-    // NOTE: We need JobClass instance because casl module works only on that.
</code_context>
<issue_to_address>
**suggestion:** Align the `user` parameter type with its actual usage (nullable vs non-null).

`user` is handled as optional (e.g. `if (!user && jobCreateDto.ownerGroup)` and within `processJobUser`/`checkDatasetsAccess`), but the signatures for `instanceAuthorizationJobCreate` and helpers (`isAdminUser`, `isJobCreationPrivilegedUser`, `isPriviligedUser`) declare `user: JWTUser` as non-nullable. Please update these signatures to `user: JWTUser | null` (or `JWTUser | undefined`) and adjust internal checks to match the actual usage.

Suggested implementation:

```typescript
  private isAdminUser(user: JWTUser | null): boolean {

```

```typescript
  private isJobCreationPrivilegedUser(user: JWTUser | null): boolean {

```

```typescript
  private isPriviligedUser(user: JWTUser | null): boolean {

```

Because I cannot see the bodies of `isAdminUser`, `isJobCreationPrivilegedUser`, and `isPriviligedUser`, you should also:

1. Add a null guard at the top of each helper, for example:
   - `if (!user) { return false; }`
   This ensures you don't access properties on `user` when it is `null`.
2. If there are any other methods in this file that declare `user: JWTUser` but are used in a nullable way (e.g., guarded by `if (!user)`), update their signatures to `user: JWTUser | null` and add a similar null guard.
3. If there are call sites passing `undefined` instead of `null`, you may prefer `JWTUser | null | undefined` for full alignment with runtime behavior.
</issue_to_address>

### Comment 2
<location path="src/jobs/jobs.controller.utils.ts" line_range="336" />
<code_context>
-        await this.datasetsService.count(datasetsWhere);
-      datasetsNoAccess = datasetIds.length - numberOfDatasetsWithAccess.count;
-    }
+  private isPriviligedUser(user: JWTUser): boolean {
+    return this.isAdminUser(user) || this.isJobCreationPrivilegedUser(user);
+  }
</code_context>
<issue_to_address>
**nitpick (typo):** Fix the spelling of `isPriviligedUser` for clarity and consistency.

Please rename this to `isPrivilegedUser` and update all call sites to keep naming consistent and make the method easier to find.

Suggested implementation:

```typescript
  private isPrivilegedUser(user: JWTUser): boolean {
    return this.isAdminUser(user) || this.isJobCreationPrivilegedUser(user);
  }

```

1. Search in `src/jobs/jobs.controller.utils.ts` (and any related files, if this class is extended/used elsewhere) for all usages of `isPriviligedUser(` and rename them to `isPrivilegedUser(`.
2. If there are any unit tests, mocks, or stubs referencing `isPriviligedUser`, update those names as well to keep tests compiling and consistent.
</issue_to_address>

### Comment 3
<location path="src/jobs/jobs.controller.utils.ts" line_range="392" />
<code_context>
+    jobInstance: JobClass,
+  ) {
+    if (!user) return null;
+    let jobUser: JWTUser | null = cloneDeep(user);
+    const userGroups = new Set(user?.currentGroups ?? []);
+    if (this.isPriviligedUser(user)) {
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid unnecessary `cloneDeep` on `user` in `processJobUser` to reduce overhead.

`jobUser` is deep-cloned from `user` but then either replaced by `findByUsername2JWTUser` or set to `null`, and in the non-privileged path it stays identical to `user`. Since `user` is not mutated here, you can initialize `let jobUser: JWTUser | null = user;` and remove `cloneDeep` to avoid unnecessary allocation and deep copy.

Suggested implementation:

```typescript
  ) {
    if (!user) return null;
    let jobUser: JWTUser | null = user;
    const userGroups = new Set(user?.currentGroups ?? []);

```

Search the top of `src/jobs/jobs.controller.utils.ts` for an import of `cloneDeep` (e.g. `import cloneDeep from 'lodash/cloneDeep';` or `import { cloneDeep } from 'lodash';`) and remove it if it is no longer used anywhere in the file to keep the imports clean.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[Junjiequan]

It seems that the jobs system has its own auth managing logic per user, instead of solely relying on CASL ability, which we should avoid. Once we start doing that, we lose the point of using CASL ability as the single source of truth for access management.
This is not related to this PR, since the old one was already doing that. I think at some point we will have to deal with the tech debt of having a separated access logic implementation. But this refactor seems to be mitigating that cost.

[Junjiequan]

It's hard to thoroughly understand the entire changes since I'm not very familiar with the whole job system, but it seems to be mainly refactoring and breaking down big functions into smaller pieces. Additionally, the job system has quite decent test coverage, so as long as the current tests pass I'm happy for it to be merged.