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

anonymous upload feature #2780

Closed
wants to merge 2 commits into from
Closed

anonymous upload feature #2780

wants to merge 2 commits into from

Conversation

sojer
Copy link

@sojer sojer commented Apr 7, 2013

I added the anonymous upload feature.
Can someone verify this?

}
$.post(OC.filePath('core', 'ajax', 'share.php'), { action: 'setUpload', itemType: itemType, itemSource: itemSource, allow: allow }, function(result) {
if (!result || result.status !== 'success') {
OC.dialogs.alert(t('core', 'Error'), t('core', 'Error setting public upload rights'));
Copy link
Member

Choose a reason for hiding this comment

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

Swap the arguments so "Error" is the title and "Error setting …" the content.

@karlitschek
Copy link
Contributor

@schiesbn @icewind1991 @MTGap Can you review please?

@MTGap
Copy link
Contributor

MTGap commented Apr 7, 2013

You made it way too complicated. The CRUDS permissions should be used instead of that additional field you added to the table. The max upload sizes are also wrong. I don't have time to look over this further.


// Temporary login as user for anonymous uploading
if(isset($_POST['public_uploading'])) {
$_SESSION['user_id'] = $_POST['uidOwner'];
Copy link
Member

Choose a reason for hiding this comment

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

This is absolutely insecure - 👎

Copy link

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I have also never worked with the CRUDS system and therefore can't help you much :-(

@LukasReschke
Copy link
Member

Absolutely insecure. Don't merge this.

@MTGap
Copy link
Contributor

MTGap commented Apr 8, 2013

As a follow up - @sojer we appreciate you working on this highly requested feature. We would like it if you resolved the security issues as well as using CRUDS permissions instead of modifying the share api.

@sojer
Copy link
Author

sojer commented Apr 8, 2013

Thanks for the checking.
I will do the changes.

@ghost
Copy link

ghost commented Apr 17, 2013

@MTGap: I'm currently working on this patch. I clearly see the POST data security issues however I do hava bit of a hard time to figure out the CRUDS permissions. I guess the 'permissions' fiel in database table 'oc_share' is what you want us to use instead of an additional 'uploadable' field.

How would I do that? Does the permission system implement pubic upload access as it is? Is it similar to unix permissions (e.g. 775)? Any help is appreciated :)

@MTGap
Copy link
Contributor

MTGap commented Apr 17, 2013

@rgeber My suggestion is to not look at this patch at all, because it is pretty much wrong.

Read the top lines of lib/public/share.php to find out about CRUDS.

@ghost
Copy link

ghost commented Apr 18, 2013

@MTGap in a perfect world I wouldn't touch the core but put this all into an app. Do you see a way to make this happen? Can I hook into the files_sharing app somehow, adding a new button without touching the files_sharing app as such? What about the back-end to which this patch adds a checkbox in the sharing dialog? Again it would be great to leave the OC core code untouched and have an app do it remotely. I'm just starting out finding my way around OC so please forgive me if those are silly questions. Thanks for your input so far.

@Niduroki
Copy link
Member

Just referencing #740 here, so it isn't forgotten once this is implemented (@rgeber)

@ghost
Copy link

ghost commented Apr 18, 2013

@MTGap I'm doing a clean rewrite of this thing now, however I still don't know how to use the share api to allow anonymous uploads. If I'd use setPermissions I'd have to define a user I want to share this item with, is that correct? Since I don't want to share it with a user but with the world I'm in trouble here. I think once that nut is cracked I should be able to deliver a nice and clean patch.

@ghost
Copy link

ghost commented Apr 18, 2013

@MTGap Alright I think I got this nut cracked :)
I went over the share.js code and found out how the sharing links are created. I'm now using the same method with a different level of permission. That seems to do the trick, however I couldn't test it yet because the actual upload part isn't there. If thats all there's to it, your are quite right, the patch above would be far too complicated

@lock lock bot locked as resolved and limited conversation to collaborators Aug 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants