Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@tus/server: introduce middleware for access control #460

Closed
Murderlon opened this issue Jul 19, 2023 · 6 comments · Fixed by #471
Closed

@tus/server: introduce middleware for access control #460

Murderlon opened this issue Jul 19, 2023 · 6 comments · Fixed by #471

Comments

@Murderlon
Copy link
Member

Access control can be done in many ways. We don't want to be opinionated on that so implementing a generic middleware function which is used before all requests seems to be the best approach.

@fenos
Copy link
Collaborator

fenos commented Jul 25, 2023

One way we can implement this is to have an option on the TUS server instance ex: onTusRequest function with the following signature:

new Server({
  ...,
  onTusRequest: (request, upload) => {
      // custom logic
      throw {
        status_code: 403,
        body: "Unauthorized"
      }
  }
})

another approach could be to use a sort of pipeline pattern and chain custom handlers

async function authorizer(request, upload) {
   // custom logic
      throw {
        status_code: 403,
        body: "Unauthorized"
      }
}

function uploadPrefix(request, upload) {
   upload.addPrefix('some-prefix')
}

const server = new Server({...})

server.use(authorizer)
server.use(uploadPrefix)

server.handle(request, response)

The handler should be invoked after the Upload object is constructed since we might want to use the upload-id in order to determine the permission on that specific path

@Acconut
Copy link
Member

Acconut commented Jul 25, 2023

I think a simple callback is sufficient here. Providing it with the current upload and HTTP request should be sufficient to give users enough information to determine the authorization. I was also wondering if it would make sense to differentiate between read (HEAD, GET requests and upload concatenation) versus write (DELETE, PATCH) access. Are there situations where you want to allow a user to have read but no write access to an upload?

@fenos
Copy link
Collaborator

fenos commented Jul 26, 2023

@Acconut Yes! Absolutely - some user might have permission on GET but not on POST or DELETE

Since we have the request object passed in, I thought we could use that to determine the TUS action / method, happy to explore alternatives in case we don't want the request object to be passed in directly

@Acconut
Copy link
Member

Acconut commented Aug 3, 2023

I thought we could use that to determine the TUS action / method

I don't think this is a sufficient approach because edge cases will likely not be covered by the users on their own. For example, if a POST request uses Upload-Concat, you also need to ensure that the end-user has read access to the uploads that get concatenated. That is something that users will easily miss and we should implement in tus-node-server. Does that make sense?

@Murderlon
Copy link
Member Author

don't think this is a sufficient approach because edge cases will likely not be covered by the users on their own

I'm not sure what more we can do internally regarding differentiating read/write access together with Upload-Concat?

@Acconut
Copy link
Member

Acconut commented Aug 25, 2023

I'm not sure what more we can do internally regarding differentiating read/write access together with Upload-Concat?

Let me explain this with a few examples:

  • For a normal POST request, we must check if the user is allowed to create an upload (and validate any metadata we get with it)
  • For a GET/HEAD request to upload a, we must check if the user has read access to upload a.
  • For a PATCH/DELETE request to upload a, we must check if the user has write access to upload a.
  • For a POST request with Upload-Concat: final;a,b, we must check if the user is allowed to create an upload and if they have read access to upload a and b.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants