Allow to password protect shares #1252

alvaroaleman · 2021-01-25T22:35:33Z

This changes allows to password protect shares. It works by:

Allowing to optionally pass a password when creating a share
If set, the password + salt that is configured via a new flag will be
hashed via bcrypt and the hash stored together with the rest of the
share
Additionally, a random 96 byte long token gets generated and stored
as part of the share
When the backend retrieves an unauthenticated request for a share
that has authentication configured, it will return a http 401
The frontend detects this and will show a login prompt
The actual download links are protected via an url arg that contains
the previously generated token. This allows us to avoid buffering the
download in the browser and allows pasting the link without breaking
it

Thanks a lot for all the existing work on filebrowser btw, it's great!

Fixes #1223

ramiresviana

This PR is not working correctly as the way it is, you need to do some testing. Also found some issues:

Unprotected shares cannot be accessed.
There's no feedback when user type password wrong.

http/public.go

frontend/src/views/Share.vue

frontend/src/components/prompts/Share.vue

alvaroaleman · 2021-02-02T14:16:32Z

This PR is not working correctly as the way it is, you need to do some testing

@ramiresviana can you elaborate on that, what exactly is not working correctly? Or do you just mean the two issues you mentioned?

ramiresviana · 2021-02-02T17:37:19Z

This PR is not working correctly as the way it is, you need to do some testing

@ramiresviana can you elaborate on that, what exactly is not working correctly? Or do you just mean the two issues you mentioned?

The most important aspect of this feature was not working, the password validation. On my tests i was able to access any share by providing a wrong password. Also found another issue:

Downloading selected files, using the select button on the header bar, is not possible. Looks like the download url is missing the token.

alvaroaleman · 2021-02-04T03:13:42Z

@ramiresviana addressed all feedback:

Passwords are now properly checked
Also added some tests for that
Shares that are created without password now end up actually not having a password
Entering a wrong password will result in an error being shown
Selecting files and then downloading them via the button at the top works
No inline styling
Localized placeholder

Please have another look.

ramiresviana

What about using JWT based authentication? I think would be a more secure approach.

frontend/src/views/Share.vue

frontend/src/components/prompts/Share.vue

alvaroaleman · 2021-02-08T14:19:24Z

What about using JWT based authentication? I think would be a more secure approach.

First, if you have conceptional remarks about a change, it would be awesome to give those in the first round of feedback, not in the second one. Second, can you elaborate on how exactly JWT would be more secure? The password is sent exactly once, that will be needed however auth works. The token can be any random value which must be hard to guess but making it a JWT or not will not make anything more or less secure.

ramiresviana · 2021-02-08T17:43:20Z

What about using JWT based authentication? I think would be a more secure approach.

First, if you have conceptional remarks about a change, it would be awesome to give those in the first round of feedback, not in the second one. Second, can you elaborate on how exactly JWT would be more secure? The password is sent exactly once, that will be needed however auth works. The token can be any random value which must be hard to guess but making it a JWT or not will not make anything more or less secure.

Just wanted to hear your opinion about JWT, that's not an requirement. I was worried about the fact that the share token is always the same, with the JWT standard the token needs to be refreshed periodically and the frontend ~~do not have access to the payload~~ (it has access to payload but cannot generate a valid token).

alvaroaleman · 2021-02-14T22:41:55Z

Just wanted to hear your opinion about JWT, that's not an requirement. I was worried about the fact that the share token is always the same, with the JWT standard the token needs to be refreshed periodically and the frontend do not have access to the payload (it has access to payload but cannot generate a valid token).

Well, I guess that depends on the expectation. Right now the download links from the share will work indefinitely for as long as that share exists, which is nice for example if you want to be able to pass around a link that will work without making the share public. If I wanted to brute force a filebrowser share, I would most likely try to brute force the password instead, because I would assume that it is almost always shorter than the 96 byte of the token and that it will in most cases be easier to guess.

alvaroaleman · 2021-02-15T19:39:07Z

@o1egl Can you have a look, please?

o1egl · 2021-02-16T16:34:34Z

.circleci/config.yml

@@ -2,7 +2,7 @@ version: 2
 jobs:
  lint:
    docker:
-      - image: golangci/golangci-lint:v1.27.0
+      - image: golangci/golangci-lint:v1.31.0


The last version is v1.36.0. Either upgrade to the latest or revert it.

It might not look like that, but the version was a very conscious choice, it had to be updated to something with go 1.15 to have support for *testing.T.TempDir which was added in go 1.15. This is the oldest version of the golangci-lint that uses go 1.15.

