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

endorse password for share links #1894

Closed
wants to merge 1 commit into from
Closed

Conversation

MorrisJobke
Copy link
Member

works like "enforce password protection", but let the
user optionally remove the password protection after the
password is set.

cc @rullzer @schiessle @nickvergessen

I tested this and it works

works like "enforce password protection", but let the
user optionally remove the password protection after the
password is set.

endorseLinkPassword changed to enableLinkPasswordByDefault

let the user set an empty password for share link

label changed to better match workflow
@mention-bot
Copy link

@MorrisJobke, thanks for your PR! By analyzing the history of the files in this pull request, we identified @blizzz, @PVince81 and @LukasReschke to be potential reviewers.

@rullzer
Copy link
Member

rullzer commented Oct 24, 2016

mmmm. Do we really want this? It feels like a very specific option.

@MorrisJobke
Copy link
Member Author

mmmm. Do we really want this? It feels like a very specific option.

This is up for discussion now.

@jancborchardt for simplify the possible settings vs @karlitschek for feature completeness

The stage is yours 😉

@jancborchardt
Copy link
Member

Sounds like it can make sense, yes. Only being lax and forced about it is strange, this seems like a needed middle ground to do it properly.

@LukasReschke
Copy link
Member

LukasReschke commented Oct 24, 2016

Rather strange UX and I don't really like it. A proper behaviour would be to actually show a confirmation dialogue or something like that which asks "Do you really not want to set any password?" or something like that…

Otherwise this just seems like an confusing solution that increases code complexity… 👎 from my side.

If we want that we should take care to do it with a better UX than this. Also we should make sure to add this to the API so that clients can behave similar like the web.

@karlitschek
Copy link
Member

It is a very specific usecase but not bad. I think it makes some sense 👍

@LukasReschke
Copy link
Member

LukasReschke commented Oct 25, 2016

👎 until this is fixed on the client side (e.g. desktop client) as well or a big warning is appended that this only works on web.

This gives a wrong level of security plus would qualify for a bounty actually in our bounty program… (Client-Side Enforcement of Server-Side Security)

(e.g. Always ask for a password (web-only) would be fine for me)

@@ -47,6 +47,10 @@
value="1" <?php if ($_['allowPublicUpload'] == 'yes') print_unescaped('checked="checked"'); ?> />
<label for="allowPublicUpload"><?php p($l->t('Allow public uploads'));?></label><br/>

<input type="checkbox" name="shareapi_enable_link_password_by_default" id="enableLinkPasswordByDefault" class="checkbox"
value="1" <?php if ($_['enableLinkPasswordByDefault'] === 'yes') print_unescaped('checked="checked"'); ?> />
<label for="enableLinkPasswordByDefault"><?php p($l->t('Always ask for a password'));?></label><br/>
Copy link
Member

Choose a reason for hiding this comment

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

Needs to be adjusted. Or the clients fixed.

Copy link
Member

@LukasReschke LukasReschke Oct 25, 2016

Choose a reason for hiding this comment

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

(once that is fixed I'm "ok" with this change, not happy but "ok".)

@nickvergessen
Copy link
Member

Test\Settings\Admin\SharingTest::testGetFormWithoutExcludedGroups
Expectation failed for method name is equal to string:getAppValue when invoked at sequence index 8
Parameter 1 for invocation OCP\IConfig::getAppValue('core', 'shareapi_enable_link_password...efault', 'no') does not match expected value.
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'shareapi_enabled'
+'shareapi_enable_link_password_by_default'

@nickvergessen nickvergessen added 1. to develop Accepted and waiting to be taken care of and removed 3. to review Waiting for reviews labels Oct 25, 2016
@MorrisJobke
Copy link
Member Author

Lets close this for now. We have no real use case for this already and it is only a partial solution that will cause more issues, than it solves.

If someone feels the need for this - feel free to reopen this and work again on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1. to develop Accepted and waiting to be taken care of enhancement feature: sharing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants