fix listing dirs with dfs links to forbidden shares #26845

Merged
merged 1 commit into from Feb 10, 2017

Conversation

Projects
None yet
6 participants
@butonic
Member

butonic commented Dec 20, 2016

Description

This fix swallows NotFoundException when fetching a directory listing to skip forbidden shares.

Related Issue

Motivation and Context

A forbidden folder might prevent listing a dir, even when other folders are readable

How Has This Been Tested?

  • create dfs link to share that is forbidden for user fred
  • try browsing into the forbidden folder
  • without this patch the dir listing for the dfs links fails
  • with it it lists the links (without the forbidden one)

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.
@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Dec 20, 2016

@butonic, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jmaciasportela, @jvillafanez, @PVince81 and @Xenopathic to be potential reviewers.

@butonic, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jmaciasportela, @jvillafanez, @PVince81 and @Xenopathic to be potential reviewers.

@butonic

This comment has been minimized.

Show comment
Hide comment
@butonic

butonic Dec 20, 2016

Member

backport to 9.1 requested

Member

butonic commented Dec 20, 2016

backport to 9.1 requested

@felixboehm

This comment has been minimized.

Show comment
Hide comment
@felixboehm

felixboehm Dec 20, 2016

Contributor

yes, backport it

Contributor

felixboehm commented Dec 20, 2016

yes, backport it

@felixboehm

This comment has been minimized.

Show comment
Hide comment
@felixboehm

felixboehm Dec 20, 2016

Contributor

well, this should be covered with tests, right?
@butonic reach out for help with testing, if you won't write them yourself ...

@SergioBertolinSG @davitol

Contributor

felixboehm commented Dec 20, 2016

well, this should be covered with tests, right?
@butonic reach out for help with testing, if you won't write them yourself ...

@SergioBertolinSG @davitol

@JKawohl

This comment has been minimized.

Show comment
Hide comment
@JKawohl

JKawohl Jan 9, 2017

Contributor

@butonic backported yet?

Contributor

JKawohl commented Jan 9, 2017

@butonic backported yet?

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jan 11, 2017

Member

@jvillafanez please review

Member

PVince81 commented Jan 11, 2017

@jvillafanez please review

@PVince81 PVince81 requested a review from jvillafanez Jan 11, 2017

@PVince81 PVince81 added green-ticket and removed blue-ticket labels Jan 11, 2017

@felixboehm

This comment has been minimized.

Show comment
Hide comment
@felixboehm

felixboehm Jan 12, 2017

Contributor

@jvillafanez please review and backport to 9.1
@SergioBertolinSG please test to ensure the fix works with DFS.
Let me know timeline and blockers.

Contributor

felixboehm commented Jan 12, 2017

@jvillafanez please review and backport to 9.1
@SergioBertolinSG please test to ensure the fix works with DFS.
Let me know timeline and blockers.

+ //remember entry so we can answer file_exists and filetype without a full stat
+ $this->statCache[$path . '/' . $fileInfo->getName()] = $fileInfo;
+ }
+ } catch (NotFoundException $e) {

This comment has been minimized.

@jvillafanez

jvillafanez Jan 13, 2017

Member

When is this exception thrown? I don't think it will be thrown in this code block.

@jvillafanez

jvillafanez Jan 13, 2017

Member

When is this exception thrown? I don't think it will be thrown in this code block.

This comment has been minimized.

@PVince81

PVince81 Jan 24, 2017

Member

@butonic see previous comment

@PVince81

PVince81 Jan 24, 2017

Member

@butonic see previous comment

This comment has been minimized.

@PVince81

PVince81 Jan 24, 2017

Member

@jvillafanez if this exception is really never thrown then this fix is useless.
If it was tested on a real env with hidden files and did work, then the exception is indeed thrown somewhere.

The ticket mentions some tests so I assume that it was tested manually.
Tests from @SergioBertolinSG will confirm.

@PVince81

PVince81 Jan 24, 2017

Member

@jvillafanez if this exception is really never thrown then this fix is useless.
If it was tested on a real env with hidden files and did work, then the exception is indeed thrown somewhere.

The ticket mentions some tests so I assume that it was tested manually.
Tests from @SergioBertolinSG will confirm.

This comment has been minimized.

@jvillafanez

jvillafanez Jan 25, 2017

Member

I'm starting to think we should have a specific connector for DFS with all the special cases.

Yes, the exception could be thrown in the isHidden method, and the file might not be found. This could happen, but there might be other scenarios not covered if we consider this special case.

@jvillafanez

jvillafanez Jan 25, 2017

Member

I'm starting to think we should have a specific connector for DFS with all the special cases.

Yes, the exception could be thrown in the isHidden method, and the file might not be found. This could happen, but there might be other scenarios not covered if we consider this special case.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jan 19, 2017

Member

@butonic @SergioBertolinSG can we finish this + backports before RC1 next week ?

Member

PVince81 commented Jan 19, 2017

@butonic @SergioBertolinSG can we finish this + backports before RC1 next week ?

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jan 24, 2017

Member

@butonic @jvillafanez @SergioBertolinSG ping. Running out of time for the backports.

Member

PVince81 commented Jan 24, 2017

@butonic @jvillafanez @SergioBertolinSG ping. Running out of time for the backports.

@jvillafanez

This comment has been minimized.

Show comment
Hide comment
Member

jvillafanez commented Jan 25, 2017

👍

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jan 25, 2017

Member

Rebased for CI

Member

PVince81 commented Jan 25, 2017

Rebased for CI

@PVince81 PVince81 merged commit 9adb460 into master Feb 10, 2017

4 checks passed

Scrutinizer 1 new issues
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

@PVince81 PVince81 deleted the listdfslinkforforbiddenshare branch Feb 10, 2017

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Feb 10, 2017

Member

@butonic can you backport ? or ask someone else to do it if you can't

Member

PVince81 commented Feb 10, 2017

@butonic can you backport ? or ask someone else to do it if you can't

@felixboehm

This comment has been minimized.

Show comment
Hide comment
@felixboehm

felixboehm Feb 13, 2017

Contributor

This was not part of the 9.1.4 release?
@PVince81 @jvillafanez

Contributor

felixboehm commented Feb 13, 2017

This was not part of the 9.1.4 release?
@PVince81 @jvillafanez

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Feb 13, 2017

Member

No. Backport still needs to be done. @jvillafanez @butonic should I take care of this if you don't have time ?

Member

PVince81 commented Feb 13, 2017

No. Backport still needs to be done. @jvillafanez @butonic should I take care of this if you don't have time ?

@butonic

This comment has been minimized.

Show comment
Hide comment
@butonic

butonic Feb 14, 2017

Member

please take over!

Member

butonic commented Feb 14, 2017

please take over!

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Feb 14, 2017

Member

I'll take care of this backport and let @jvillafanez review.
Thanks for letting us know!

Member

PVince81 commented Feb 14, 2017

I'll take care of this backport and let @jvillafanez review.
Thanks for letting us know!

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Feb 14, 2017

Member

stable9.1: #27152

Member

PVince81 commented Feb 14, 2017

stable9.1: #27152

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Feb 14, 2017

Member

I didn't backport further because only 9.1 was requested.

Member

PVince81 commented Feb 14, 2017

I didn't backport further because only 9.1 was requested.

@felixboehm

This comment has been minimized.

Show comment
Hide comment
@felixboehm

felixboehm Mar 22, 2017

Contributor

Thanks

Contributor

felixboehm commented Mar 22, 2017

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment