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

Personal settings auth tokens #24703

Merged
merged 3 commits into from May 23, 2016

Conversation

Projects
None yet
5 participants
@ChristophWurst
Contributor

ChristophWurst commented May 18, 2016

requires #24686, #24677
fixes #23458

TODO:

  • rebase onto #24686
  • rebase onto #24677
  • list browser sessions and devices
  • add button to invalidate token
  • add button to create a new device token
  • unit tests

Ideas for follow-up PRs:

  • Allow omitting the dashes of device tokens, so users can use both 'ABCDEFGHIJKLMNOPQRST' and 'ABCDE-FGHIJ-KLMNO-PQRST'
  • Don't allow the users to kill the active browser session as that would log them out (probably unintended)
  • Convert sync client user agents like Mozilla/5.0 (Linux) mirall/2.1.1 to something like ownCloud sync client 2.1.1

cc @MorrisJobke

@ChristophWurst ChristophWurst added this to the 9.1-current milestone May 18, 2016

@ChristophWurst ChristophWurst changed the title from Personal settings auth tokens to [WIP] Personal settings auth tokens May 18, 2016

@nickvergessen

View changes

Show outdated Hide outdated lib/private/Server.php
@@ -837,6 +837,13 @@ public function getUserSession() {
}
/**
* @return Authentication\Token\DefaultTokenProvider

This comment has been minimized.

@nickvergessen

nickvergessen May 18, 2016

Contributor

Please either use the full class or import the full class

@nickvergessen

nickvergessen May 18, 2016

Contributor

Please either use the full class or import the full class

This comment has been minimized.

@nickvergessen

nickvergessen May 18, 2016

Contributor

Also only public interfaces shall be returned™

@nickvergessen

nickvergessen May 18, 2016

Contributor

Also only public interfaces shall be returned™

This comment has been minimized.

@nickvergessen

nickvergessen May 18, 2016

Contributor

If there is no public interface yet, because it's not yet done add a FIXME/TODO to make that clear

@nickvergessen

nickvergessen May 18, 2016

Contributor

If there is no public interface yet, because it's not yet done add a FIXME/TODO to make that clear

This comment has been minimized.

@MorrisJobke

MorrisJobke May 18, 2016

Member

But this is an internal only method.

@MorrisJobke

MorrisJobke May 18, 2016

Member

But this is an internal only method.

This comment has been minimized.

@nickvergessen

nickvergessen May 18, 2016

Contributor

Then @internal ? ;)

@nickvergessen

nickvergessen May 18, 2016

Contributor

Then @internal ? ;)

@MorrisJobke

This comment has been minimized.

Show comment
Hide comment
@MorrisJobke

MorrisJobke May 18, 2016

Member

After applying this:

diff --git a/settings/css/settings.css b/settings/css/settings.css
index be61265..42e2594 100644
--- a/settings/css/settings.css
+++ b/settings/css/settings.css
@@ -100,21 +100,27 @@ input#identity {
 table.nostyle label { margin-right: 2em; }
 table.nostyle td { padding: 0.2em 0; }

-#sessions,
-#devices {
-   min-height: 180px;
-}
 #sessions table,
 #devices table {
    width: 100%;
-   min-height: 150px;
-   padding-top: 25px;
 }
 #sessions table th,
 #devices table th {
    font-weight: 800;
 }

+#sessions table th,
+#sessions table td,
+#devices table th,
+#devices table td {
+   padding: 10px;
+}
+
+#sessions .token-list td,
+#devices .token-list td {
+   border-top: 1px solid #DDD;
+}
+
 /* USERS */
 #newgroup-init a span { margin-left: 20px; }
 #newgroup-init a span:before {
diff --git a/settings/js/authtoken_view.js b/settings/js/authtoken_view.js
index 0ca1682..b2bdee8 100644
--- a/settings/js/authtoken_view.js
+++ b/settings/js/authtoken_view.js
@@ -50,7 +50,9 @@
            list.html('');

            tokens.forEach(function(token) {
-               var html = _this.template(token.toJSON());
+               var t = token.toJSON();
+               t.lastActivity = moment(t.lastActivity, 'X').format('LLL');
+               var html = _this.template(t);
                list.append(html);
            });
        },

It looks like this:

bildschirmfoto 2016-05-18 um 17 45 17

Member

MorrisJobke commented May 18, 2016

After applying this:

