feat: make user's accessToken available for jobv4 - #2761
Conversation
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Storing the raw JWT in the Job document is sensitive; consider either not persisting it (e.g., keep it only in memory/context) or at least marking the field
select: false/encrypting it so it cannot be read back from the database by default. - The
toJSON.transformhook will not protectaccessTokenforlean()queries or other serialization paths (e.g.,toObject()), so if the token must never be exposed externally you may want to add additional safeguards (e.g., schema-levelselect: false, explicit projections, or a dedicated DTO layer) to guarantee it never leaks.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Storing the raw JWT in the Job document is sensitive; consider either not persisting it (e.g., keep it only in memory/context) or at least marking the field `select: false`/encrypting it so it cannot be read back from the database by default.
- The `toJSON.transform` hook will not protect `accessToken` for `lean()` queries or other serialization paths (e.g., `toObject()`), so if the token must never be exposed externally you may want to add additional safeguards (e.g., schema-level `select: false`, explicit projections, or a dedicated DTO layer) to guarantee it never leaks.
## Individual Comments
### Comment 1
<location path="src/jobs/schemas/job.schema.ts" line_range="15-17" />
<code_context>
timestamps: true,
toJSON: {
getters: true,
+ transform: (_doc: Document, ret: Record<string, unknown>) => {
+ delete ret.accessToken;
+ return ret;
+ },
},
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Current hiding of `accessToken` only covers `toJSON`; consider also protecting it at query/serialization level.
This only protects API responses using `toJSON`; the field is still included by default in queries and in `toObject()` results. If `accessToken` is sensitive, also mark it as non-selectable (e.g. `select: false`) or use an equivalent mechanism so it can’t be exposed via other serialization paths that bypass this transform.
Suggested implementation:
```typescript
timestamps: true,
toJSON: {
getters: true,
transform: (_doc: Document, ret: Record<string, unknown>) => {
delete ret.accessToken;
return ret;
},
},
toObject: {
getters: true,
transform: (_doc: Document, ret: Record<string, unknown>) => {
delete ret.accessToken;
return ret;
},
},
})
export class JobClass extends OwnableClass {
```
To fully implement the suggestion and protect `accessToken` at the query level, you should also mark the `accessToken` field as non-selectable in its `@Prop` definition. For example, if the field currently looks like:
```ts
@Prop()
accessToken: string;
```
you should change it to:
```ts
@Prop({ select: false })
accessToken: string;
```
or, if there are already options:
```ts
@Prop({ type: String, select: false })
accessToken: string;
```
This ensures `accessToken` is excluded by default from query results and all serialization paths, while still allowing explicit inclusion via `.select('+accessToken')` when needed.
</issue_to_address>
### Comment 2
<location path="src/jobs/schemas/job.schema.ts" line_range="119-123" />
<code_context>
+ * Stored for reuse by actions performed within the job.
+ * Not exposed in API responses for security reasons.
+ */
+ @Prop({
+ type: String,
+ required: false,
+ })
+ accessToken?: string;
}
export const JobSchema = SchemaFactory.createForClass(JobClass);
</code_context>
<issue_to_address>
**🚨 issue (security):** Storing raw JWTs in the database may not be necessary and increases the blast radius of a DB compromise.
Persisting the full access token means a DB leak exposes reusable credentials until expiry. If you only need it to call downstream services, consider storing a less-sensitive representation (e.g. minimal claims or a reference/ID) or shortening its lifetime. If the full token must be stored, consider additional at-rest protection for this field (e.g. encryption).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Hmm, while I see the idea, maybe a different implementation would be better:
If we need the token to access non-scicat resources, it will get a bit more tricky as either a token exchange is needed anyway (the token audience field would not match) or maybe the new keycloak Identity Assertion JWT Grant ( see https://www.keycloak.org/2026/07/keycloak-2670-released) may later be used. |
|
I like the idea of a short-lived token that is generated, that could also work. I'll investigate. |
a09498b to
1f57b82
Compare
|
@bpedersen2 I changed the approach and at least in a first test it seems to work as expected. Does that look better from a safety perspective? |
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The JWT payload structure in
UsersService.createUserJWTchanged (usingcurrentGroupsand signing the entireJWTUserinstead of a{ username, groups }shape), which may break any consumers expecting the oldgroupsfield or a narrower payload; consider keeping backward-compatible field names or mapping to a minimal DTO for the token. JOB_TOKEN_EXPIRES_INis documented as required in the README but the configuration treats it as optional and falls back toJWT_EXPIRES_IN; align the env var documentation with the actual behavior so operators understand when they must set it.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The JWT payload structure in `UsersService.createUserJWT` changed (using `currentGroups` and signing the entire `JWTUser` instead of a `{ username, groups }` shape), which may break any consumers expecting the old `groups` field or a narrower payload; consider keeping backward-compatible field names or mapping to a minimal DTO for the token.
- `JOB_TOKEN_EXPIRES_IN` is documented as required in the README but the configuration treats it as optional and falls back to `JWT_EXPIRES_IN`; align the env var documentation with the actual behavior so operators understand when they must set it.
## Individual Comments
### Comment 1
<location path="src/users/users.service.ts" line_range="377-383" />
<code_context>
async createUserJWT(
accessToken: JWTUser | undefined,
+ expiresIn?: string,
): Promise<CreateUserJWT | null> {
const expiresInOption =
- this.configService.get<string>("jwt.expiresIn") || "1h";
+ expiresIn ||
+ this.configService.get<string>("jwt.jobTokenExpiresIn") ||
+ this.configService.get<string>("jwt.expiresIn") ||
+ "1h";
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `jwt.jobTokenExpiresIn` in `createUserJWT` affects all JWTs, not just job tokens
Because `createUserJWT` now prefers `jwt.jobTokenExpiresIn`, all callers that don’t pass `expiresIn` will use the job-specific expiry instead of the general `jwt.expiresIn`. That can unintentionally shorten the lifetime of non-job JWTs that reuse this helper. To keep job-token expiry scoped to job execution only, consider:
- Leaving `createUserJWT` defaulted to `jwt.expiresIn` / `jwt.neverExpires`, and
- Having `generateJobUserToken` pass an explicit `expiresIn` for job tokens, or
- Extracting job-token creation into a separate method.
Otherwise, this change risks altering the expiration behavior of all JWTs system-wide.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
concering the change of the payload from groups to currentGroups:
|
|
Any more comments @bpedersen2 ? Maybe you also want to take a look @sbliven ? |
…cessToken in handlebars
b317dcd to
cb592c8
Compare
Add accessToken to jobClass so it can be reused in actions via job.accessToken in handlebars
Description
the
accessTokenwith which the Jobv4 was submitted is added to the jobClass so that in a jobConfigyou can use
authorization: "Bearer {{job.accessToken}}"for authorization.This is especially useful if the job action performs a Scicat API call as the user which is not possible otherwise.
Motivation
configurable actions and jobs using urlactoin make it possible to perform arbitrary API calls, and with the proposed changes these calls can be done as the user that is currently logged in a submits a job to scicat.
Summary by Sourcery
Generate short-lived user JWTs for job executions and expose them to job templates to enable authenticated user-scoped actions.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: