Allow updating of password on public share #14868

rullzer · 2015-03-13T11:35:57Z

PR to resolve #10671 and #14826 (and possibly others).

currently we cannot update passwords on public shares. What happens is that a new share is created. This leads to problems with the expiration date being not properly propagated and the share getting a new id. Several things have to happen:

- Update password on exisiting share - OCS function updated to call proper function

LukasReschke · 2015-03-13T11:38:00Z

lib/private/share/share.php

+	public static function setPassword($itemType, $itemSource, $password) {
+		$user = \OC_User::getUser();
+
+		if ($password == '') {


=== as per code guidelines

LukasReschke · 2015-03-13T11:38:38Z

lib/private/share/share.php

+	 * @return boolean
+	 */
+	public static function setPassword($itemType, $itemSource, $password) {
+		$user = \OC_User::getUser();


\OCP\IUserSession::getUser with a check if the user is null and throwing an exception then?

rullzer · 2015-03-13T11:41:32Z

@LukasReschke thanks for the feedback. Most stuff is just the result of copy pasting to get it to work. Cleaning up now.

ghost · 2015-03-13T12:02:09Z

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

ghost · 2015-03-13T13:16:50Z

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

ghost · 2015-03-13T13:59:43Z

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

Build result: FAILURE

[...truncated 14 lines...] > git config remote.origin.url https://github.com/owncloud/core.git # timeout=10 > git config --add remote.origin.fetch +refs/heads/:refs/remotes/origin/ # timeout=10 > git config remote.origin.url https://github.com/owncloud/core.git # timeout=10Fetching upstream changes from https://github.com/owncloud/core.gitusing GIT_SSH to set credentials using .gitcredentials to set credentials > git config --local credential.helper store --file=/tmp/git422834675921777452.credentials # timeout=10 > git -c core.askpass=true fetch --tags --progress https://github.com/owncloud/core.git +refs/pull/:refs/remotes/origin/pr/ > git config --local --remove-section credential # timeout=10 > git rev-parse refs/remotes/origin/pr/14868/merge^{commit} # timeout=10 > git rev-parse refs/remotes/origin/origin/pr/14868/merge^{commit} # timeout=10Checking out Revision 18f1443976774ba3422fa512e86fc069ef7bef05 (refs/remotes/origin/pr/14868/merge) > git config core.sparsecheckout # timeout=10 > git checkout -f 18f1443976774ba3422fa512e86fc069ef7bef05 > git rev-list 0b32d578dd3e8076be59703c767f373bfbbe8eaf # 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 --recursiveTriggering pull-request-analyser-ng-simple » SLAVEpull-request-analyser-ng-simple » SLAVE completed with result FAILURESetting status of 3ed3284 to FAILURE with url https://ci.owncloud.org/job/pull-request-analyser-ng-simple/10441/ and message: Merged build finished.
Test FAILed.

LukasReschke · 2015-03-13T14:00:36Z

lib/private/share/share.php

+	 * @return boolean
+	 */
+	public static function setPassword($itemType, $itemSource, $password) {
+		$user = \OC::$server->getUserSession()->getUser();


For unit test purposes you might want to inject the UserSession as well as the DatabaseConnection in the functions signature.

(obviously not inject it in the public API as that would look ugly - but here it is a valid approach for static code 🙈)

yeah seems like a good intermediate solution... since rewriting share.php is clearly not in the scope of this PR

rullzer · 2015-03-13T14:07:50Z

@owncloud-bot retest this please

ghost · 2015-03-13T14:14:18Z

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

ghost · 2015-03-13T14:37:16Z

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

ghost · 2015-03-13T15:44:38Z

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

Build result: FAILURE

[...truncated 14 lines...] > git config remote.origin.url https://github.com/owncloud/core.git # timeout=10 > git config --add remote.origin.fetch +refs/heads/:refs/remotes/origin/ # timeout=10 > git config remote.origin.url https://github.com/owncloud/core.git # timeout=10Fetching upstream changes from https://github.com/owncloud/core.gitusing GIT_SSH to set credentials using .gitcredentials to set credentials > git config --local credential.helper store --file=/tmp/git7563855787991763589.credentials # timeout=10 > git -c core.askpass=true fetch --tags --progress https://github.com/owncloud/core.git +refs/pull/:refs/remotes/origin/pr/ > git config --local --remove-section credential # timeout=10 > git rev-parse refs/remotes/origin/pr/14868/merge^{commit} # timeout=10 > git rev-parse refs/remotes/origin/origin/pr/14868/merge^{commit} # timeout=10Checking out Revision 838add88badafddb120e24ea4b0bb7d3eac38f63 (refs/remotes/origin/pr/14868/merge) > git config core.sparsecheckout # timeout=10 > git checkout -f 838add88badafddb120e24ea4b0bb7d3eac38f63 > git rev-list 18f1443976774ba3422fa512e86fc069ef7bef05 # 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 --recursiveTriggering pull-request-analyser-ng-simple » SLAVEpull-request-analyser-ng-simple » SLAVE completed with result FAILURESetting status of dfb7542 to FAILURE with url https://ci.owncloud.org/job/pull-request-analyser-ng-simple/10449/ and message: Merged build finished.
Test FAILed.

rullzer · 2015-03-13T15:45:44Z

@owncloud-bot retest this please

scrutinizer-notifier · 2015-03-13T15:58:25Z

The inspection completed: 7 new issues, 7 updated code elements

ghost · 2015-03-13T16:13:06Z

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

rullzer · 2015-03-16T10:33:47Z

Please review @schiesbn @PVince81 @LukasReschke @DeepDiver1975

PVince81 · 2015-03-16T16:22:19Z

lib/public/idbconnection.php

+	 *
+	 * @return \Doctrine\DBAL\Query\QueryBuilder
+	 */
+	public function createQueryBuilder();


Interface change, @icewind1991 @DeepDiver1975 @schiesbn I guess we need this anyway ?

Good call. as on my todo list to ask more people about this. We somehow need it if we want to support the QueryBuilder or we can't use the IDBConnection to obtain it

PVince81 · 2015-03-16T16:24:36Z

Nice one!

Originally I was hoping to have a more REST-style API where one must send the whole share model (all values), see #11674

But I think for now we can live with this until we move everything to the OCS Share API.

rullzer · 2015-03-16T16:26:11Z

@PVince81 I agree. But for now this should be a workable solution. Changing the OCS API is something that probably needs to happen. But that is most likely a slow process.

PVince81 · 2015-03-16T16:26:46Z

Tests

share with link, open link, change password: link still valid
share with link, set expiration, allow upload: 🚫 FAIL it says "only 1 public share allowed"

PVince81 · 2015-03-16T16:28:31Z

Ideally you should also add/update the JS unit tests, see here for an example: https://github.com/owncloud/core/pull/11674/files#diff-760d15f0463b1b3a6ca4b48223badbafR289

rullzer · 2015-03-17T14:38:05Z

@PVince81, the reason the second tests fails is because we try to create a new share. I did remove some code at that point. So it is now broken in a different way.

Just chaning the ajax call to call setPermissions does not work. Since then a shareWith field is requierd in the code. We cannot provide this since it is often not known.

The reason it works in the OCS Api is because there we just fecth all info from a share by the id. The hacks to get this to work properly probably are not pretty.

So I suggest to just move that call to the OCS API. Getting the javascript to act nice is probably way easier than hacking the ajax/share.php stuff (and will lead to much duplicated code). Like we already talked about. The new share modal should use more OCS stuff anyways.

On a related note, we should really start on cleaning up (or starting over) with lib/private/share/share.php 🙊

PVince81 · 2015-03-17T14:49:28Z

How about splitting the OCS Share fix from the ajax ones ? At least getting the OCS Share work has higher priority (especially if we intend to move the ajax call to use that too).

And in this way the risk of regression will hopefully be lower. What do you think ?

rullzer · 2015-03-17T14:50:59Z

Yeah that sounds probably best.
I'll have a look later.
You guys can then have a look at the shiny new model with OCS stuff next week 😉

rullzer · 2015-03-18T08:31:58Z

See #14987 for a PR only for the OCS API. Closing this one for now.

Initial update to update passwords on exisitings shares

1e6af0a

- Update password on exisiting share - OCS function updated to call proper function

rullzer added Type:Bug 2 - Developing feature:sharing labels Mar 13, 2015

rullzer added this to the 8.1-current milestone Mar 13, 2015

rullzer mentioned this pull request Mar 13, 2015

OC Sharing API removes expiration date when updating a share with a password #14826

Closed

LukasReschke reviewed Mar 13, 2015
View reviewed changes

rullzer mentioned this pull request Mar 13, 2015

Updating a password using OCS Share API changes the share ID #10671

Closed

LukasReschke reviewed Mar 13, 2015
View reviewed changes

rullzer added 3 commits March 13, 2015 13:12

Comments from Lukas

0e58403

Unused code

03be039

Updated JS, updated ajax/share.php

89e9486

rullzer added 2 commits March 13, 2015 14:38

Cleanup of share.php

96456ff

IDBConnection interface should expose createQueryBuilder

3ed3284

LukasReschke reviewed Mar 13, 2015
View reviewed changes

Inject IUserSession and IDBConnection for better testability

58e9c95

Added unit tests

dfb7542

rullzer added 3 - To Review and removed 2 - Developing labels Mar 14, 2015

PVince81 changed the title ~~[WIP] Allow updating of password on public share~~ Allow updating of password on public share Mar 16, 2015

PVince81 reviewed Mar 16, 2015
View reviewed changes

rullzer added 2 - Developing and removed 3 - To Review labels Mar 17, 2015

rullzer mentioned this pull request Mar 18, 2015

OCS Fixes to allow setting of password without removing additional settings #14987

Merged

rullzer closed this Mar 18, 2015

rullzer removed this from the 8.1-current milestone Mar 18, 2015

lock bot locked as resolved and limited conversation to collaborators Aug 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow updating of password on public share #14868

Allow updating of password on public share #14868

rullzer commented Mar 13, 2015

LukasReschke Mar 13, 2015

LukasReschke Mar 13, 2015

rullzer commented Mar 13, 2015

ghost commented Mar 13, 2015

ghost commented Mar 13, 2015

ghost commented Mar 13, 2015

LukasReschke Mar 13, 2015

rullzer Mar 13, 2015

rullzer commented Mar 13, 2015

ghost commented Mar 13, 2015

ghost commented Mar 13, 2015

ghost commented Mar 13, 2015

rullzer commented Mar 13, 2015

scrutinizer-notifier commented Mar 13, 2015

ghost commented Mar 13, 2015

rullzer commented Mar 16, 2015

PVince81 Mar 16, 2015

rullzer Mar 16, 2015

PVince81 commented Mar 16, 2015

rullzer commented Mar 16, 2015

PVince81 commented Mar 16, 2015

PVince81 commented Mar 16, 2015

rullzer commented Mar 17, 2015

PVince81 commented Mar 17, 2015

rullzer commented Mar 17, 2015

rullzer commented Mar 18, 2015

Allow updating of password on public share #14868

Allow updating of password on public share #14868

Conversation

rullzer commented Mar 13, 2015

LukasReschke Mar 13, 2015

Choose a reason for hiding this comment

LukasReschke Mar 13, 2015

Choose a reason for hiding this comment

rullzer commented Mar 13, 2015

ghost commented Mar 13, 2015

ghost commented Mar 13, 2015

ghost commented Mar 13, 2015

Build result: FAILURE

LukasReschke Mar 13, 2015

Choose a reason for hiding this comment

rullzer Mar 13, 2015

Choose a reason for hiding this comment

rullzer commented Mar 13, 2015

ghost commented Mar 13, 2015

ghost commented Mar 13, 2015

ghost commented Mar 13, 2015

Build result: FAILURE

rullzer commented Mar 13, 2015

scrutinizer-notifier commented Mar 13, 2015

ghost commented Mar 13, 2015

rullzer commented Mar 16, 2015

PVince81 Mar 16, 2015

Choose a reason for hiding this comment

rullzer Mar 16, 2015

Choose a reason for hiding this comment

PVince81 commented Mar 16, 2015

rullzer commented Mar 16, 2015

PVince81 commented Mar 16, 2015

Tests

PVince81 commented Mar 16, 2015

rullzer commented Mar 17, 2015

PVince81 commented Mar 17, 2015

rullzer commented Mar 17, 2015

rullzer commented Mar 18, 2015