diff --git a/settings/css/settings.css b/settings/css/settings.css
index be61265..42e2594 100644
--- a/settings/css/settings.css
+++ b/settings/css/settings.css
@@ -100,21 +100,27 @@ input#identity {
 table.nostyle label { margin-right: 2em; }
 table.nostyle td { padding: 0.2em 0; }

-#sessions,
-#devices {
-   min-height: 180px;
-}
 #sessions table,
 #devices table {
    width: 100%;
-   min-height: 150px;
-   padding-top: 25px;
 }
 #sessions table th,
 #devices table th {
    font-weight: 800;
 }

+#sessions table th,
+#sessions table td,
+#devices table th,
+#devices table td {
+   padding: 10px;
+}
+
+#sessions .token-list td,
+#devices .token-list td {
+   border-top: 1px solid #DDD;
+}
+
 /* USERS */
 #newgroup-init a span { margin-left: 20px; }
 #newgroup-init a span:before {
diff --git a/settings/js/authtoken_view.js b/settings/js/authtoken_view.js
index 0ca1682..b2bdee8 100644
--- a/settings/js/authtoken_view.js
+++ b/settings/js/authtoken_view.js
@@ -50,7 +50,9 @@
            list.html('');

            tokens.forEach(function(token) {
-               var html = _this.template(token.toJSON());
+               var t = token.toJSON();
+               t.lastActivity = moment(t.lastActivity, 'X').format('LLL');
+               var html = _this.template(t);
                list.append(html);
            });
        },

It looks like this:

bildschirmfoto 2016-05-18 um 17 45 17

@MorrisJobke

This comment has been minimized.

Show comment
Hide comment
@MorrisJobke

MorrisJobke May 18, 2016

Member

@ChristophWurst @PVince81 What is the best way in Backbone to apply the date format stuff? Should this be a template filter?

Member

MorrisJobke commented May 18, 2016

@ChristophWurst @PVince81 What is the best way in Backbone to apply the date format stuff? Should this be a template filter?

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 18, 2016

Member

So far I'd just preformat the date before passing it to the template as string.
See how it's done in here: https://github.com/owncloud/core/blob/master/apps/files/js/mainfileinfodetailview.js#L148

Member

PVince81 commented May 18, 2016

So far I'd just preformat the date before passing it to the template as string.
See how it's done in here: https://github.com/owncloud/core/blob/master/apps/files/js/mainfileinfodetailview.js#L148

