From dfc260cea1c7a24cbefb95aa0b87c8d7bf6f9347 Mon Sep 17 00:00:00 2001 From: Drew Fisher Date: Mon, 18 Apr 2016 17:56:29 -0700 Subject: [PATCH 1/3] Remove lastUsed from ApiTokenOwner in supervisor.capnp This is the sort of change that you are ABSOLUTELY NEVER allowed to make, unless @kentonv tells you exactly how and why it's okay and signs off on the mess. It turns out this is safe and won't break anything because: * lastUsed is the last currently-assigned field in the data section for `ApiTokenOwner` (all the subsequent values live in the pointer section) * lastUsed is not actually used by any software in the wild (indeed, its addition caused a regression instead) * decrementing the annotation number of all fields with annotation greater than that of `lastUsed` does not change the current wire format, but prevents a future field addition from being inserted before `denormalizedGrainMetadata`, `introducerIdentity`, and `identityId`, which would redefine existing serialized structs, which would be bad. --- src/sandstorm/supervisor.capnp | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/sandstorm/supervisor.capnp b/src/sandstorm/supervisor.capnp index 63ffa5bcba..0cab98f1d7 100644 --- a/src/sandstorm/supervisor.capnp +++ b/src/sandstorm/supervisor.capnp @@ -272,7 +272,7 @@ struct ApiTokenOwner { saveLabel @2 :Util.LocalizedText; # As passed to `save()` in Sandstorm's Persistent interface. - introducerIdentity @10 :Text; + introducerIdentity @9 :Text; # The identity ID through which a user's powerbox action caused the grain to receive this # token. This is the identity against which the `requiredPermissions` parameter # to `restore()` will be checked. This field is only intended to be filled in by the @@ -298,7 +298,7 @@ struct ApiTokenOwner { # Owned by a user's identity. If the token represents a UiView, then it will show up in this # user's grain list. - identityId @11 :Text; + identityId @10 :Text; # The identity that is allowed to restore this token. title @7 :Text; @@ -307,11 +307,7 @@ struct ApiTokenOwner { # Fields below this line are not actually allowed to be passed to save(), but are added # internally. - lastUsed @8 :Int64; - # The last time the user used this API token with the associated grain, in milliseconds - # since the epoch (equivalent to javascript's new Date().getTime()) - - denormalizedGrainMetadata @9 :DenormalizedGrainMetadata; + denormalizedGrainMetadata @8 :DenormalizedGrainMetadata; # Information needed to show the user an app title and icon in the grain list. userId @6 :Text; From f66fed096c2e98f193a731ae7177318517f974ad Mon Sep 17 00:00:00 2001 From: Drew Fisher Date: Mon, 18 Apr 2016 19:06:16 -0700 Subject: [PATCH 2/3] Move ApiTokenOwner.user.lastUsed onto ApiTokens proper It seems likely we'll be interested in tracking when *any* kind of ApiToken gets used, so we can show more useful information in the connections UI. Additionally, it doesn't really make sense to be passing around lastUsed in calls to save() or other things that take an ApiTokenOwner. So move the field out of token.owner.user and onto token itself. --- .../sandstorm-backend/sandstorm-backend.js | 2 +- shell/packages/sandstorm-db/db.js | 1 + shell/packages/sandstorm-db/migrations.js | 14 ++++++++++++++ .../packages/sandstorm-permissions/permissions.js | 1 - .../sandstorm-ui-grainlist/grainlist-client.js | 7 ++++--- shell/packages/sandstorm-ui-grainview/grainview.js | 2 +- .../sandstorm-ui-powerbox/powerbox-client.js | 2 +- shell/server/hack-session.js | 1 - shell/server/proxy.js | 2 +- shell/server/stats-server.js | 2 +- 10 files changed, 24 insertions(+), 10 deletions(-) diff --git a/shell/packages/sandstorm-backend/sandstorm-backend.js b/shell/packages/sandstorm-backend/sandstorm-backend.js index 1dbb2e00f4..26123aa213 100644 --- a/shell/packages/sandstorm-backend/sandstorm-backend.js +++ b/shell/packages/sandstorm-backend/sandstorm-backend.js @@ -212,7 +212,7 @@ SandstormBackend.prototype.updateLastActive = function (grainId, userId, identit Meteor.users.update({ _id: identityId }, { $set: { lastActive: now } }); // Update any API tokens that match this user/grain pairing as well ApiTokens.update({ grainId: grainId, "owner.user.identityId": identityId }, - { $set: { "owner.user.lastUsed": now } }); + { $set: { lastUsed: now } }); } if (Meteor.settings.public.quotaEnabled) { diff --git a/shell/packages/sandstorm-db/db.js b/shell/packages/sandstorm-db/db.js index dc9a778b31..5ba9794b6f 100644 --- a/shell/packages/sandstorm-db/db.js +++ b/shell/packages/sandstorm-db/db.js @@ -390,6 +390,7 @@ ApiTokens = new Mongo.Collection("apiTokens"); // revoked: If true, then this sturdyref has been revoked and can no longer be restored. It may // become un-revoked in the future. // expires: Optional expiration Date. If undefined, the token does not expire. +// lastUsed: Optional Date when this token was last used. // owner: A `ApiTokenOwner` (defined in `supervisor.capnp`, stored as a JSON object) // as passed to the `save()` call that created this token. If not present, treat // as `webkey` (the default for `ApiTokenOwner`). diff --git a/shell/packages/sandstorm-db/migrations.js b/shell/packages/sandstorm-db/migrations.js index 0e56b24185..6de66e2ba5 100644 --- a/shell/packages/sandstorm-db/migrations.js +++ b/shell/packages/sandstorm-db/migrations.js @@ -543,6 +543,19 @@ function unsetSmtpDefaultHostnameIfNoUsersExist() { } } +function extractLastUsedFromApiTokenOwner() { + // We used to store lastUsed as a field on owner.user. It makes more sense to store lastUsed on + // the apiToken as a whole. This migration hoists such values from owner.user onto the apiToken + // itself. + ApiTokens.find({ "owner.user": { $exists: true } }).forEach(function (token) { + const lastUsed = token.owner.user.lastUsed; + ApiTokens.update({ _id: token._id }, { + $set: { lastUsed: lastUsed }, + $unset: { "owner.user.lastUsed": true }, + }); + }); +} + // This must come after all the functions named within are defined. // Only append to this list! Do not modify or remove list entries; // doing so is likely change the meaning and semantics of user databases. @@ -571,6 +584,7 @@ const MIGRATIONS = [ smtpPortShouldBeNumber, consolidateOrgSettings, unsetSmtpDefaultHostnameIfNoUsersExist, + extractLastUsedFromApiTokenOwner, ]; function migrateToLatest() { diff --git a/shell/packages/sandstorm-permissions/permissions.js b/shell/packages/sandstorm-permissions/permissions.js index 617a9dac34..c8edc97a48 100644 --- a/shell/packages/sandstorm-permissions/permissions.js +++ b/shell/packages/sandstorm-permissions/permissions.js @@ -517,7 +517,6 @@ SandstormPermissions.createNewApiToken = function (db, provider, grainId, petnam user: { identityId: owner.user.identityId, title: owner.user.title, - // lastUsed: ?? denormalizedGrainMetadata: grainInfo, } }; diff --git a/shell/packages/sandstorm-ui-grainlist/grainlist-client.js b/shell/packages/sandstorm-ui-grainlist/grainlist-client.js index 8639baee82..996fc136dc 100644 --- a/shell/packages/sandstorm-ui-grainlist/grainlist-client.js +++ b/shell/packages/sandstorm-ui-grainlist/grainlist-client.js @@ -35,8 +35,9 @@ SandstormGrainListPage.mapApiTokensToTemplateObject = function (apiTokens, stati const grainIdsForApiTokens = Object.keys(tokensForGrain); return grainIdsForApiTokens.map(function (grainId) { // Pick the most recently used one. - const token = _.sortBy(tokensForGrain[grainId], function (t) { - if (t.owner && t.owner.user && t.owner.user.lastUsed) { return -t.owner.user.lastUsed; } else {return 0; } })[0]; + const token = _.sortBy(tokensForGrain[grainId], (t) => { + return t.lastUsed ? -t.lastUsed : 0; + })[0]; const ownerData = token.owner.user; const grainInfo = ownerData.denormalizedGrainMetadata; @@ -49,7 +50,7 @@ SandstormGrainListPage.mapApiTokensToTemplateObject = function (apiTokens, stati _id: grainId, title: ownerData.title, appTitle: appTitle, - lastUsed: ownerData.lastUsed, + lastUsed: token.lastUsed, iconSrc: iconSrc, isOwnedByMe: false, }; diff --git a/shell/packages/sandstorm-ui-grainview/grainview.js b/shell/packages/sandstorm-ui-grainview/grainview.js index 08aca807bc..ce734398a3 100644 --- a/shell/packages/sandstorm-ui-grainview/grainview.js +++ b/shell/packages/sandstorm-ui-grainview/grainview.js @@ -430,7 +430,7 @@ GrainView = class GrainView { grainId: this._grainId, "owner.user.identityId": { $in: myIdentityIds }, }, { - sort:{ "owner.user.lastUsed": -1 }, + sort: { lastUsed: -1 }, }); if (token) { diff --git a/shell/packages/sandstorm-ui-powerbox/powerbox-client.js b/shell/packages/sandstorm-ui-powerbox/powerbox-client.js index b247a4aafa..ad95e055a5 100644 --- a/shell/packages/sandstorm-ui-powerbox/powerbox-client.js +++ b/shell/packages/sandstorm-ui-powerbox/powerbox-client.js @@ -237,7 +237,7 @@ SandstormPowerboxRequest = class SandstormPowerboxRequest { (window.location.protocol + "//" + staticAssetHost + "/" + grainInfo.icon.assetId) : Identicon.identiconForApp( (grainInfo && grainInfo.appId) || "00000000000000000000000000000000"); - cardData.lastUsed = ownerData.lastUsed; + cardData.lastUsed = apiToken.lastUsed; cardData.apiTokenId = apiToken._id; cardData.callback = () => () => { diff --git a/shell/server/hack-session.js b/shell/server/hack-session.js index d0edcfd5e3..52e87e1482 100644 --- a/shell/server/hack-session.js +++ b/shell/server/hack-session.js @@ -62,7 +62,6 @@ SessionContextImpl = class SessionContextImpl { identityId: this.identityId, // The following fields will be overwritten by PersistentUiView.save(), so no need to // pass them in: - //lastUsed: new Date(), //title: "", // This will be replaced by the token's title //denormalizedGrainMetadata: {}, // This will look up the package for the grain referenced. }, diff --git a/shell/server/proxy.js b/shell/server/proxy.js index 62fc2ea8f7..2d46ece8e2 100644 --- a/shell/server/proxy.js +++ b/shell/server/proxy.js @@ -358,7 +358,7 @@ Meteor.methods({ "owner.user.identityId": apiToken.identityId, }, { sort: { - "owner.user.lastUsed": -1, + lastUsed: -1, }, }); if (sharerToken) { diff --git a/shell/server/stats-server.js b/shell/server/stats-server.js index 08bab2eb3c..38878428ba 100644 --- a/shell/server/stats-server.js +++ b/shell/server/stats-server.js @@ -93,7 +93,7 @@ computeStats = function (since) { const counts = ApiTokens.aggregate([ { $match: { - "owner.user.lastUsed": timeConstraint, + lastUsed: timeConstraint, grainId: { $in: grainIds }, }, }, From afc9e166cd02898b3ce6ae901a859dc15893c71b Mon Sep 17 00:00:00 2001 From: Drew Fisher Date: Wed, 20 Apr 2016 16:20:06 -0700 Subject: [PATCH 3/3] stats: make sure to only count tokens with owner.user --- shell/server/stats-server.js | 1 + 1 file changed, 1 insertion(+) diff --git a/shell/server/stats-server.js b/shell/server/stats-server.js index 38878428ba..3526e848d2 100644 --- a/shell/server/stats-server.js +++ b/shell/server/stats-server.js @@ -93,6 +93,7 @@ computeStats = function (since) { const counts = ApiTokens.aggregate([ { $match: { + "owner.user": { $exists: true }, lastUsed: timeConstraint, grainId: { $in: grainIds }, },