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.

Sign up for GitHub

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

Jump to bottom

Allow restricting of app password permissions #719

Merged
merged 18 commits into from
Nov 16, 2016
Merged

Allow restricting of app password permissions #719

merged 18 commits into from
Nov 16, 2016

Conversation

icewind1991
Copy link
Member

This adds the option to configure app tokens to limit the permissions it provides.

Currently it only allows to deny filesystem access from a token. Restricting access to apps is possible already in the backend but neesd a ui.

Restricted app passwords reduce the risk that you take by giving a (3rdparty) client, you no longer need to give access to all your files to the cal/carddav sync app you use.

cc @ChristophWurst @LukasReschke

@icewind1991 icewind1991 added the 3. to review Waiting for reviews label Aug 3, 2016
@icewind1991 icewind1991 added this to the Nextcloud 11.0 milestone Aug 3, 2016
@mention-bot
Copy link

@icewind1991, thanks for your PR! By analyzing the annotation information on this pull request, we identified @ChristophWurst, @nickvergessen, @blizzz and @DeepDiver1975 to be potential reviewers

ChristophWurst
ChristophWurst reviewed Aug 3, 2016
View reviewed changes
lib/private/Authentication/Token/DefaultToken.php

public function setScope($scope) {
if (is_string($scope)) {
$this->scope = $scope;
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't you use the parent method instead to mark the property as updated? AFAIK app framework entities remember which properties are dirty and only updates those.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@MorrisJobke
Copy link
Member

@icewind1991 Please add documentation:

Parsing all files in lib/public for the presence of @since or @deprecated on each method...

PHPDoc is needed for OCP\Lockdown\ILockdownManager::ILockdownManager
PHPDoc is needed for OCP\Lockdown\ILockdownManager::enable
PHPDoc is needed for OCP\Lockdown\ILockdownManager::setToken
PHPDoc is needed for OCP\Lockdown\ILockdownManager::canAccessFilesystem
PHPDoc is needed for OCP\Lockdown\ILockdownManager::canAccessApp

nickvergessen
nickvergessen reviewed Aug 5, 2016
View reviewed changes
lib/private/Lockdown/Filesystem/NullCache.php
}

public function put($file, array $data) {
throw new \OC\ForbiddenException('This request is not allowed to access the filesystem');
Copy link
Member

Choose a reason for hiding this comment

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

OCP\Files\ForbiddenException?

@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Aug 17, 2016
@MorrisJobke
Copy link
Member

@icewind1991 Any progress on this?

@icewind1991
Copy link
Member Author

Limited the scope of this PR to only restricting filesystem access for now.

We can look into a better way of handling restricting apps or other things separately, imo fs access is the most important thing to be able to restrict

@icewind1991 icewind1991 added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 19, 2016
@MorrisJobke MorrisJobke mentioned this pull request Oct 24, 2016
Closed
47 tasks
@icewind1991 icewind1991 force-pushed the lockdown branch 3 times, most recently from 9f0c5ba to 73919c8 Compare October 31, 2016 13:39
@icewind1991
Copy link
Member Author

ready for review

cc @MorrisJobke @rullzer @LukasReschke

@MorrisJobke
Copy link
Member

For me the setting is not saved and after a refresh of the page it is still granted.

Clicking the gear icon will look like this:

Before clicking:
bildschirmfoto 2016-11-02 um 21 55 41

After clicking:
bildschirmfoto 2016-11-02 um 21 55 48

@nextcloud/designers Any ideas how to improve this?

MorrisJobke
MorrisJobke previously requested changes Nov 2, 2016
View reviewed changes
settings/js/authtoken_view.js
@@ -353,6 +375,26 @@
});
},

_onSetTokenScope: function (event) {
var $target = $(event.target);
console.log($target);
Copy link
Member

Choose a reason for hiding this comment

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

Debug?

settings/js/authtoken_view.js

var scope = token.get('scope');
scope.filesystem = $target.is(":checked");
console.log(scope);
Copy link
Member

Choose a reason for hiding this comment

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

Debug?

Copy link
Member Author

Choose a reason for hiding this comment

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

removed both

@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Nov 2, 2016
@MorrisJobke MorrisJobke added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 7, 2016
icewind1991 and others added 16 commits November 16, 2016 15:24
@icewind1991
app password scope wip
b4e27d3 
Signed-off-by: Robin Appelman <icewind@owncloud.com>
@icewind1991
allow configuring filesystem access
1afccde 
Signed-off-by: Robin Appelman <icewind@owncloud.com>
@icewind1991
enable deleting/configuring new tokens
da63af8 
Signed-off-by: Robin Appelman <icewind@owncloud.com>
@icewind1991
fix setscope
7e9e5db 
Signed-off-by: Robin Appelman <icewind@owncloud.com>
@icewind1991
phpdoc
c5df58e 
Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991
update tests
bb65d3b 
Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991
cast to int
a4ea20a 
Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991
explicit types
4c3d18a 
Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991
update autoloader
4837904 
Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991
add tests
91851c3 
Signed-off-by: Robin Appelman <robin@icewind.nl>
@rullzer @icewind1991
Adds TokenProvider and Mapper tests
e5bc80b 
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer @icewind1991
Adds NullCache ans NullStorage tests for Lockdown
59d6003 
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer @icewind1991
Adds tests for the AuthSettingsController
311531e 
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@icewind1991
dropdown ui
9157f80 
Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991
Add test for setting up fake fs
e774327 
Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991
add test
e633f2f 
Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991
Copy link
Member Author

@LukasReschke @rullzer please review

@rullzer
Copy link
Member

rullzer commented Nov 16, 2016

LETS DO THIS 👍

@rullzer rullzer merged commit 61453f5 into master Nov 16, 2016
@rullzer rullzer deleted the lockdown branch November 16, 2016 15:17
@jancborchardt
Copy link
Member

Awesome stuff 👍 :)

@skjnldsv
Copy link
Member

Well done!!

@MariusBluem MariusBluem mentioned this pull request Jan 1, 2017
Open
@MorrisJobke MorrisJobke mentioned this pull request Apr 5, 2017
Closed
@e-markus e-markus mentioned this pull request Sep 6, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rullzer rullzer rullzer approved these changes

@MorrisJobke MorrisJobke MorrisJobke left review comments

@LukasReschke LukasReschke LukasReschke left review comments

Assignees
No one assigned
Labels
3. to review Waiting for reviews feature: authentication security
Projects
None yet
Milestone
Nextcloud 11.0
Development

Successfully merging this pull request may close these issues.