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

Stable8.1 backport sharingcheckmiddleware fixes #19638

Merged
merged 4 commits into from
Oct 16, 2015

Conversation

rullzer
Copy link
Contributor

@rullzer rullzer commented Oct 8, 2015

As reqeusted in #19487 (comment)

Basically backports 3 PR's that solve issues we had in the sharing middleware. The request was only for the last PR but without the whole bunch things would get kind of messy. And since all 3 PR's are pretty clean this seemed like the best solution and improved the sharing middleware for 8.1.

In order:

  1. Throw error if link sharing is disabled and a link share is visited: Respect disabled sharing API settings #19262
  2. Show not found page (via not found exception): Only intercept exceptions of type "NotFoundException" instead of any … #19470
  3. Split files sharing middleware to allow federated sharing even when link sharing is disabled: Split files_sharing middelware #19487

Of course the introduced tests are backported as well.

@karlitschek please confirm backport

@MorrisJobke @michaelstingl: please have a look and test.

rullzer and others added 4 commits October 8, 2015 08:09
If the sharing API setting is disabled that sharing check middle ware
should block the request. Thus making link shares unavailable.
Fixes #18970

* Unit test added
* Unit tests updated
…Exception

The sharing backend may throw another exception for example when the activity app encounters a problem. Previously this just triggered a 404 error page and the exception got not logged at all. With this change such exceptions get not intercepted and regularly handled as exceptions so that we have meaningful log data. Also the user will be shown a window informing him that an error happened.

Helps to debug cases such as #19465
Since for external shares there is no need for link shares to be enabled
we should check which controller is actually being called.

This makes sure that in all cases we verify that the files_sharing app
is enabled. But only for the share controller (public shares) we check
if the API is enabled and if links are enabled.

TODO: add checks for federated sharing as well
Added new annotations for the externalsharescontroller class
* @NoOutgoingFederatedSharingRequired
* @NoIncomingFederatedSharingRequired

By default both are required for all functions in the
externalSharesController.

A proper exception is thrown and then a 405 is returned instead of the
default error page. Since it is only an API endpoint this makes more
sense.

Unit tests added and updated
@MorrisJobke
Copy link
Contributor

  • public link shares are not accessible anymore after deactivating sharing API in admin settings
  • federated cloud shares can be received if only the share API (first checkbox) and the two federated cloud sharing settings are active

👍 from my side

@nickvergessen @LukasReschke Can you review this please?

@PVince81
Copy link
Contributor

👍

But what's up with the OCS API integration tests ci ?

@MorrisJobke
Copy link
Contributor

@DeepDiver1975 Do the ocs api integration tests work on stable8.1?

@PVince81
Copy link
Contributor

maybe @SergioBertolinSG knows ?

@PVince81
Copy link
Contributor

Looking at #19784 which contains changes unrelated to OCS also have it fail on 8.1, so the tests for OCS API probably don't work against 8.1.

@rullzer
Copy link
Contributor Author

rullzer commented Oct 15, 2015

@MorrisJobke @PVince81 intergration tests stuff was only added in 8.2 so the files are missing here ;)

@PVince81
Copy link
Contributor

Okay, then this is good to go

@rullzer
Copy link
Contributor Author

rullzer commented Oct 15, 2015

@karlitschek can you approve this backport(s)?

@karlitschek
Copy link
Contributor

yes. please backport

rullzer added a commit that referenced this pull request Oct 16, 2015
…ckmiddleware_fixes

Stable8.1 backport sharingcheckmiddleware fixes
@rullzer rullzer merged commit 64fb704 into stable8.1 Oct 16, 2015
@rullzer rullzer deleted the stable8.1_backport_sharingcheckmiddleware_fixes branch October 16, 2015 06:51
@SergioBertolinSG
Copy link
Contributor

But what's up with the OCS API integration tests ci ?

It is only prepared for master at the moment.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants