Switch hashes to SHA-256 and implement token limitations #1263
Conversation
| @@ -10,12 +11,37 @@ export const TokenScope = { | |||
| export type TokenScopeKeys = (typeof TokenScope)[keyof typeof TokenScope] | |||
|
|
|||
| export const TokenActions = { | |||
| ImageRead: 'image:read', | |||
| ModelRead: 'model:read', | |||
There was a problem hiding this comment.
These all seem very similar to the permissions defined in the authorisation actions. My concern here is duplication and confusion in how they are similar but not the same- eg in the authorisation actions there's access_request:create and access_request:view and in this file you've defined access_request:write and access_request:read.
There was a problem hiding this comment.
To avoid duplication these token actions should be exposed via the API so clients know what token actions are available to them
There was a problem hiding this comment.
To avoid having to have a separate mapping to authorisation actions and to improve readability, each of these definitions should include an array of the authorisation actions they contain as well as the existing name of the token action eg 'model:read'
| @@ -110,22 +111,68 @@ export async function getTokenFromAuthHeader(header: string) { | |||
| return token | |||
| } | |||
|
|
|||
| export async function validateTokenForModel(token: TokenDoc, modelId: string, action: TokenActionsKeys) { | |||
| export async function validateTokenForUse(token: TokenDoc | undefined, action: TokenActionsKeys): Promise<Response> { | |||
There was a problem hiding this comment.
How could token be undefined when the only place this method is used is in an if statement that checks if it's defined? Additionally, why should lack of a token just grant you access?
| return Promise.all( | ||
| schemas.map(async (schema) => { | ||
| // Is this a constrained user token. | ||
| if (user.token && (await validateTokenForUse(user.token, ActionLookup[action]))) { |
There was a problem hiding this comment.
validateTokenForUse() is used in if statement and returns an object not a boolean? What's the point in the method returning information if you're not going to use it?
| @@ -133,6 +158,15 @@ export class BasicAuthorisationConnector { | |||
| [ReleaseAction.View]: ModelAction.View, | |||
| } | |||
|
|
|||
| // Is this a constrained user token. | |||
| if (user.token?.actions && !user.token.actions.includes(ActionLookup[action])) { | |||
There was a problem hiding this comment.
Why not use validateTokenForUse() in places like this?
There was a problem hiding this comment.
Also, you don't check the model restriction on the token?
| @@ -77,6 +80,15 @@ export class BasicAuthorisationConnector { | |||
| async models(user: UserInterface, models: Array<ModelDoc>, action: ModelActionKeys): Promise<Array<Response>> { | |||
| return Promise.all( | |||
| models.map(async (model) => { | |||
| // Is this a constrained user token. | |||
| if (user.token && (await validateTokenForModel(user.token, model.id, ActionLookup[action]))) { | |||
There was a problem hiding this comment.
validateTokenForModel() no longer returns a boolean?
| } | ||
|
|
||
| export async function validateTokenForModel( | ||
| token: TokenDoc | undefined, |
There was a problem hiding this comment.
Same as with the other method- the sole use of this function already checks for the token's presence and why should the lack of a token just grant access?
backend/src/services/token.ts
Outdated
| success: true, | ||
| } | ||
| } | ||
|
|
||
| if (token.scope === 'models' && !token.modelIds.includes(modelId)) { |
| }) | ||
| } else { | ||
| throw new Error('Unexpected hash type: ' + this.hashMethod) | ||
| } | ||
| }) | ||
|
|
||
| TokenSchema.methods.compareToken = function compareToken(candidateToken: string) { |
There was a problem hiding this comment.
Could/ should this function be in the service? Are there unit tests for this as it's a very important function?
No description provided.