@@ -36,7 +36,8 @@
$application->registerRoutes($this, [
'resources' => [
'groups' => ['url' => '/settings/users/groups'],
'users' => ['url' => '/settings/users/users']
'users' => ['url' => '/settings/users/users'],
'AuthSettings' => ['url' => '/settings/personal/authtokens'],

This comment has been minimized.

@nickvergessen

nickvergessen May 18, 2016

Contributor

if we want this information to be available on the client one day, it needs to be OCS, but I think for now it's okay

@nickvergessen

nickvergessen May 18, 2016

Contributor

if we want this information to be available on the client one day, it needs to be OCS, but I think for now it's okay

@ChristophWurst

This comment has been minimized.

Show comment
Hide comment
@ChristophWurst

ChristophWurst May 19, 2016

Contributor

bildschirmfoto von 2016-05-19 11-19-36
bildschirmfoto von 2016-05-19 11-19-48

(Note that the token is now 4x5chars and not 5x5 like in the screenshot)

Contributor

ChristophWurst commented May 19, 2016

bildschirmfoto von 2016-05-19 11-19-36
bildschirmfoto von 2016-05-19 11-19-48

(Note that the token is now 4x5chars and not 5x5 like in the screenshot)

@ChristophWurst ChristophWurst changed the title from [WIP] Personal settings auth tokens to Personal settings auth tokens May 19, 2016

@ChristophWurst

This comment has been minimized.

Show comment
Hide comment
@ChristophWurst
Contributor

ChristophWurst commented May 19, 2016

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 20, 2016

Member
  • BUG: use $(...).tooltip() on the trashbin element to make the tooltip look better
Member

PVince81 commented May 20, 2016

  • BUG: use $(...).tooltip() on the trashbin element to make the tooltip look better
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 20, 2016

Member
Member

PVince81 commented May 20, 2016

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 20, 2016

Member

I'd even say, as soon as the token field gets displayed, autoselect it. If the user clicks away and clicks on it again, reselect again. Goal is to be able to quickly Ctrl+C.

Member

PVince81 commented May 20, 2016

I'd even say, as soon as the token field gets displayed, autoselect it. If the user clicks away and clicks on it again, reselect again. Goal is to be able to quickly Ctrl+C.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 20, 2016

Member
  • do we want the date in the table to be human readable, like "1 hour ago" + tooltip with full date ? Use OC.Util.relativeModifiedDate(timeStamp) for that
Member

PVince81 commented May 20, 2016

  • do we want the date in the table to be human readable, like "1 hour ago" + tooltip with full date ? Use OC.Util.relativeModifiedDate(timeStamp) for that
Show outdated Hide outdated settings/js/authtoken_view.js
var SubView = Backbone.View.extend({
collection: null,
type: 0,
template: Handlebars.compile(TEMPLATE_TOKEN),

This comment has been minimized.

Show outdated Hide outdated settings/js/authtoken_view.js
var SubView = Backbone.View.extend({
collection: null,
type: 0,

This comment has been minimized.

@PVince81

PVince81 May 20, 2016

Member

what's type ? add JS comment

@PVince81

PVince81 May 20, 2016

Member

what's type ? add JS comment

Show outdated Hide outdated settings/js/authtoken_view.js
this.type = options.type;
this.collection = options.collection;
},
render: function() {

This comment has been minimized.

@PVince81

PVince81 May 20, 2016

Member

a bit of spacing (one line) between the functions would be nice 😄

@PVince81

PVince81 May 20, 2016

Member

a bit of spacing (one line) between the functions would be nice 😄

Show outdated Hide outdated settings/js/authtoken_view.js
var list = this.$el.find('.token-list');
var tokens = this.collection.filter(function(token) {
return parseInt(token.get('type')) === _this.type;

This comment has been minimized.

@PVince81

PVince81 May 20, 2016

Member

use parseInt(str, 10) to be safe (leading zeroes are parsed as octal otherwise!)

@PVince81

PVince81 May 20, 2016

Member

use parseInt(str, 10) to be safe (leading zeroes are parsed as octal otherwise!)

this._toggleAddingToken(true);
var deviceName = this._tokenName.val();
var creatingToken = $.ajax(OC.generateUrl('/settings/personal/authtokens'), {

This comment has been minimized.

@PVince81

PVince81 May 20, 2016

Member

Wouldn't the backbone model.save() or collection.create() achieve that ? If using collection.create() combined with collection.on('add') it would even auto-add the new device in the list

@PVince81

PVince81 May 20, 2016

Member

Wouldn't the backbone model.save() or collection.create() achieve that ? If using collection.create() combined with collection.on('add') it would even auto-add the new device in the list

This comment has been minimized.

@ChristophWurst

ChristophWurst May 20, 2016

Contributor

Nope, the back-end not only returns the new token, but also the plaintext token password. Hence, I'd have to write my own backbone sync for that AFAIK

@ChristophWurst

ChristophWurst May 20, 2016

Contributor

Nope, the back-end not only returns the new token, but also the plaintext token password. Hence, I'd have to write my own backbone sync for that AFAIK

This comment has been minimized.

@PVince81

PVince81 May 20, 2016

Member

I thought collection.create({name: 'test'}) on the devices collection would do, if you set the correct url on the model.

From the backbone docs if you search for create:

POST /books/ .... collection.create();

@PVince81

PVince81 May 20, 2016

Member

I thought collection.create({name: 'test'}) on the devices collection would do, if you set the correct url on the model.

From the backbone docs if you search for create:

POST /books/ .... collection.create();

This comment has been minimized.

@ChristophWurst

ChristophWurst May 20, 2016

Contributor

If only the token was returned, yes. But it's a custom response: https://github.com/owncloud/core/pull/24703/files#diff-86dbc8c463c390f9fcc767a856c332a3R116
How would I get access to the plain text token if I used backbone?

@ChristophWurst

ChristophWurst May 20, 2016

Contributor

If only the token was returned, yes. But it's a custom response: https://github.com/owncloud/core/pull/24703/files#diff-86dbc8c463c390f9fcc767a856c332a3R116
How would I get access to the plain text token if I used backbone?

This comment has been minimized.

@PVince81

PVince81 May 20, 2016

Member

Looks like it's a JSON response: https://github.com/owncloud/core/pull/24703/files#diff-86dbc8c463c390f9fcc767a856c332a3R91

Backbone automatically parses it and sets the attributes on a new model. You should receive the model in the callback of collection.create. See how it's done here:

tag = this.collection.create({

Ideally you could also return the device name in that JSON response too.

@PVince81

PVince81 May 20, 2016

Member

Looks like it's a JSON response: https://github.com/owncloud/core/pull/24703/files#diff-86dbc8c463c390f9fcc767a856c332a3R91

Backbone automatically parses it and sets the attributes on a new model. You should receive the model in the callback of collection.create. See how it's done here:

tag = this.collection.create({

Ideally you could also return the device name in that JSON response too.

This comment has been minimized.

@PVince81

PVince81 May 20, 2016

Member

Ok, I think I get your point. You used the same collection for both types.

So to be properly backbony the endpoint would have to return the type attribute too, the JSON response would need to match what the list elements return too. In this specific case, the token value would then be returned too.

But in general the token isn't part of the model when retrieveing the list, isn't it ?

I'm fine with leaving this as is.

@PVince81

PVince81 May 20, 2016

Member

Ok, I think I get your point. You used the same collection for both types.

So to be properly backbony the endpoint would have to return the type attribute too, the JSON response would need to match what the list elements return too. In this specific case, the token value would then be returned too.

But in general the token isn't part of the model when retrieveing the list, isn't it ?

I'm fine with leaving this as is.

This comment has been minimized.

@ChristophWurst

ChristophWurst May 20, 2016

Contributor

Okay, so the solution would be to add the plain text token to the json data of the created token?
Something like

$data = $deviceToken->jsonSerialize();
$data['plain'] = $token;
return $data;

and then extract that plain text token in the success callback of the create method on the collection?

@ChristophWurst

ChristophWurst May 20, 2016

Contributor

Okay, so the solution would be to add the plain text token to the json data of the created token?
Something like

$data = $deviceToken->jsonSerialize();
$data['plain'] = $token;
return $data;

and then extract that plain text token in the success callback of the create method on the collection?

This comment has been minimized.

@PVince81

PVince81 May 20, 2016

Member

Yes, but I don't understand why you're suggested to write "naked" PHP.
The code you pointed at here https://github.com/owncloud/core/pull/24703/files#diff-86dbc8c463c390f9fcc767a856c332a3R116 already produces JSON through the app framework, doesn't it ?

@PVince81

PVince81 May 20, 2016

Member

Yes, but I don't understand why you're suggested to write "naked" PHP.
The code you pointed at here https://github.com/owncloud/core/pull/24703/files#diff-86dbc8c463c390f9fcc767a856c332a3R116 already produces JSON through the app framework, doesn't it ?

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 20, 2016

Member
  • javascript unit tests would be nice! but will be quite difficult since you added the templates in PHP
Member

PVince81 commented May 20, 2016

  • javascript unit tests would be nice! but will be quite difficult since you added the templates in PHP
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 20, 2016

Member

Great stuff! Looking forward to using this feature soon 😄

Member

PVince81 commented May 20, 2016

Great stuff! Looking forward to using this feature soon 😄

}
#device-new-token {
width: 186px;

This comment has been minimized.

@ChristophWurst

ChristophWurst May 20, 2016

Contributor

if someone knows a better way let me know

@ChristophWurst

ChristophWurst May 20, 2016

Contributor

if someone knows a better way let me know

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 20, 2016

Member
  • minor: missing empty message for the lists when empty. I'd suggest to hide the table header and the message "You've linked these devices" when the list is empty. Challenge is to make the list appear as soon as the first entry gets added
Member

PVince81 commented May 20, 2016

  • minor: missing empty message for the lists when empty. I'd suggest to hide the table header and the message "You've linked these devices" when the list is empty. Challenge is to make the list appear as soon as the first entry gets added
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 20, 2016

Member
  • bug? I can create several devices with the same name
Member

PVince81 commented May 20, 2016

  • bug? I can create several devices with the same name
@ChristophWurst

This comment has been minimized.

Show comment
Hide comment
@ChristophWurst

ChristophWurst May 20, 2016

Contributor

I think i broke something, no browser sessions are listed now fixed

Contributor

ChristophWurst commented May 20, 2016

I think i broke something, no browser sessions are listed now fixed

ChristophWurst added some commits May 18, 2016

@ChristophWurst

This comment has been minimized.

Show comment
Hide comment
@ChristophWurst

ChristophWurst May 23, 2016

Contributor

bug? I can create several devices with the same name

Not a bug in my opinion, but I'll open an enhancement issue.

Contributor

ChristophWurst commented May 23, 2016

bug? I can create several devices with the same name

Not a bug in my opinion, but I'll open an enhancement issue.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 23, 2016

Member

Fair enough 👍

Member

PVince81 commented May 23, 2016

Fair enough 👍

@PVince81

This comment has been minimized.

Show comment
Hide comment
Member

PVince81 commented May 23, 2016

$qb = $this->db->getQueryBuilder();
$qb->delete('authtoken')
->where($qb->expr()->eq('id', $qb->createNamedParameter($id)))
->andWhere($qb->expr()->eq('uid', $qb->createNamedParameter($user->getUID())));

This comment has been minimized.

@nickvergessen

nickvergessen May 23, 2016

Contributor

not required, because id is unique?

@nickvergessen

nickvergessen May 23, 2016

Contributor

not required, because id is unique?

This comment has been minimized.

@nickvergessen

nickvergessen May 23, 2016

Contributor

Or don't you check ownership before

@nickvergessen

nickvergessen May 23, 2016

Contributor

Or don't you check ownership before

This comment has been minimized.

@ChristophWurst
@ChristophWurst
@nickvergessen

This comment has been minimized.

Show comment
Hide comment
@nickvergessen

nickvergessen May 23, 2016

Contributor

Works, and I agree that the same name is "not a bug"

👍

Contributor

nickvergessen commented May 23, 2016

Works, and I agree that the same name is "not a bug"

👍

@PVince81 PVince81 merged commit 57525a0 into master May 23, 2016

19 of 21 checks passed

core-ci-linux-swift-primary-storage/database=mysql,label=SLAVE Build #56553 failed in 1 min 22 sec
Details
server-master-linux-externals-ci/database=sqlite,external=swift-ceph,label=SLAVE Build #10536 found unstable in 10 min
Details
Scrutinizer 4 new issues, 10 updated code elements
Details
cla-bot-core Build #4303 succeeded in 11 sec
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
core-ci-linux-jsunit/database=sqlite,label=SLAVE Build #62339 succeeded in 8 min 0 sec
Details
core-ci-linux/database=mysql,label=SLAVE Build #31014 succeeded in 15 min
Details
core-ci-linux/database=oci,label=SLAVE Build #31014 succeeded in 23 min
Details
core-ci-linux/database=pgsql,label=SLAVE Build #31014 succeeded in 14 min
Details
core-ci-linux/database=sqlite,label=SLAVE Build #31014 succeeded in 8 min 12 sec
Details
ocs-api-integration-tests-ci Build #10996 succeeded in 1 hr 57 min
Details
server-master-linux-externals-ci/database=sqlite,external=smb-silvershell,label=SLAVE Build #10536 succeeded in 3 min 19 sec
Details
server-master-linux-externals-smb-windows-ext-ci/database=sqlite,external=smb-windows,label=master Build #18423 succeeded in 4 min 10 sec
Details
server-master-linux-php7-ci/database=sqlite,label=SLAVE Build #39370 succeeded in 11 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=litmus,mirallBranch=v2.0.2,slave=SMASH Build #14915 succeeded in 6 min 49 sec
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_basicSync@0,mirallBranch=v2.0.2,slave=SMASH Build #14915 succeeded in 14 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_basicSync@1,mirallBranch=v2.0.2,slave=SMASH Build #14915 succeeded in 53 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_shareLink,mirallBranch=v2.0.2,slave=SMASH Build #14915 succeeded in 27 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_sharePermissions,mirallBranch=v2.0.2,slave=SMASH Build #14915 succeeded in 49 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_sharePropagationGroups,mirallBranch=v2.0.2,slave=SMASH Build #14915 succeeded in 17 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_sharePropagationInsideGroups,mirallBranch=v2.0.2,slave=SMASH Build #14915 succeeded in 11 min
Details

@PVince81 PVince81 deleted the personal-settings-auth-tokens branch May 23, 2016

@oparoz

This comment has been minimized.

Show comment
Hide comment
@oparoz

oparoz May 31, 2016

Contributor

Cool feature, thanks guys :)

Contributor

oparoz commented May 31, 2016

Cool feature, thanks guys :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment