fixed the FileInfo.BreadcrumbFolderURL in a collaboration api #10720

2403905 · 2024-12-03T22:08:24Z

Description

Fixed the FileInfo.BreadcrumbFolderURL in a collaboration api"

Related Issue

Fixes #7004

Motivation and Context

How Has This Been Tested?

test environment:
test case 1:
test case 2:
...

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Technical debt
Tests only (no source changes)

Checklist:

Code changes
Unit tests added
Acceptance tests added
Documentation ticket raised:

jvillafanez

I'm pretty sure the coverage.out file shouldn't be there... make sure it's deleted from the repo because it's quite big for something we don't want to keep.

jvillafanez · 2024-12-04T08:54:13Z

services/collaboration/pkg/connector/fileconnector.go

+	parentFolderURL := &url.URL{}
+	*parentFolderURL = *ocisURL
+	parentFolderURL.Path = path.Join(ocisURL.Path, "f", storagespace.FormatResourceID(statRes.GetInfo().GetParentId()))
+	if publicShare := wopiContext.GetPublicShare(); publicShare != nil && isPublicShare {


Is the isPublicShare boolean obsolete now? I mean, you already have the publicShare variable.
Maybe we can use the publicShare variable, which has more info, instead of the isPublicShare

jvillafanez · 2024-12-04T08:59:53Z

services/collaboration/pkg/middleware/wopicontext.go

@@ -36,6 +38,7 @@ type WopiContext struct {
 	FileReference     *providerv1beta1.Reference
 	TemplateReference *providerv1beta1.Reference
 	ViewMode          appproviderv1beta1.ViewMode
+	publicShare       *link.PublicShare


Note that this information will be sent around (encrypted) through the network in the URL. Please, double-check the size of the access token to ensure it doesn't increase by a lot.
We should try to minimize the amount of information we need to move around.

As an alternative, you could parse / decrypt / dismantle the WopiContext.AccessToken on demand. This might be better for consistency because there is only one source of truth (the access token), so it's impossible to have inconsistencies if the access token says one thing and the public share information says a different thing. It's true that there will be a performance penalty because we need to decrypt the access token twice, but hopefully it's fine.

Why publicShare will be added if it is a private field?

I thought it would be added, but it seems it the json marshaller ignores the private fields, so it should be fine

jvillafanez · 2024-12-04T09:20:53Z

services/collaboration/pkg/middleware/wopicontext.go

+		for k, v := range scope {
+			if strings.HasPrefix(k, "publicshare:") && v.Resource.Decoder == "json" {
+				share := &link.PublicShare{}
+				err := utils.UnmarshalJSONToProtoV1(v.Resource.Value, share)
+				if err == nil {
+					claims.WopiContext.publicShare = share
+					break
+				}
+			}
+		}


This might need some changes...
Do we need some debug log to know what public shares are we interested in, or how many scopes are detected? In any case, if there is some error unmarshaling the JSON, not only it will be skipped (which might be fine), but we don't know what happen (which is bad). The error needs to be logged at the very least.

added the error logging

Do we need some debug log to know what public shares are we interested in, or how many scopes are detected?

I don't think so. we use the same approach in a publicstorage provider as well https://github.com/cs3org/reva/blob/bddf9f01f7decc0c839a3da26bacbac5a8a05156/internal/grpc/services/publicstorageprovider/publicstorageprovider.go#L525

It feels weird that the last sentence is an unconditional error log. It seems the goal is to log the error and not filling the publicShare property.
What about

if strings.HasPrefix(k, "publicshare:") && ..... { err := utils.UnmarshalJSONToProtoV1(......) if err != nil { wopiLogger.Error().Err(err)...... } else { claims.WopiContext.publicShare = share } break }

Note that the usual pattern is if err != nil { return / break....}, so changing the pattern makes the code slower to read for a person.

jvillafanez · 2024-12-04T09:25:37Z

services/collaboration/pkg/middleware/wopicontext.go

@@ -230,6 +245,13 @@ func GenerateWopiToken(wopiContext WopiContext, cfg *config.Config, st microstor
 	return accessToken, claims.ExpiresAt.UnixMilli(), err
 }

+func (w *WopiContext) GetPublicShare() *link.PublicShare {


This is questionable as it is at the moment. Why the publicShare property isn't public like the rest? That would make this method obsolete. At the moment, this method doesn't add anything of value other than getting the value, so making the property public to keep the consistency with the rest of the data seems fine to me.

So the property needs to be private in order not to include it in the jwt token, and we need a public method to access to the private property.

A couple of additional things:

If the method is attached to the WopiContext, it's better to have the code as close as possible to the WopiContext definition, so the code should be move upwards. Reading the code should be easier this way.

This method just returns the private property, but it might not be set or evaluated yet (a bit weird scenario, but it could happen if we prepare a WopiContext outside of the http middleware). In this regard, it might be better to either include a public EvaluatePublicShare method that should be called before getting the public share (most of the code is already in the middleware, so it's moving things around), or extract the public share information on demand with the GetPublicShare method and let the private property act as a cache.
I'd go with the EvaluatePublicShare in case the access token changes (which shouldn't happen under normal conditions) because it gives us extra flexibility. As long as it's documented that the EvaluatePublicShare must be called before the GetPublicShare, it should be fine.

I don't think that EvaluatePublicShare method will be good because it has some dependencies.
We could change the private field publicShare to scope and change the GetPublicShare to generic GetScope(keyPrefix string, m proto.Message) method

The scope is only filled in the middleware, and I don't think it's a good idea. If you check the tests, we create a custom WopiContext, but even if we supply correct data to the public WopiContext properties, the scope won't return anything valuable. This is why a suggested to include the EvaluatePublicShare method: to fill and adjust the private properties.
Changing the WopiContext, in particular the access token inside, could also have negative consequences because the scope information isn't valid any longer and we can't update the information in any way. This means that the only valid context is the one supplied by the middleware, and changing or adjusting data in the WopiContext is discouraged.

We can try to do it another way, use the context like for the user.

ctx = ctxpkg.ContextSetScopes(ctx, scope)

ocis/services/collaboration/pkg/middleware/wopicontext.go

Lines 123 to 134 in 2237534

user, _, err := tokenManager.DismantleToken(ctx, wopiContextAccessToken)

if err != nil {

wopiLogger.Error().Err(err).Msg("failed to dismantle reva token manager")

http.Error(w, http.StatusText(http.StatusUnauthorized), http.StatusUnauthorized)

return

}

claims.WopiContext.AccessToken = wopiContextAccessToken

ctx = context.WithValue(ctx, wopiContextKey, claims.WopiContext)

// authentication for the CS3 api

ctx = metadata.AppendToOutgoingContext(ctx, ctxpkg.TokenHeader, claims.WopiContext.AccessToken)

ctx = ctxpkg.ContextSetUser(ctx, user)

In this case we can use existing functions ContextSetScopes, ContextGetScopes and add some scope extractor function

jvillafanez

remember to remove the coverage.out file!

jvillafanez · 2024-12-05T08:14:02Z

services/collaboration/pkg/middleware/wopicontext.go

+	scope             map[string]*auth.Scope
+}
+
+// GetScope returns the scope from the WopiContext


We might need some additional information to say that you need to provide a "message" and the result will be written there.

In general, parameters are treated as read-only, so nobody expects a parameter to change inside the function. The cases where this happens are limited. If the idea is that you just need to pass a placeholder where the result will be written, I think it's better to document it to avoid confusion.

sonarqubecloud · 2024-12-09T14:01:20Z

Quality Gate passed

Issues
0 New issues
1 Accepted issue

Measures
0 Security Hotspots
62.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

fixed the FileInfo.BreadcrumbFolderURL in a collaboration api

jvillafanez · 2024-12-10T08:55:22Z

We could move the GetScopeByKeyPrefix function as a private method of the fileConnector because it's only used there and I don't think there are plans to use the method elsewhere. Anyway, I don't think the change is worth a PR for now.
The rest of the code looks good to me.

2403905 force-pushed the issue-e7004 branch 2 times, most recently from 8e3aead to c25ae0b Compare December 4, 2024 08:42

jvillafanez reviewed Dec 4, 2024

View reviewed changes

2403905 force-pushed the issue-e7004 branch 3 times, most recently from c07323d to eee7a4e Compare December 5, 2024 06:46

2403905 requested a review from jvillafanez December 5, 2024 07:08

jvillafanez reviewed Dec 5, 2024

View reviewed changes

2403905 force-pushed the issue-e7004 branch 3 times, most recently from 39e42ce to 9d62fbc Compare December 6, 2024 11:16

2403905 requested review from jvillafanez and micbar December 6, 2024 11:49

2403905 added 3 commits December 9, 2024 14:45

fixed the FileInfo.BreadcrumbFolderURL in a collaboration api

8d83ac4

use generic wopiContext.scope decoder

ebfcf5a

use the context as a scopes carrier

1d08e9f

2403905 force-pushed the issue-e7004 branch from 9d62fbc to 1d08e9f Compare December 9, 2024 13:45

kobergj approved these changes Dec 9, 2024

View reviewed changes

2403905 merged commit 6455f54 into owncloud:master Dec 9, 2024
4 checks passed

ownclouders pushed a commit that referenced this pull request Dec 9, 2024

Merge pull request #10720 from 2403905/issue-e7004

0784f3e

fixed the FileInfo.BreadcrumbFolderURL in a collaboration api

2403905 mentioned this pull request Dec 10, 2024

Made GetScopeByKeyPrefix function as a private method of the FileConnector #10757

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixed the FileInfo.BreadcrumbFolderURL in a collaboration api #10720

fixed the FileInfo.BreadcrumbFolderURL in a collaboration api #10720

2403905 commented Dec 3, 2024

jvillafanez left a comment

jvillafanez Dec 4, 2024

jvillafanez Dec 4, 2024

2403905 Dec 4, 2024

jvillafanez Dec 4, 2024

jvillafanez Dec 4, 2024

2403905 Dec 4, 2024

2403905 Dec 4, 2024 •

edited

Loading

jvillafanez Dec 4, 2024

jvillafanez Dec 4, 2024

jvillafanez Dec 4, 2024

2403905 Dec 4, 2024

2403905 Dec 4, 2024

jvillafanez Dec 5, 2024

2403905 Dec 5, 2024 •

edited

Loading

2403905 Dec 6, 2024

jvillafanez left a comment

jvillafanez Dec 5, 2024

sonarqubecloud bot commented Dec 9, 2024 •

edited

Loading

jvillafanez commented Dec 10, 2024

	user, _, err := tokenManager.DismantleToken(ctx, wopiContextAccessToken)
	if err != nil {
	wopiLogger.Error().Err(err).Msg("failed to dismantle reva token manager")
	http.Error(w, http.StatusText(http.StatusUnauthorized), http.StatusUnauthorized)
	return
	}
	claims.WopiContext.AccessToken = wopiContextAccessToken

	ctx = context.WithValue(ctx, wopiContextKey, claims.WopiContext)
	// authentication for the CS3 api
	ctx = metadata.AppendToOutgoingContext(ctx, ctxpkg.TokenHeader, claims.WopiContext.AccessToken)
	ctx = ctxpkg.ContextSetUser(ctx, user)

fixed the FileInfo.BreadcrumbFolderURL in a collaboration api #10720

fixed the FileInfo.BreadcrumbFolderURL in a collaboration api #10720

Conversation

2403905 commented Dec 3, 2024

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

jvillafanez 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

Choose a reason for hiding this comment

2403905 Dec 4, 2024 • edited Loading

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

2403905 Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvillafanez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Dec 9, 2024 • edited Loading

Quality Gate passed

jvillafanez commented Dec 10, 2024

2403905 Dec 4, 2024 •

edited

Loading

2403905 Dec 5, 2024 •

edited

Loading

sonarqubecloud bot commented Dec 9, 2024 •

edited

Loading