Repository: rclone/rclone
Pull request #6678 (2023-01-09 08:15:17)

lib/oauthutil: don't retry for probably fatal errors
According to the OAuth2 RFC (https://www.rfc-editor.org/rfc/rfc6749#section-5.2), the server should return a 400 (or maybe sometimes 401) for a list of unrecoverable errors, such as expired refresh tokens. Rclone shouldn't still make 5 attempts to get a token in those cases. @ncw do you see any potential problem with this?

#### Checklist

- [x] I have read the [contribution guidelines](https://github.com/rclone/rclone/blob/master/CONTRIBUTING.md#submitting-a-new-feature-or-bug-fix).
- [ ] ~~I have added tests for all changes in this PR if appropriate.~~
- [ ] ~~I have added documentation for the changes if appropriate.~~
- [x] All commit messages are in [house style](https://github.com/rclone/rclone/blob/master/CONTRIBUTING.md#commit-messages).
- [x] I'm done, this Pull Request is ready for review :-)


ncw: I think the idea looks like a good one. We shouldn't retry 400 errors. 401 too probably.

The way we do this in most backends is to call

https://github.com/rclone/rclone/blob/98fa93f6d17909e9c6b9778a4dc9e67c58151f6a/fs/fserrors/error.go#L440

And then have a list of HTTP error codes we will retry, eg

https://github.com/rclone/rclone/blob/98fa93f6d17909e9c6b9778a4dc9e67c58151f6a/backend/box/box.go#L318-L326

Is it worth using this method? That will retry fewer errors, but it should be a lot more targeted.



Patch:
diff --git a/backend/crypt/crypt.go b/backend/crypt/crypt.go
index b1c8111d0..f3eb8317d 100644
--- a/backend/crypt/crypt.go
+++ b/backend/crypt/crypt.go
@@ -235,7 +235,7 @@ func NewFs(ctx context.Context, name, rpath string, m configmap.Mapper) (fs.Fs,
 	// the features here are ones we could support, and they are
 	// ANDed with the ones from wrappedFs
 	f.features = (&fs.Features{
-		CaseInsensitive:         cipher.NameEncryptionMode() == NameEncryptionOff,
+		CaseInsensitive:         !cipher.dirNameEncrypt || cipher.NameEncryptionMode() == NameEncryptionOff,
 		DuplicateFiles:          true,
 		ReadMimeType:            false, // MimeTypes not supported with crypt
 		WriteMimeType:           false,
diff --git a/backend/storj/fs.go b/backend/storj/fs.go
index 9a0a5ebe2..ed3004099 100644
--- a/backend/storj/fs.go
+++ b/backend/storj/fs.go
@@ -23,6 +23,7 @@ import (
 	"golang.org/x/text/unicode/norm"
 
 	"storj.io/uplink"
+	"storj.io/uplink/edge"
 )
 
 const (
@@ -156,11 +157,12 @@ type Fs struct {
 
 // Check the interfaces are satisfied.
 var (
-	_ fs.Fs          = &Fs{}
-	_ fs.ListRer     = &Fs{}
-	_ fs.PutStreamer = &Fs{}
-	_ fs.Mover       = &Fs{}
-	_ fs.Copier      = &Fs{}
+	_ fs.Fs           = &Fs{}
+	_ fs.ListRer      = &Fs{}
+	_ fs.PutStreamer  = &Fs{}
+	_ fs.Mover        = &Fs{}
+	_ fs.Copier       = &Fs{}
+	_ fs.PublicLinker = &Fs{}
 )
 
 // NewFs creates a filesystem backed by Storj.
@@ -545,7 +547,7 @@ func (f *Fs) Put(ctx context.Context, in io.Reader, src fs.ObjectInfo, options .
 	defer func() {
 		if err != nil {
 			aerr := upload.Abort()
-			if aerr != nil {
+			if aerr != nil && !errors.Is(aerr, uplink.ErrUploadDone) {
 				fs.Errorf(f, "cp input ./%s %+v: %+v", src.Remote(), options, aerr)
 			}
 		}
@@ -560,6 +562,16 @@ func (f *Fs) Put(ctx context.Context, in io.Reader, src fs.ObjectInfo, options .
 
 	_, err = io.Copy(upload, in)
 	if err != nil {
+		if errors.Is(err, uplink.ErrBucketNotFound) {
+			// Rclone assumes the backend will create the bucket if not existing yet.
+			// Here we create the bucket and return a retry error for rclone to retry the upload.
+			_, err = f.project.EnsureBucket(ctx, bucketName)
+			if err != nil {
+				return nil, err
+			}
+			return nil, fserrors.RetryError(errors.New("bucket was not available, now created, the upload must be retried"))
+		}
+
 		err = fserrors.RetryError(err)
 		fs.Errorf(f, "cp input ./%s %+v: %+v\n", src.Remote(), options, err)
 
@@ -761,3 +773,55 @@ func (f *Fs) Copy(ctx context.Context, src fs.Object, remote string) (fs.Object,
 	// Return the new object
 	return newObjectFromUplink(f, remote, newObject), nil
 }
+
+// PublicLink generates a public link to the remote path (usually readable by anyone)
+func (f *Fs) PublicLink(ctx context.Context, remote string, expire fs.Duration, unlink bool) (string, error) {
+	bucket, key := f.absolute(remote)
+	if len(bucket) == 0 {
+		return "", errors.New("path must be specified")
+	}
+
+	// Rclone requires that a link is only generated if the remote path exists
+	if len(key) == 0 {
+		_, err := f.project.StatBucket(ctx, bucket)
+		if err != nil {
+			return "", err
+		}
+	} else {
+		_, err := f.project.StatObject(ctx, bucket, key)
+		if err != nil {
+			if !errors.Is(err, uplink.ErrObjectNotFound) {
+				return "", err
+			}
+			// No object found, check if there is such a prefix
+			iter := f.project.ListObjects(ctx, bucket, &uplink.ListObjectsOptions{Prefix: key + "/"})
+			if iter.Err() != nil {
+				return "", iter.Err()
+			}
+			if !iter.Next() {
+				return "", err
+			}
+		}
+	}
+
+	sharedPrefix := uplink.SharePrefix{Bucket: bucket, Prefix: key}
+
+	permission := uplink.ReadOnlyPermission()
+	if expire.IsSet() {
+		permission.NotAfter = time.Now().Add(time.Duration(expire))
+	}
+
+	sharedAccess, err := f.access.Share(permission, sharedPrefix)
+	if err != nil {
+		return "", fmt.Errorf("sharing access to object failed: %w", err)
+	}
+
+	creds, err := (&edge.Config{
+		AuthServiceAddress: "auth.storjshare.io:7777",
+	}).RegisterAccess(ctx, sharedAccess, &edge.RegisterAccessOptions{Public: true})
+	if err != nil {
+		return "", fmt.Errorf("creating public link failed: %w", err)
+	}
+
+	return edge.JoinShareURL("https://link.storjshare.io", creds.AccessKeyID, bucket, key, nil)
+}
diff --git a/docs/content/crypt.md b/docs/content/crypt.md
index 813aa0830..2b0550246 100644
--- a/docs/content/crypt.md
+++ b/docs/content/crypt.md
@@ -455,6 +455,7 @@ Properties:
     - "off"
         - Don't encrypt the file names.
         - Adds a ".bin" extension only.
+        - May cause problems on [case insensitive](/overview/#case-insensitive) [storage systems](/overview/#features) like OneDrive, Dropbox, Windows, OSX and SMB.
 
 #### --crypt-directory-name-encryption
 
@@ -473,6 +474,7 @@ Properties:
         - Encrypt directory names.
     - "false"
         - Don't encrypt directory names, leave them intact.
+        - May cause problems on [case insensitive](/overview/#case-insensitive) [storage systems](/overview/#features) like OneDrive, Dropbox, Windows, OSX and SMB.
 
 #### --crypt-password
 
diff --git a/docs/content/overview.md b/docs/content/overview.md
index eca35d8d4..df19d05a9 100644
--- a/docs/content/overview.md
+++ b/docs/content/overview.md
@@ -504,7 +504,7 @@ upon backend-specific capabilities.
 | Sia                          | No    | No   | No   | No      | No      | No    | Yes          | No           | No    | Yes      |
 | SMB                          | No    | No   | Yes  | Yes     | No      | No    | Yes          | No           | No    | Yes      |
 | SugarSync                    | Yes   | Yes  | Yes  | Yes     | No      | No    | Yes          | Yes          | No    | Yes      |
-| Storj                        | Yes † | Yes  | Yes  | No      | No      | Yes   | Yes          | No           | No    | No       |
+| Storj                        | Yes † | Yes  | Yes  | No      | No      | Yes   | Yes          | Yes          | No    | No       |
 | Uptobox                      | No    | Yes  | Yes  | Yes     | No      | No    | No           | No           | No    | No       |
 | WebDAV                       | Yes   | Yes  | Yes  | Yes     | No      | No    | Yes ‡        | No           | Yes   | Yes      |
 | Yandex Disk                  | Yes   | Yes  | Yes  | Yes     | Yes     | No    | Yes          | Yes          | Yes   | Yes      |
diff --git a/lib/oauthutil/oauthutil.go b/lib/oauthutil/oauthutil.go
index cd9cb74d3..5f3e62f23 100644
--- a/lib/oauthutil/oauthutil.go
+++ b/lib/oauthutil/oauthutil.go
@@ -204,6 +204,37 @@ func (ts *TokenSource) reReadToken() (changed bool) {
 	return changed
 }
 
+type retrieveErrResponse struct {
+	Error string `json:"error"`
+}
+
+// If err is nil or an error other than fatal OAuth errors, returns err itself.
+// Otherwise returns a more user-friendly error.
+func maybeWrapOAuthError(err error, remoteName string) (newErr error) {
+	newErr = err
+	if rErr, ok := err.(*oauth2.RetrieveError); ok {
+		if rErr.Response.StatusCode == 400 || rErr.Response.StatusCode == 401 {
+			fs.Debugf(remoteName, "got fatal oauth error: %v", rErr)
+			var resp retrieveErrResponse
+			if err = json.Unmarshal(rErr.Body, &resp); err != nil {
+				newErr = fmt.Errorf("(can't decode error info) - try refreshing token with \"rclone config reconnect %s:\"", remoteName)
+				return
+			}
+			var suggestion string
+			switch resp.Error {
+			case "invalid_client", "unauthorized_client", "unsupported_grant_type", "invalid_scope":
+				suggestion = fmt.Sprintf("if you're using your own client id/secret, make sure they're properly set up following the docs")
+			case "invalid_grant":
+				fallthrough
+			default:
+				suggestion = fmt.Sprintf("maybe token expired? - try refreshing with \"rclone config reconnect %s:\"", remoteName)
+			}
+			newErr = fmt.Errorf("%s: %s", resp.Error, suggestion)
+		}
+	}
+	return
+}
+
 // Token returns a token or an error.
 // Token must be safe for concurrent use by multiple goroutines.
 // The returned Token must not be modified.
@@ -242,11 +273,15 @@ func (ts *TokenSource) Token() (*oauth2.Token, error) {
 		if err == nil {
 			break
 		}
+		if newErr := maybeWrapOAuthError(err, ts.name); newErr != err {
+			err = newErr // Fatal OAuth error
+			break
+		}
 		fs.Debugf(ts.name, "Token refresh failed try %d/%d: %v", i, maxTries, err)
 		time.Sleep(1 * time.Second)
 	}
 	if err != nil {
-		return nil, fmt.Errorf("couldn't fetch token - maybe it has expired? - refresh with \"rclone config reconnect %s:\": %w", ts.name, err)
+		return nil, fmt.Errorf("couldn't fetch token: %w", err)
 	}
 	changed = changed || (*token != *ts.token)
 	ts.token = token


Test patch:
diff --git a/fstest/fstests/fstests.go b/fstest/fstests/fstests.go
index 40fb39851..9c3ea61ff 100644
--- a/fstest/fstests/fstests.go
+++ b/fstest/fstests/fstests.go
@@ -1734,8 +1734,10 @@ func Run(t *testing.T, opt *Opt) {
 					// ensure sub remote isn't empty
 					buf := bytes.NewBufferString("somecontent")
 					obji := object.NewStaticObjectInfo("somefile", time.Now(), int64(buf.Len()), true, nil, nil)
-					_, err = subRemote.Put(ctx, buf, obji)
-					require.NoError(t, err)
+					retry(t, "Put", func() error {
+						_, err := subRemote.Put(ctx, buf, obji)
+						return err
+					})
 
 					link4, err := wrapPublicLinkFunc(subRemote.Features().PublicLink)(ctx, "", expiry, false)
 					require.NoError(t, err, "Sharing root in a sub-remote should work")