Updating it to v1.36.0 results in a bunch of new failures, because filebrowser doesn't have a golangci-lint config, so it uses whatever its default linters are, which is not stable across versions.

cmd/root.go

frontend/src/api/files.js

o1egl · 2021-02-16T16:49:27Z

frontend/src/components/prompts/Share.vue

+      password: '',
+      passwordPermalink: ''


Why is it 2 different variables?

Because its two different inputs

frontend/src/i18n/en.json

http/public_test.go

http/share.go

o1egl · 2021-02-16T18:06:37Z

share/storage.go

+			if len(links) > i+1 {
+				links = append(links[:i], links[i+1:]...)
+			}


Why this change is introduced?

Because a bunch of methods in storage.go unconditionally access links[i+1:] which results in an out of bounds panic if the last link expired. I will remove it from this PR.

This check doesn't save from data race

Sorry, what data race do you mean? It stops accessing an array in an unsafe manner and risking an out of bounds

o1egl · 2021-02-16T18:08:05Z

users/storage.go

@@ -17,6 +17,15 @@ type StorageBackend interface {
 	DeleteByUsername(string) error
 }

+type Interface interface {


Please give a proper name to this interface. UserStore?

The linter will refuse that due to stuttering, package symbols should not start with the package name. This ends up being used as users.Interface which IMHO is totally reasonable

I just gave an example. It can be named Store. An interface name should be meaningful and self explainable. Interface name is too generic.

Renamed it to Store, please take another look

alvaroaleman

@o1egl all feedback addressed, ptal

alvaroaleman · 2021-02-17T01:35:29Z

.circleci/config.yml

@@ -2,7 +2,7 @@ version: 2
 jobs:
  lint:
    docker:
-      - image: golangci/golangci-lint:v1.27.0
+      - image: golangci/golangci-lint:v1.31.0


It might not look like that, but the version was a very conscious choice, it had to be updated to something with go 1.15 to have support for *testing.T.TempDir which was added in go 1.15. This is the oldest version of the golangci-lint that uses go 1.15.

Updating it to v1.36.0 results in a bunch of new failures, because filebrowser doesn't have a golangci-lint config, so it uses whatever its default linters are, which is not stable across versions.

alvaroaleman · 2021-02-17T01:39:07Z

frontend/src/components/prompts/Share.vue

+      password: '',
+      passwordPermalink: ''


Because its two different inputs

alvaroaleman · 2021-02-17T01:42:03Z

http/public.go

@@ -94,3 +104,26 @@ var publicDlHandler = withHashFile(func(w http.ResponseWriter, r *http.Request,

 	return rawDirHandler(w, r, d, file)
 })
+
+func authenticateShareRequest(r *http.Request, l *share.Link, salt string) (int, error) {


An error here means something went wrong with our code and yes, in that case the status code can be ignored. Statuscode != 0 means the verification failed. The distinction is important, because they result in different http codes. Our code erroring is a 5XX, the verification failing is a 4XX, if we return an error in both cases, we can not distinguish those two anymore

alvaroaleman · 2021-02-17T01:52:39Z

share/storage.go

+			if len(links) > i+1 {
+				links = append(links[:i], links[i+1:]...)
+			}


Because a bunch of methods in storage.go unconditionally access links[i+1:] which results in an out of bounds panic if the last link expired. I will remove it from this PR.

alvaroaleman · 2021-02-17T02:00:45Z

users/storage.go

@@ -17,6 +17,15 @@ type StorageBackend interface {
 	DeleteByUsername(string) error
 }

+type Interface interface {


The linter will refuse that due to stuttering, package symbols should not start with the package name. This ends up being used as users.Interface which IMHO is totally reasonable

alvaroaleman · 2021-02-25T17:42:19Z

@o1egl any chance you can have another look?

o1egl · 2021-03-01T09:01:48Z

@alvaroaleman Get Permanent Link returns 400 error. It tries to decode an empty body

o1egl · 2021-03-01T09:13:09Z

frontend/src/api/share.js

  url = removePrefix(url)
  url = `/api/share${url}`
  if (expires !== '') {
    url += `?expires=${expires}&unit=${unit}`
  }
-
+  let body = '';


The server expects to receive a JSON. An empty string is not a correct JSON.

@o1egl fixed

This changes allows to password protect shares. It works by: * Allowing to optionally pass a password when creating a share * If set, the password + salt that is configured via a new flag will be hashed via bcrypt and the hash stored together with the rest of the share * Additionally, a random 96 byte long token gets generated and stored as part of the share * When the backend retrieves an unauthenticated request for a share that has authentication configured, it will return a http 401 * The frontend detects this and will show a login prompt * The actual download links are protected via an url arg that contains the previously generated token. This allows us to avoid buffering the download in the browser and allows pasting the link without breaking it

alvaroaleman force-pushed the share branch 3 times, most recently from 9d3a301 to 52a98c6 Compare January 25, 2021 22:47

ramiresviana suggested changes Feb 2, 2021

View reviewed changes

http/public.go Outdated Show resolved Hide resolved

frontend/src/views/Share.vue Outdated Show resolved Hide resolved

frontend/src/components/prompts/Share.vue Outdated Show resolved Hide resolved

frontend/src/components/prompts/Share.vue Outdated Show resolved Hide resolved

alvaroaleman force-pushed the share branch 6 times, most recently from 46fba45 to a35b468 Compare February 4, 2021 03:04

alvaroaleman force-pushed the share branch 2 times, most recently from 3d1b278 to 2d2d42e Compare February 4, 2021 03:29

ramiresviana suggested changes Feb 8, 2021

View reviewed changes

alvaroaleman force-pushed the share branch from 2d2d42e to a935cf6 Compare February 14, 2021 22:31

alvaroaleman force-pushed the share branch from a935cf6 to 4d53e5b Compare February 14, 2021 22:46

ramiresviana approved these changes Feb 15, 2021

View reviewed changes

o1egl requested changes Feb 16, 2021

View reviewed changes

alvaroaleman force-pushed the share branch from 4d53e5b to 58ae3ab Compare February 17, 2021 02:11

alvaroaleman commented Feb 17, 2021

View reviewed changes

alvaroaleman force-pushed the share branch 3 times, most recently from ef55a1b to 2409822 Compare February 18, 2021 01:45

o1egl requested changes Mar 1, 2021

View reviewed changes

alvaroaleman force-pushed the share branch from 2409822 to ef89c89 Compare March 1, 2021 22:34

o1egl approved these changes Mar 2, 2021

View reviewed changes

o1egl merged commit d8f415f into filebrowser:master Mar 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to password protect shares #1252

Allow to password protect shares #1252

alvaroaleman commented Jan 25, 2021

ramiresviana left a comment

alvaroaleman commented Feb 2, 2021 •

edited

ramiresviana commented Feb 2, 2021

alvaroaleman commented Feb 4, 2021

ramiresviana left a comment

alvaroaleman commented Feb 8, 2021

ramiresviana commented Feb 8, 2021 •

edited

alvaroaleman commented Feb 14, 2021

alvaroaleman commented Feb 15, 2021

o1egl Feb 16, 2021

alvaroaleman Feb 17, 2021

o1egl Feb 16, 2021

alvaroaleman Feb 17, 2021

o1egl Feb 16, 2021

alvaroaleman Feb 17, 2021

o1egl Feb 17, 2021

alvaroaleman Feb 18, 2021

o1egl Feb 16, 2021

alvaroaleman Feb 17, 2021

o1egl Feb 17, 2021

alvaroaleman Feb 18, 2021

alvaroaleman left a comment

alvaroaleman Feb 17, 2021

alvaroaleman Feb 17, 2021

alvaroaleman Feb 17, 2021

alvaroaleman Feb 17, 2021

alvaroaleman Feb 17, 2021

alvaroaleman commented Feb 25, 2021

o1egl commented Mar 1, 2021

o1egl Mar 1, 2021

alvaroaleman Mar 1, 2021

Allow to password protect shares #1252

Allow to password protect shares #1252

Conversation

alvaroaleman commented Jan 25, 2021

ramiresviana left a comment

Choose a reason for hiding this comment

alvaroaleman commented Feb 2, 2021 • edited

ramiresviana commented Feb 2, 2021

alvaroaleman commented Feb 4, 2021

ramiresviana left a comment

Choose a reason for hiding this comment

alvaroaleman commented Feb 8, 2021

ramiresviana commented Feb 8, 2021 • edited

alvaroaleman commented Feb 14, 2021

alvaroaleman commented Feb 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvaroaleman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvaroaleman commented Feb 25, 2021

o1egl commented Mar 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvaroaleman commented Feb 2, 2021 •

edited

ramiresviana commented Feb 8, 2021 •

edited