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

don't store private public-share-key in session #12410

Merged
merged 4 commits into from Nov 25, 2014

Conversation

Projects
None yet
6 participants
@schiessle
Copy link
Member

schiessle commented Nov 25, 2014

proposed solution for #12405

cc @DeepDiver1975 @LukasReschke @MorrisJobke what do you think?

@@ -29,6 +29,7 @@
class Session {

private $view;
private static $publicShareKey = false;

This comment has been minimized.

Copy link
@DeepDiver1975

DeepDiver1975 Nov 25, 2014

Member

static field but non-static accessors feels strange

This comment has been minimized.

Copy link
@schiessle

schiessle Nov 25, 2014

Author Member

good catch, changed...

@DeepDiver1975

This comment has been minimized.

Copy link
Member

DeepDiver1975 commented Nov 25, 2014

Looks good

@schiessle schiessle force-pushed the no_session_for_public_share_key branch Nov 25, 2014

@DeepDiver1975

View changes

apps/files_encryption/lib/session.php Outdated

\OC::$server->getSession()->set('publicSharePrivateKey', $privateKey);

public static function setPublicSharePrivateKey($privateKey) {

This comment has been minimized.

Copy link
@DeepDiver1975

DeepDiver1975 Nov 25, 2014

Member

these functions are still called non static in some cases in this class - please consider private visibility because these methods are only used in this class afaik - thx

This comment has been minimized.

Copy link
@schiessle

schiessle Nov 25, 2014

Author Member

damn, I shouldn't do to much stuff in parallel. You are right the methods are only used internally, don't know why they were public maybe in the past they were also used from outside... Whatever... Updated.

@owncloud-bot

This comment has been minimized.

Copy link
Contributor

owncloud-bot commented Nov 25, 2014

💣 Test FAILed. 💣
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/3214/

Build result: FAILURE

[...truncated 18 lines...] > git rev-parse origin/pr/12410/merge^{commit} # timeout=10Checking out Revision 911e478cc88e6db3dc72912bb6ee4488dfbbf9e8 (detached) > git config core.sparsecheckout # timeout=10 > git checkout -f 911e478cc88e6db3dc72912bb6ee4488dfbbf9e8 > git rev-list ce4eea39bda3f414b6611daec3d5762f047d1c5c # timeout=10First time build. Skipping changelog. > git remote # timeout=10 > git submodule init # timeout=10 > git submodule sync # timeout=10 > git config --get remote.origin.url # timeout=10 > git submodule update --init --recursiveCleaning workspace > git rev-parse --verify HEAD # timeout=10Resetting working tree > git reset --hard # timeout=10 > git clean -fdx # timeout=10 > git submodule foreach --recursive git reset --hard # timeout=10 > git submodule foreach --recursive git clean -fdx # timeout=10Triggering pull-request-analyser-ng-simple » vm-slave-02pull-request-analyser-ng-simple » vm-slave-02 completed with result FAILUREStarted calculate disk usage of buildFinished Calculation of disk usage of build in 0 secondsStarted calculate disk usage of workspaceFinished Calculation of disk usage of workspace in 15 second
💣 Test FAILed. 💣

@schiessle schiessle force-pushed the no_session_for_public_share_key branch to e5bcc2d Nov 25, 2014

@schiessle

This comment has been minimized.

Copy link
Member Author

schiessle commented Nov 25, 2014

hope I didn't killed Jenkins again with my rebase

@LukasReschke

This comment has been minimized.

Copy link
Member

LukasReschke commented Nov 25, 2014

You did ;-)

@LukasReschke

This comment has been minimized.

Copy link
Member

LukasReschke commented Nov 25, 2014

👍

if (!\OCP\App::isEnabled('files_encryption')) {
// encryption app need to create keys later, so can't close too early
\OC::$server->getSession()->close();
}

This comment has been minimized.

Copy link
@LukasReschke

LukasReschke Nov 25, 2014

Member

Add the session close to the top of the file then. Otherwise it will be closed never.

This comment has been minimized.

Copy link
@LukasReschke

LukasReschke Nov 25, 2014

Member

(This is different in regard to the AppFramework which has the UseSession annotation to keep the session open for you)

This comment has been minimized.

Copy link
@schiessle

schiessle Nov 25, 2014

Author Member

fixed

@schiessle schiessle force-pushed the no_session_for_public_share_key branch from e5bcc2d to 1d33503 Nov 25, 2014

@owncloud-bot

This comment has been minimized.

Copy link
Contributor

owncloud-bot commented Nov 25, 2014

💣 Test FAILed. 💣
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/3224/

Build result: FAILURE

[...truncated 17 lines...] > git fetch --tags --progress https://github.com/owncloud/core.git +refs/pull/:refs/remotes/origin/pr/ > git rev-parse origin/pr/12410/merge^{commit} # timeout=10Checking out Revision c35d1f8ef0bef266322daa2a21fc49ec322e5bb0 (detached) > git config core.sparsecheckout # timeout=10 > git checkout -f c35d1f8ef0bef266322daa2a21fc49ec322e5bb0 > git rev-list 359c80be06176a66465a5246624aa3ffae5efbe4 # timeout=10 > git remote # timeout=10 > git submodule init # timeout=10 > git submodule sync # timeout=10 > git config --get remote.origin.url # timeout=10 > git submodule update --init --recursiveCleaning workspace > git rev-parse --verify HEAD # timeout=10Resetting working tree > git reset --hard # timeout=10 > git clean -fdx # timeout=10 > git submodule foreach --recursive git reset --hard # timeout=10 > git submodule foreach --recursive git clean -fdx # timeout=10Triggering pull-request-analyser-ng-simple » vm-slave-02pull-request-analyser-ng-simple » vm-slave-02 completed with result FAILUREStarted calculate disk usage of buildFinished Calculation of disk usage of build in 0 secondsStarted calculate disk usage of workspaceFinished Calculation of disk usage of workspace in 28 second
💣 Test FAILed. 💣

@owncloud-bot

This comment has been minimized.

Copy link
Contributor

owncloud-bot commented Nov 25, 2014

💣 Test FAILed. 💣
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/3230/

Build result: FAILURE

[...truncated 18 lines...] > git rev-parse origin/pr/12410/merge^{commit} # timeout=10Checking out Revision c0397d6acfa54953f72c621b0fb621ea97a221e3 (detached) > git config core.sparsecheckout # timeout=10 > git checkout -f c0397d6acfa54953f72c621b0fb621ea97a221e3 > git rev-list 4a30ab7a1ac0f06b65c18c0a9a4561d291f1dd26 # timeout=10First time build. Skipping changelog. > git remote # timeout=10 > git submodule init # timeout=10 > git submodule sync # timeout=10 > git config --get remote.origin.url # timeout=10 > git submodule update --init --recursiveCleaning workspace > git rev-parse --verify HEAD # timeout=10Resetting working tree > git reset --hard # timeout=10 > git clean -fdx # timeout=10 > git submodule foreach --recursive git reset --hard # timeout=10 > git submodule foreach --recursive git clean -fdx # timeout=10Triggering pull-request-analyser-ng-simple » vm-slave-02pull-request-analyser-ng-simple » vm-slave-02 completed with result FAILUREStarted calculate disk usage of buildFinished Calculation of disk usage of build in 0 secondsStarted calculate disk usage of workspaceFinished Calculation of disk usage of workspace in 3 second
💣 Test FAILed. 💣

@MorrisJobke

This comment has been minimized.

Copy link
Member

MorrisJobke commented Nov 25, 2014

hope I didn't killed Jenkins again with my rebase

I also tested this and it doesn't kiled jenkins -.- This Jenkins GH plugin is a huge pile of 💩 -.- cc @DeepDiver1975 Another Jenkins killer PR

@scrutinizer-notifier

This comment has been minimized.

Copy link

scrutinizer-notifier commented Nov 25, 2014

A new inspection was created.

@DeepDiver1975

This comment has been minimized.

Copy link
Member

DeepDiver1975 commented Nov 25, 2014

@MorrisJobke looks like it's time to fix the plugin ourselves ....

@owncloud-bot

This comment has been minimized.

Copy link
Contributor

owncloud-bot commented Nov 25, 2014

💣 Test FAILed. 💣
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/3248/

Build result: FAILURE

[...truncated 18 lines...] > git rev-parse origin/pr/12410/merge^{commit} # timeout=10Checking out Revision c672854a86fff2bd33b8bfb5ef566c5269d11373 (detached) > git config core.sparsecheckout # timeout=10 > git checkout -f c672854a86fff2bd33b8bfb5ef566c5269d11373 > git rev-list ef090e4a78f79bd9bc366ebffce602d8b3a99a21 # timeout=10First time build. Skipping changelog. > git remote # timeout=10 > git submodule init # timeout=10 > git submodule sync # timeout=10 > git config --get remote.origin.url # timeout=10 > git submodule update --init --recursiveCleaning workspace > git rev-parse --verify HEAD # timeout=10Resetting working tree > git reset --hard # timeout=10 > git clean -fdx # timeout=10 > git submodule foreach --recursive git reset --hard # timeout=10 > git submodule foreach --recursive git clean -fdx # timeout=10Triggering pull-request-analyser-ng-simple » vm-slave-02pull-request-analyser-ng-simple » vm-slave-02 completed with result FAILUREStarted calculate disk usage of buildFinished Calculation of disk usage of build in 0 secondsStarted calculate disk usage of workspaceFinished Calculation of disk usage of workspace in 3 second
💣 Test FAILed. 💣

@schiessle

This comment has been minimized.

Copy link
Member Author

schiessle commented Nov 25, 2014

I create a new PR just for Jenkins to run the tests... In the meantime who want to have a look for the second +1 ?

@DeepDiver1975

This comment has been minimized.

Copy link
Member

DeepDiver1975 commented Nov 25, 2014

failing unit tests are unreated:

Test Result (4 failures / -3)

    Test\TempManager.testCleanFolder
    Test\TempManager.testCleanOld
    Test\TempManager.testLogCantCreateFile
    Test\TempManager.testLogCantCreateFolder
@DeepDiver1975

This comment has been minimized.

Copy link
Member

DeepDiver1975 commented Nov 25, 2014

👍

DeepDiver1975 added a commit that referenced this pull request Nov 25, 2014

Merge pull request #12410 from owncloud/no_session_for_public_share_key
don't store private public-share-key in session

@DeepDiver1975 DeepDiver1975 merged commit e6a7022 into master Nov 25, 2014

1 check failed

default Merged build finished.
Details

@DeepDiver1975 DeepDiver1975 deleted the no_session_for_public_share_key branch Nov 25, 2014

@MorrisJobke MorrisJobke added this to the 8.0-current milestone Nov 26, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.