Skip to content

Handle no user session in isSharingDisabledForUser()#28911

Merged
PVince81 merged 1 commit intomasterfrom
undef-var-user
Sep 18, 2017
Merged

Handle no user session in isSharingDisabledForUser()#28911
PVince81 merged 1 commit intomasterfrom
undef-var-user

Conversation

@phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Sep 5, 2017

Description

If isSharingDisabledForUser() is called when there is no user context, then set user to null.

Related Issue

Motivation and Context

In the log of Travis output, I noticed:

owncloud.log:
{"reqId":"pqAijCPvaiHvZolHHCdO","level":3,"time":"2017-09-05T08:06:49+00:00","remoteAddr":"","user":"--","app":"PHP","method":"--","url":"--","message":"Undefined variable: user at \/home\/travis\/build\/[secure]\/core\/lib\/public\/Util.php#193"}

It is nice to get rid of undefined variable messages.

How Has This Been Tested?

Try logging in and creating files... as a user with this code in place. See that it does not explode.

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)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PVince81
PVince81 previously approved these changes Sep 5, 2017
Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 nice catch, I saw this error many times

@phil-davis
Copy link
Contributor Author

phil-davis commented Sep 5, 2017

Hmmm - I broke unit tests, better have a look.
Update: code changed, all passing now.

@phil-davis
Copy link
Contributor Author

Previously I tried returning isSharingDisabledForUser() as true when there was no user session found. But that causes problems for stuff like public shares, where the incoming request has no user authentication.
The neatest looking solution is just to set $user to null when there is no user session. self::$shareManager->sharingDisabledForUser($user) already handles that fine and returns false and so when there is no user session, sharing is NOT disabled. This lets public shares and the like work.

While messing with ViewControllerTest.php my IDE indented some stuff to make it consistent. No functional change there, but just nice to have improved indenting while I am there.

@PVince81 please re-review.

@phil-davis
Copy link
Contributor Author

Backport stable10 #28915
(prospective)

@phil-davis phil-davis changed the title sharing is disabled when there is no user Handle no user session in isSharingDisabledForUser() Sep 6, 2017
@phil-davis
Copy link
Contributor Author

@PVince81 please merge if happy - and merge backport also.

@phil-davis phil-davis dismissed PVince81’s stale review September 9, 2017 10:57

Logic was reversed since the approval, please review again.

@phil-davis phil-davis requested a review from PVince81 September 18, 2017 05:59
@PVince81 PVince81 merged commit 6243360 into master Sep 18, 2017
@PVince81 PVince81 deleted the undef-var-user branch September 18, 2017 08:29
@lock
Copy link

lock bot commented Aug 2, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 2, 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.

3 participants