From 52c5cf33f87990a5828a34f68ff9df833cdc4a15 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Tue, 5 Nov 2019 14:10:23 +0000 Subject: [PATCH] FEATURE: Overhaul of admin API key system (#8284) - Allow revoking keys without deleting them - Auto-revoke keys after a period of no use (default 6 months) - Allow multiple keys per user - Allow attaching a description to each key, for easier auditing - Log changes to keys in the staff action log - Move all key management to one place, and improve the UI --- .../javascripts/admin/adapters/api-key.js.es6 | 11 ++ .../controllers/admin-api-keys-index.js.es6 | 13 ++ .../controllers/admin-api-keys-new.js.es6 | 39 ++++ .../controllers/admin-api-keys-show.js.es6 | 54 ++++++ .../admin/controllers/admin-api-keys.js.es6 | 42 ----- .../admin/controllers/admin-user-index.js.es6 | 30 --- .../admin/models/admin-user.js.es6 | 11 -- .../javascripts/admin/models/api-key.js.es6 | 74 ++++---- .../admin/routes/admin-api-keys-index.js.es6 | 7 + .../admin/routes/admin-api-keys-new.es6 | 7 + .../admin/routes/admin-api-keys-show.js.es6 | 5 + .../admin/routes/admin-api-keys.js.es6 | 11 +- .../admin/routes/admin-route-map.js.es6 | 9 +- .../{api-keys.hbs => api-keys-index.hbs} | 52 +++--- .../admin/templates/api-keys-new.hbs | 27 +++ .../admin/templates/api-keys-show.hbs | 80 ++++++++ .../admin/templates/user-index.hbs | 35 +--- app/assets/stylesheets/common/admin/api.scss | 59 +++++- app/controllers/admin/api_controller.rb | 88 ++++++++- app/controllers/admin/users_controller.rb | 13 -- .../scheduled/clean_up_unused_api_keys.rb | 14 ++ app/models/api_key.rb | 43 +++-- app/models/user.rb | 15 +- app/models/user_history.rb | 10 +- .../admin_detailed_user_serializer.rb | 7 +- app/serializers/api_key_serializer.rb | 5 +- app/services/staff_action_logger.rb | 33 ++++ app/services/user_anonymizer.rb | 2 +- config/locales/client.en.yml | 27 ++- config/locales/server.en.yml | 7 + config/routes.rb | 11 +- config/site_settings.yml | 5 + ...0191101113230_add_revoked_at_to_api_key.rb | 17 ++ lib/auth/default_current_user_provider.rb | 2 +- .../default_current_user_provider_spec.rb | 18 ++ spec/models/api_key_spec.rb | 57 +++++- spec/models/user_spec.rb | 50 ----- spec/requests/admin/api_controller_spec.rb | 176 ++++++++++++++---- spec/requests/admin/users_controller_spec.rb | 40 ++-- spec/requests/embed_controller_spec.rb | 2 +- spec/requests/posts_controller_spec.rb | 10 +- spec/requests/topics_controller_spec.rb | 2 +- spec/requests/user_badges_controller_spec.rb | 4 +- spec/services/user_anonymizer_spec.rb | 2 +- .../admin/models/admin-user-test.js.es6 | 30 --- .../helpers/create-pretender.js.es6 | 2 - 46 files changed, 863 insertions(+), 395 deletions(-) create mode 100644 app/assets/javascripts/admin/adapters/api-key.js.es6 create mode 100644 app/assets/javascripts/admin/controllers/admin-api-keys-index.js.es6 create mode 100644 app/assets/javascripts/admin/controllers/admin-api-keys-new.js.es6 create mode 100644 app/assets/javascripts/admin/controllers/admin-api-keys-show.js.es6 create mode 100644 app/assets/javascripts/admin/routes/admin-api-keys-index.js.es6 create mode 100644 app/assets/javascripts/admin/routes/admin-api-keys-new.es6 create mode 100644 app/assets/javascripts/admin/routes/admin-api-keys-show.js.es6 rename app/assets/javascripts/admin/templates/{api-keys.hbs => api-keys-index.hbs} (55%) create mode 100644 app/assets/javascripts/admin/templates/api-keys-new.hbs create mode 100644 app/assets/javascripts/admin/templates/api-keys-show.hbs create mode 100644 app/jobs/scheduled/clean_up_unused_api_keys.rb create mode 100644 db/migrate/20191101113230_add_revoked_at_to_api_key.rb delete mode 100644 test/javascripts/admin/models/admin-user-test.js.es6 diff --git a/app/assets/javascripts/admin/adapters/api-key.js.es6 b/app/assets/javascripts/admin/adapters/api-key.js.es6 new file mode 100644 index 00000000000..a473f66e085 --- /dev/null +++ b/app/assets/javascripts/admin/adapters/api-key.js.es6 @@ -0,0 +1,11 @@ +import RESTAdapter from "discourse/adapters/rest"; + +export default RESTAdapter.extend({ + basePath() { + return "/admin/api/"; + }, + + apiNameFor() { + return "key"; + } +}); diff --git a/app/assets/javascripts/admin/controllers/admin-api-keys-index.js.es6 b/app/assets/javascripts/admin/controllers/admin-api-keys-index.js.es6 new file mode 100644 index 00000000000..b0872696263 --- /dev/null +++ b/app/assets/javascripts/admin/controllers/admin-api-keys-index.js.es6 @@ -0,0 +1,13 @@ +import { popupAjaxError } from "discourse/lib/ajax-error"; + +export default Ember.Controller.extend({ + actions: { + revokeKey(key) { + key.revoke().catch(popupAjaxError); + }, + + undoRevokeKey(key) { + key.undoRevoke().catch(popupAjaxError); + } + } +}); diff --git a/app/assets/javascripts/admin/controllers/admin-api-keys-new.js.es6 b/app/assets/javascripts/admin/controllers/admin-api-keys-new.js.es6 new file mode 100644 index 00000000000..f4d56c0a058 --- /dev/null +++ b/app/assets/javascripts/admin/controllers/admin-api-keys-new.js.es6 @@ -0,0 +1,39 @@ +import { default as computed } from "ember-addons/ember-computed-decorators"; +import { popupAjaxError } from "discourse/lib/ajax-error"; + +export default Ember.Controller.extend({ + userModes: [ + { id: "all", name: I18n.t("admin.api.all_users") }, + { id: "single", name: I18n.t("admin.api.single_user") } + ], + + @computed("userMode") + showUserSelector(mode) { + return mode === "single"; + }, + + @computed("model.description", "model.username", "userMode") + saveDisabled(description, username, userMode) { + if (Ember.isBlank(description)) return true; + if (userMode === "single" && Ember.isBlank(username)) return true; + return false; + }, + + actions: { + changeUserMode(value) { + if (value === "all") { + this.model.set("username", null); + } + this.set("userMode", value); + }, + + save() { + this.model + .save() + .then(() => { + this.transitionToRoute("adminApiKeys.show", this.model.id); + }) + .catch(popupAjaxError); + } + } +}); diff --git a/app/assets/javascripts/admin/controllers/admin-api-keys-show.js.es6 b/app/assets/javascripts/admin/controllers/admin-api-keys-show.js.es6 new file mode 100644 index 00000000000..2e19ea57799 --- /dev/null +++ b/app/assets/javascripts/admin/controllers/admin-api-keys-show.js.es6 @@ -0,0 +1,54 @@ +import { bufferedProperty } from "discourse/mixins/buffered-content"; +import { popupAjaxError } from "discourse/lib/ajax-error"; +import { empty } from "@ember/object/computed"; + +export default Ember.Controller.extend(bufferedProperty("model"), { + isNew: empty("model.id"), + + actions: { + saveDescription() { + const buffered = this.buffered; + const attrs = buffered.getProperties("description"); + + this.model + .save(attrs) + .then(() => { + this.set("editingDescription", false); + this.rollbackBuffer(); + }) + .catch(popupAjaxError); + }, + + cancel() { + const id = this.get("userField.id"); + if (Ember.isEmpty(id)) { + this.destroyAction(this.userField); + } else { + this.rollbackBuffer(); + this.set("editing", false); + } + }, + + editDescription() { + this.toggleProperty("editingDescription"); + if (!this.editingDescription) { + this.rollbackBuffer(); + } + }, + + revokeKey(key) { + key.revoke().catch(popupAjaxError); + }, + + deleteKey(key) { + key + .destroyRecord() + .then(() => this.transitionToRoute("adminApiKeys.index")) + .catch(popupAjaxError); + }, + + undoRevokeKey(key) { + key.undoRevoke().catch(popupAjaxError); + } + } +}); diff --git a/app/assets/javascripts/admin/controllers/admin-api-keys.js.es6 b/app/assets/javascripts/admin/controllers/admin-api-keys.js.es6 index 9cd1682ff63..e69de29bb2d 100644 --- a/app/assets/javascripts/admin/controllers/admin-api-keys.js.es6 +++ b/app/assets/javascripts/admin/controllers/admin-api-keys.js.es6 @@ -1,42 +0,0 @@ -import ApiKey from "admin/models/api-key"; -import { default as computed } from "ember-addons/ember-computed-decorators"; -import Controller from "@ember/controller"; - -export default Controller.extend({ - @computed("model.[]") - hasMasterKey(model) { - return !!model.findBy("user", null); - }, - - actions: { - generateMasterKey() { - ApiKey.generateMasterKey().then(key => this.model.pushObject(key)); - }, - - regenerateKey(key) { - bootbox.confirm( - I18n.t("admin.api.confirm_regen"), - I18n.t("no_value"), - I18n.t("yes_value"), - result => { - if (result) { - key.regenerate(); - } - } - ); - }, - - revokeKey(key) { - bootbox.confirm( - I18n.t("admin.api.confirm_revoke"), - I18n.t("no_value"), - I18n.t("yes_value"), - result => { - if (result) { - key.revoke().then(() => this.model.removeObject(key)); - } - } - ); - } - } -}); diff --git a/app/assets/javascripts/admin/controllers/admin-user-index.js.es6 b/app/assets/javascripts/admin/controllers/admin-user-index.js.es6 index 9a25f679ef4..47a54b8ebfb 100644 --- a/app/assets/javascripts/admin/controllers/admin-user-index.js.es6 +++ b/app/assets/javascripts/admin/controllers/admin-user-index.js.es6 @@ -258,10 +258,6 @@ export default Controller.extend(CanCheckEmails, { .finally(() => this.toggleProperty("editingTitle")); }, - generateApiKey() { - this.model.generateApiKey(); - }, - saveCustomGroups() { const currentIds = this.customGroupIds; const bufferedIds = this.customGroupIdsBuffer; @@ -294,32 +290,6 @@ export default Controller.extend(CanCheckEmails, { resetPrimaryGroup() { this.set("model.primary_group_id", this.originalPrimaryGroupId); - }, - - regenerateApiKey() { - bootbox.confirm( - I18n.t("admin.api.confirm_regen"), - I18n.t("no_value"), - I18n.t("yes_value"), - result => { - if (result) { - this.model.generateApiKey(); - } - } - ); - }, - - revokeApiKey() { - bootbox.confirm( - I18n.t("admin.api.confirm_revoke"), - I18n.t("no_value"), - I18n.t("yes_value"), - result => { - if (result) { - this.model.revokeApiKey(); - } - } - ); } } }); diff --git a/app/assets/javascripts/admin/models/admin-user.js.es6 b/app/assets/javascripts/admin/models/admin-user.js.es6 index 891a2923c6a..e99c84e0b42 100644 --- a/app/assets/javascripts/admin/models/admin-user.js.es6 +++ b/app/assets/javascripts/admin/models/admin-user.js.es6 @@ -4,7 +4,6 @@ import { ajax } from "discourse/lib/ajax"; import computed from "ember-addons/ember-computed-decorators"; import { propertyNotEqual } from "discourse/lib/computed"; import { popupAjaxError } from "discourse/lib/ajax-error"; -import ApiKey from "admin/models/api-key"; import Group from "discourse/models/group"; import { userPath } from "discourse/lib/url"; @@ -57,16 +56,6 @@ const AdminUser = Discourse.User.extend({ ); }, - generateApiKey() { - return ajax(`/admin/users/${this.id}/generate_api_key`, { - type: "POST" - }).then(result => { - const apiKey = ApiKey.create(result.api_key); - this.set("api_key", apiKey); - return apiKey; - }); - }, - groupAdded(added) { return ajax(`/admin/users/${this.id}/groups`, { type: "POST", diff --git a/app/assets/javascripts/admin/models/api-key.js.es6 b/app/assets/javascripts/admin/models/api-key.js.es6 index 62a4e003bfc..95d8e1914cf 100644 --- a/app/assets/javascripts/admin/models/api-key.js.es6 +++ b/app/assets/javascripts/admin/models/api-key.js.es6 @@ -1,47 +1,55 @@ import AdminUser from "admin/models/admin-user"; +import RestModel from "discourse/models/rest"; import { ajax } from "discourse/lib/ajax"; +import computed from "ember-addons/ember-computed-decorators"; -const KEY_ENDPOINT = "/admin/api/key"; -const KEYS_ENDPOINT = "/admin/api/keys"; +const ApiKey = RestModel.extend({ + user: Ember.computed("_user", { + get() { + return this._user; + }, + set(key, value) { + if (value && !(value instanceof AdminUser)) { + this.set("_user", AdminUser.create(value)); + } else { + this.set("_user", value); + } + return this._user; + } + }), -const ApiKey = Discourse.Model.extend({ - regenerate() { - return ajax(KEY_ENDPOINT, { - type: "PUT", - data: { id: this.id } - }).then(result => { - this.set("key", result.api_key.key); - return this; - }); + @computed("key") + shortKey(key) { + return `${key.substring(0, 4)}...`; + }, + + @computed("description") + shortDescription(description) { + if (!description || description.length < 40) return description; + return `${description.substring(0, 40)}...`; }, revoke() { - return ajax(KEY_ENDPOINT, { - type: "DELETE", - data: { id: this.id } - }); - } -}); - -ApiKey.reopenClass({ - create() { - const result = this._super.apply(this, arguments); - if (result.user) { - result.user = AdminUser.create(result.user); - } - return result; + return ajax(`${this.basePath}/revoke`, { + type: "POST" + }).then(result => this.setProperties(result.api_key)); }, - find() { - return ajax(KEYS_ENDPOINT).then(keys => - keys.map(key => ApiKey.create(key)) - ); + undoRevoke() { + return ajax(`${this.basePath}/undo-revoke`, { + type: "POST" + }).then(result => this.setProperties(result.api_key)); }, - generateMasterKey() { - return ajax(KEY_ENDPOINT, { type: "POST" }).then(result => - ApiKey.create(result.api_key) - ); + createProperties() { + return this.getProperties("description", "username"); + }, + + @computed() + basePath() { + return this.store + .adapterFor("api-key") + .pathFor(this.store, "api-key", this.id); } }); diff --git a/app/assets/javascripts/admin/routes/admin-api-keys-index.js.es6 b/app/assets/javascripts/admin/routes/admin-api-keys-index.js.es6 new file mode 100644 index 00000000000..25f6df3e4af --- /dev/null +++ b/app/assets/javascripts/admin/routes/admin-api-keys-index.js.es6 @@ -0,0 +1,7 @@ +import Route from "@ember/routing/route"; + +export default Route.extend({ + model() { + return this.store.findAll("api-key"); + } +}); diff --git a/app/assets/javascripts/admin/routes/admin-api-keys-new.es6 b/app/assets/javascripts/admin/routes/admin-api-keys-new.es6 new file mode 100644 index 00000000000..3969615c9fe --- /dev/null +++ b/app/assets/javascripts/admin/routes/admin-api-keys-new.es6 @@ -0,0 +1,7 @@ +import Route from "@ember/routing/route"; + +export default Route.extend({ + model() { + return this.store.createRecord("api-key"); + } +}); diff --git a/app/assets/javascripts/admin/routes/admin-api-keys-show.js.es6 b/app/assets/javascripts/admin/routes/admin-api-keys-show.js.es6 new file mode 100644 index 00000000000..b21f2cd021e --- /dev/null +++ b/app/assets/javascripts/admin/routes/admin-api-keys-show.js.es6 @@ -0,0 +1,5 @@ +export default Ember.Route.extend({ + model(params) { + return this.store.find("api-key", params.api_key_id); + } +}); diff --git a/app/assets/javascripts/admin/routes/admin-api-keys.js.es6 b/app/assets/javascripts/admin/routes/admin-api-keys.js.es6 index 18262775674..1b3f3067fcc 100644 --- a/app/assets/javascripts/admin/routes/admin-api-keys.js.es6 +++ b/app/assets/javascripts/admin/routes/admin-api-keys.js.es6 @@ -1,8 +1,13 @@ import Route from "@ember/routing/route"; -import ApiKey from "admin/models/api-key"; export default Route.extend({ - model() { - return ApiKey.find(); + actions: { + show(apiKey) { + this.transitionTo("adminApiKeys.show", apiKey.id); + }, + + new() { + this.transitionTo("adminApiKeys.new"); + } } }); diff --git a/app/assets/javascripts/admin/routes/admin-route-map.js.es6 b/app/assets/javascripts/admin/routes/admin-route-map.js.es6 index 478a851d864..f90b66f97ac 100644 --- a/app/assets/javascripts/admin/routes/admin-route-map.js.es6 +++ b/app/assets/javascripts/admin/routes/admin-route-map.js.es6 @@ -101,7 +101,14 @@ export default function() { ); this.route("adminApi", { path: "/api", resetNamespace: true }, function() { - this.route("adminApiKeys", { path: "/keys", resetNamespace: true }); + this.route( + "adminApiKeys", + { path: "/keys", resetNamespace: true }, + function() { + this.route("show", { path: "/:api_key_id" }); + this.route("new", { path: "/new" }); + } + ); this.route( "adminWebHooks", diff --git a/app/assets/javascripts/admin/templates/api-keys.hbs b/app/assets/javascripts/admin/templates/api-keys-index.hbs similarity index 55% rename from app/assets/javascripts/admin/templates/api-keys.hbs rename to app/assets/javascripts/admin/templates/api-keys-index.hbs index a1254f3042b..19999029eaf 100644 --- a/app/assets/javascripts/admin/templates/api-keys.hbs +++ b/app/assets/javascripts/admin/templates/api-keys-index.hbs @@ -1,7 +1,14 @@ +{{d-button + class="btn-primary" + action=(route-action "new") + icon="plus" + label="admin.api.new_key"}} + {{#if model}} + @@ -9,8 +16,14 @@ {{#each model as |k|}} - - + + + {{/each}} @@ -52,11 +69,4 @@
{{i18n "admin.api.key"}}{{i18n "admin.api.description"}} {{i18n "admin.api.user"}} {{i18n "admin.api.created"}} {{i18n "admin.api.last_used"}}
{{k.key}}
+ {{#if k.revoked_at}}{{d-icon 'times-circle'}}{{/if}} + {{k.shortKey}} + + {{k.shortDescription}} +
{{i18n 'admin.api.user'}}
{{#if k.user}} @@ -34,17 +47,21 @@ {{/if}} 		- {{d-button - class="btn-default" - action=(action "regenerateKey") - actionParam=k icon="undo" - label="admin.api.regenerate"}} - {{d-button - class="btn-default" - action=(action "revokeKey") - actionParam=k - icon="times" - label="admin.api.revoke"}} + {{d-button action=(route-action "show" k) icon="far-eye" title="admin.api.show_details"}} + {{#if k.revoked_at}} + {{d-button + action=(action "undoRevokeKey") + actionParam=k icon="undo" + title="admin.api.undo-revoke"}} + {{else}} + {{d-button + class="btn-danger" + action=(action "revokeKey") + actionParam=k + icon="times" + title="admin.api.revoke"}} + {{/if}} +
{{else}}

{{i18n "admin.api.none"}}

-{{/if}} - -{{#unless hasMasterKey}} - {{d-button - class="btn-primary" - action=(action "generateMasterKey") - icon="key"}} -{{/unless}} +{{/if}} \ No newline at end of file diff --git a/app/assets/javascripts/admin/templates/api-keys-new.hbs b/app/assets/javascripts/admin/templates/api-keys-new.hbs new file mode 100644 index 00000000000..15f25b1ce17 --- /dev/null +++ b/app/assets/javascripts/admin/templates/api-keys-new.hbs @@ -0,0 +1,27 @@ +{{#link-to 'adminApiKeys.index' class="go-back"}} + {{d-icon 'arrow-left'}} + {{i18n 'admin.api.all_api_keys'}} +{{/link-to}} + +
+ {{#admin-form-row label="admin.api.description"}} + {{input value=model.description maxlength="255" placeholder=(i18n "admin.api.description_placeholder")}} + {{/admin-form-row}} + + {{#admin-form-row label="admin.api.user_mode"}} + {{combo-box content=userModes value=userMode onSelect=(action "changeUserMode")}} + {{/admin-form-row}} + + {{#if showUserSelector}} + {{#admin-form-row label="admin.api.user"}} + {{user-selector single="true" + usernames=model.username + placeholderKey="admin.api.user_placeholder" + }} + {{/admin-form-row}} + {{/if}} + + {{#admin-form-row}} + {{d-button icon="check" label="admin.api.save" action=(action "save") class="btn-primary" disabled=saveDisabled}} + {{/admin-form-row}} +
\ No newline at end of file diff --git a/app/assets/javascripts/admin/templates/api-keys-show.hbs b/app/assets/javascripts/admin/templates/api-keys-show.hbs new file mode 100644 index 00000000000..a742cf994a1 --- /dev/null +++ b/app/assets/javascripts/admin/templates/api-keys-show.hbs @@ -0,0 +1,80 @@ +{{#link-to 'adminApiKeys.index' class="go-back"}} + {{d-icon 'arrow-left'}} + {{i18n 'admin.api.all_api_keys'}} +{{/link-to}} + +
+ {{#admin-form-row label="admin.api.key"}} + {{#if model.revoked_at}}{{d-icon 'times-circle'}}{{/if}} + {{model.key}} + {{/admin-form-row}} + + {{#admin-form-row label="admin.api.description"}} + {{#if editingDescription}} + {{input value=buffered.description maxlength="255" placeholder=(i18n "admin.api.description_placeholder")}} + {{else}} + {{if model.description model.description (i18n "admin.api.no_description")}} + {{/if}} + +
+ {{#if editingDescription}} + {{d-button class="ok" action=(action "saveDescription") icon="check"}} + {{d-button class="cancel" action=(action "editDescription") icon="times"}} + {{else}} + {{d-button class="btn-default" action=(action "editDescription") icon="pencil-alt"}} + {{/if}} +
+ {{/admin-form-row}} + + {{#admin-form-row label="admin.api.user"}} + {{#if model.user}} + {{#link-to "adminUser" model.user}} + {{avatar model.user imageSize="small"}} {{model.user.username}} + {{/link-to}} + {{else}} + {{i18n "admin.api.all_users"}} + {{/if}} + {{/admin-form-row}} + + {{#admin-form-row label="admin.api.created"}} + {{format-date model.created_at leaveAgo="true"}} + {{/admin-form-row}} + + {{#admin-form-row label="admin.api.updated"}} + {{format-date model.updated_at leaveAgo="true"}} + {{/admin-form-row}} + + {{#admin-form-row label="admin.api.last_used"}} + {{#if k.last_used_at}} + {{format-date k.last_used_at leaveAgo="true"}} + {{else}} + {{i18n "admin.api.never_used"}} + {{/if}} + {{/admin-form-row}} + + {{#admin-form-row label="admin.api.revoked"}} + {{#if model.revoked_at}} + {{format-date model.revoked_at leaveAgo="true"}} + {{/if}} +
+ {{#if model.revoked_at}} + {{d-button + action=(action "undoRevokeKey") + actionParam=model icon="undo" + label="admin.api.undo_revoke"}} + {{d-button + action=(action "deleteKey") + actionParam=model icon="trash" + label="admin.api.delete" + class="btn-danger"}} + {{else}} + {{d-button + class="btn-danger" + action=(action "revokeKey") + actionParam=model + icon="times" + label="admin.api.revoke"}} + {{/if}} +
+ {{/admin-form-row}} +
\ No newline at end of file diff --git a/app/assets/javascripts/admin/templates/user-index.hbs b/app/assets/javascripts/admin/templates/user-index.hbs index 6166eb1b9af..7eb9504fb5c 100644 --- a/app/assets/javascripts/admin/templates/user-index.hbs +++ b/app/assets/javascripts/admin/templates/user-index.hbs @@ -292,33 +292,14 @@ {{#if currentUser.admin}}
-
{{i18n "admin.api.key"}}
- {{#if model.api_key}} -
- {{model.api_key.key}} - {{d-button - class="btn-default" - action=(action "regenerateApiKey") - icon="undo" - label="admin.api.regenerate"}} - {{d-button - class="btn-default" - action=(action "revokeApiKey") - icon="times" - label="admin.api.revoke"}} -
- {{else}} -
- — -
-
- {{d-button - class="btn-default" - action=(action "generateApiKey") - icon="key" - label="admin.api.generate"}} -
- {{/if}} +
{{i18n "admin.api.active_keys"}}
+
+ {{model.api_key_count}} +
+
+ {{d-button href="/admin/api/keys" label="admin.api.manage_keys"}} + +
{{/if}} diff --git a/app/assets/stylesheets/common/admin/api.scss b/app/assets/stylesheets/common/admin/api.scss index e2a921d5856..ece2277a4f9 100644 --- a/app/assets/stylesheets/common/admin/api.scss +++ b/app/assets/stylesheets/common/admin/api.scss @@ -73,18 +73,28 @@ table.web-hooks.grid { table.api-keys { margin-bottom: 0.25em; - td.key { - font-size: $font-down-1; + + .key-controls { + text-align: right; } + + tr.revoked { + color: $primary-medium; + } + @include breakpoint(tablet) { tr { grid-template-columns: 0.25fr 1fr 1fr; } td.key { - font-size: $font-0; - word-wrap: break-word; grid-row: 1; grid-column-start: 1; + grid-column-end: 1; + max-width: 100%; + } + td.key-description { + grid-row: 1; + grid-column-start: 2; grid-column-end: -1; max-width: 100%; } @@ -138,6 +148,47 @@ table.api-keys { } } +.admin-api-keys { + h2 { + margin-bottom: 10px; + } + .api-key { + padding: 10px; + margin-bottom: 10px; + border-bottom: 1px solid $primary-low; + .form-element, + .form-element-desc { + float: left; + padding: 0.5em 0; + &.input-area { + width: 75%; + .value-list, + .select-kit, + input[type="text"] { + width: 50%; + margin: 0; + } + .ac-wrap { + width: 50% !important; + } + } + &.label-area { + width: 25%; + label { + margin: 0.5em 1em 0 0; + text-align: right; + font-weight: bold; + } + } + } + .controls { + float: right; + text-align: left; + width: 50%; + } + } +} + // Webhook .web-hook-container { .tip.good:empty { diff --git a/app/controllers/admin/api_controller.rb b/app/controllers/admin/api_controller.rb index 1af4a40d0b2..42ec4035eb7 100644 --- a/app/controllers/admin/api_controller.rb +++ b/app/controllers/admin/api_controller.rb @@ -1,16 +1,67 @@ # frozen_string_literal: true class Admin::ApiController < Admin::AdminController + # Note: in the REST API, ApiKeys are referred to simply as "key" + # If we used "api_key", then our user provider would try to use the value for authentication def index - render_serialized(ApiKey.where(hidden: false).to_a, ApiKeySerializer) + keys = ApiKey.where(hidden: false) + + # Put active keys first + # Sort active keys by created_at, sort revoked keys by revoked_at + keys = keys.order(<<~SQL) + CASE WHEN revoked_at IS NULL THEN 0 ELSE 1 END, + COALESCE(revoked_at, created_at) DESC + SQL + + render_serialized(keys.to_a, ApiKeySerializer, root: 'keys') end - def regenerate_key + def show + api_key = ApiKey.find_by!(id: params[:id]) + render_serialized(api_key, ApiKeySerializer, root: 'key') + end + + def update + api_key = ApiKey.find_by!(id: params[:id]) + ApiKey.transaction do + api_key.update!(update_params) + log_api_key(api_key, UserHistory.actions[:api_key_update], changes: api_key.saved_changes) + end + render_serialized(api_key, ApiKeySerializer, root: 'key') + end + + def destroy + api_key = ApiKey.find_by!(id: params[:id]) + ApiKey.transaction do + api_key.destroy + log_api_key(api_key, UserHistory.actions[:api_key_destroy]) + end + render json: success_json + end + + def create + api_key = ApiKey.new(update_params) + ApiKey.transaction do + api_key.created_by = current_user + if username = params.require(:key).permit(:username)[:username].presence + api_key.user = User.find_by_username(username) + raise Discourse::NotFound unless api_key.user + end + api_key.save! + log_api_key(api_key, UserHistory.actions[:api_key_create], changes: api_key.saved_changes) + end + render_serialized(api_key, ApiKeySerializer, root: 'key') + end + + def undo_revoke_key api_key = ApiKey.find_by(id: params[:id]) raise Discourse::NotFound if api_key.blank? - api_key.regenerate!(current_user) + ApiKey.transaction do + api_key.update(revoked_at: nil) + log_api_key_restore(api_key) + end render_serialized(api_key, ApiKeySerializer) end @@ -18,13 +69,32 @@ class Admin::ApiController < Admin::AdminController api_key = ApiKey.find_by(id: params[:id]) raise Discourse::NotFound if api_key.blank? - api_key.destroy - render body: nil - end - - def create_master_key - api_key = ApiKey.create_master_key + ApiKey.transaction do + api_key.update(revoked_at: Time.zone.now) + log_api_key_revoke(api_key) + end render_serialized(api_key, ApiKeySerializer) end + private + + def update_params + editable_fields = [:description] + permitted_params = params.permit(key: [*editable_fields])[:key] + raise Discourse::InvalidParameters unless permitted_params + permitted_params + end + + def log_api_key(*args) + StaffActionLogger.new(current_user).log_api_key(*args) + end + + def log_api_key_revoke(*args) + StaffActionLogger.new(current_user).log_api_key_revoke(*args) + end + + def log_api_key_restore(*args) + StaffActionLogger.new(current_user).log_api_key_restore(*args) + end + end diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 3261e8149a5..bbfd7a0a542 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -19,8 +19,6 @@ class Admin::UsersController < Admin::AdminController :add_group, :remove_group, :primary_group, - :generate_api_key, - :revoke_api_key, :anonymize, :reset_bounce_score, :disable_second_factor, @@ -102,7 +100,6 @@ class Admin::UsersController < Admin::AdminController User.transaction do @user.save! - @user.revoke_api_key user_history = StaffActionLogger.new(current_user).log_user_suspend( @user, @@ -179,16 +176,6 @@ class Admin::UsersController < Admin::AdminController render body: nil end - def generate_api_key - api_key = @user.generate_api_key(current_user) - render_serialized(api_key, ApiKeySerializer) - end - - def revoke_api_key - @user.revoke_api_key - render body: nil - end - def grant_admin AdminConfirmation.new(@user, current_user).create_confirmation render json: success_json diff --git a/app/jobs/scheduled/clean_up_unused_api_keys.rb b/app/jobs/scheduled/clean_up_unused_api_keys.rb new file mode 100644 index 00000000000..4b9e756e631 --- /dev/null +++ b/app/jobs/scheduled/clean_up_unused_api_keys.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +module Jobs + + class CleanUpUnusedApiKeys < ::Jobs::Scheduled + every 1.day + + def execute(args) + ApiKey.revoke_unused_keys! + end + + end + +end diff --git a/app/models/api_key.rb b/app/models/api_key.rb index 6062025519a..198d746b30d 100644 --- a/app/models/api_key.rb +++ b/app/models/api_key.rb @@ -4,23 +4,44 @@ class ApiKey < ActiveRecord::Base belongs_to :user belongs_to :created_by, class_name: 'User' - validates :user_id, uniqueness: true + scope :active, -> { where("revoked_at IS NULL") } + scope :revoked, -> { where("revoked_at IS NOT NULL") } + validates_presence_of :key - def regenerate!(updated_by) - self.key = SecureRandom.hex(32) - self.created_by = updated_by - save! + after_initialize :generate_key + + def generate_key + self.key ||= SecureRandom.hex(32) end - def self.create_master_key - api_key = ApiKey.find_by(user_id: nil, hidden: false) - if api_key.blank? - api_key = ApiKey.create(key: SecureRandom.hex(32), created_by: Discourse.system_user) + def truncated_key + self.key[0..3] + end + + def self.last_used_epoch + SiteSetting.api_key_last_used_epoch.presence + end + + def self.revoke_unused_keys! + return if SiteSetting.revoke_api_keys_days == 0 # Never expire keys + to_revoke = active.where("GREATEST(last_used_at, created_at, updated_at, :epoch) < :threshold", + epoch: last_used_epoch, + threshold: SiteSetting.revoke_api_keys_days.days.ago + ) + + to_revoke.find_each do |api_key| + ApiKey.transaction do + api_key.update!(revoked_at: Time.zone.now) + + StaffActionLogger.new(Discourse.system_user).log_api_key( + api_key, + UserHistory.actions[:api_key_update], + changes: api_key.saved_changes, + context: I18n.t("staff_action_logs.api_key.automatic_revoked", count: SiteSetting.revoke_api_keys_days)) + end end - api_key end - end # == Schema Information diff --git a/app/models/user.rb b/app/models/user.rb index adaf9ee9bbd..ccd5928853a 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -84,7 +84,7 @@ class User < ActiveRecord::Base has_many :muted_user_records, class_name: 'MutedUser' has_many :muted_users, through: :muted_user_records - has_one :api_key, dependent: :destroy + has_many :api_keys, dependent: :destroy has_many :push_subscriptions, dependent: :destroy @@ -1028,19 +1028,6 @@ class User < ActiveRecord::Base uploaded_avatar.present? end - def generate_api_key(created_by) - if api_key.present? - api_key.regenerate!(created_by) - api_key - else - ApiKey.create!(user: self, key: SecureRandom.hex(32), created_by: created_by) - end - end - - def revoke_api_key - ApiKey.where(user_id: self.id).delete_all - end - def find_email last_sent_email_address.present? && EmailValidator.email_regex =~ last_sent_email_address ? last_sent_email_address : email end diff --git a/app/models/user_history.rb b/app/models/user_history.rb index 015d5bd15f1..ec6fec4f11e 100644 --- a/app/models/user_history.rb +++ b/app/models/user_history.rb @@ -97,7 +97,10 @@ class UserHistory < ActiveRecord::Base web_hook_deactivate: 76, change_theme_setting: 77, disable_theme_component: 78, - enable_theme_component: 79 + enable_theme_component: 79, + api_key_create: 80, + api_key_update: 81, + api_key_destroy: 82, ) end @@ -171,7 +174,10 @@ class UserHistory < ActiveRecord::Base :embeddable_host_destroy, :change_theme_setting, :disable_theme_component, - :enable_theme_component + :enable_theme_component, + :api_key_create, + :api_key_update, + :api_key_destroy, ] end diff --git a/app/serializers/admin_detailed_user_serializer.rb b/app/serializers/admin_detailed_user_serializer.rb index abe0d137736..3555522497a 100644 --- a/app/serializers/admin_detailed_user_serializer.rb +++ b/app/serializers/admin_detailed_user_serializer.rb @@ -29,10 +29,10 @@ class AdminDetailedUserSerializer < AdminUserSerializer :reset_bounce_score_after, :can_view_action_logs, :second_factor_enabled, - :can_disable_second_factor + :can_disable_second_factor, + :api_key_count has_one :approved_by, serializer: BasicUserSerializer, embed: :objects - has_one :api_key, serializer: ApiKeySerializer, embed: :objects has_one :suspended_by, serializer: BasicUserSerializer, embed: :objects has_one :silenced_by, serializer: BasicUserSerializer, embed: :objects has_one :tl3_requirements, serializer: TrustLevel3RequirementsSerializer, embed: :objects @@ -118,4 +118,7 @@ class AdminDetailedUserSerializer < AdminUserSerializer object.posts.count end + def api_key_count + object.api_keys.active.count + end end diff --git a/app/serializers/api_key_serializer.rb b/app/serializers/api_key_serializer.rb index 59f874be3ed..c743b97d52e 100644 --- a/app/serializers/api_key_serializer.rb +++ b/app/serializers/api_key_serializer.rb @@ -4,8 +4,11 @@ class ApiKeySerializer < ApplicationSerializer attributes :id, :key, + :description, :last_used_at, - :created_at + :created_at, + :updated_at, + :revoked_at has_one :user, serializer: BasicUserSerializer, embed: :objects diff --git a/app/services/staff_action_logger.rb b/app/services/staff_action_logger.rb index e564c6c790f..f6264da3eb4 100644 --- a/app/services/staff_action_logger.rb +++ b/app/services/staff_action_logger.rb @@ -663,6 +663,39 @@ class StaffActionLogger )) end + def log_api_key(api_key, action, opts = {}) + opts[:changes]&.delete("key") # Do not log the full key + + history_params = params(opts).merge( + action: action, + subject: api_key.truncated_key + ) + + if opts[:changes] + old_values, new_values = get_changes(opts[:changes]) + history_params[:previous_value] = old_values&.join(", ") unless opts[:changes].keys.include?("id") + history_params[:new_value] = new_values&.join(", ") + end + + UserHistory.create!(history_params) + end + + def log_api_key_revoke(api_key) + UserHistory.create!(params.merge( + subject: api_key.truncated_key, + action: UserHistory.actions[:api_key_update], + details: I18n.t("staff_action_logs.api_key.revoked") + )) + end + + def log_api_key_restore(api_key) + UserHistory.create!(params.merge( + subject: api_key.truncated_key, + action: UserHistory.actions[:api_key_update], + details: I18n.t("staff_action_logs.api_key.restored") + )) + end + private def get_changes(changes) diff --git a/app/services/user_anonymizer.rb b/app/services/user_anonymizer.rb index fbc560dec03..99c7dc6dd46 100644 --- a/app/services/user_anonymizer.rb +++ b/app/services/user_anonymizer.rb @@ -66,7 +66,7 @@ class UserAnonymizer @user.user_associated_accounts.try(:destroy_all) @user.instagram_user_info.try(:destroy) @user.user_open_ids.find_each { |x| x.destroy } - @user.api_key.try(:destroy) + @user.api_keys.find_each { |x| x.try(:destroy) } @user.user_emails.secondary.destroy_all @user_history = log_action diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index a431052d317..aeb0acdc58b 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -3315,18 +3315,30 @@ en: none: "There are no active API keys right now." user: "User" title: "API" - key: "API Key" + key: "Key" created: Created + updated: Updated last_used: Last Used never_used: (never) generate: "Generate" - regenerate: "Regenerate" + undo_revoke: "Undo Revoke" revoke: "Revoke" - confirm_regen: "Are you sure you want to replace that API Key with a new one?" - confirm_revoke: "Are you sure you want to revoke that key?" - info_html: "Your API key will allow you to create and update topics using JSON calls." all_users: "All Users" - note_html: "Keep this key secret, all users that have it may create arbitrary posts as any user." + active_keys: "Active API Keys" + manage_keys: Manage Keys + show_details: Details + description: Description + no_description: (no description) + all_api_keys: All API Keys + user_mode: User Level + impersonate_all_users: Impersonate any user + single_user: "Single User" + user_placeholder: Enter username + description_placeholder: What will this key be used for? + save: Save + new_key: New API Key + revoked: Revoked + delete: Permenantly Delete web_hooks: title: "Webhooks" @@ -3918,6 +3930,9 @@ en: change_theme_setting: "change theme setting" disable_theme_component: "disable theme component" enable_theme_component: "enable theme component" + api_key_create: "api key create" + api_key_update: "api key update" + api_key_destroy: "api key destroy" screened_emails: title: "Screened Emails" description: "When someone tries to create a new account, the following email addresses will be checked and the registration will be blocked, or some other action performed." diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index b1619c47d4e..fded4fd68d1 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -2078,6 +2078,7 @@ en: retain_web_hook_events_period_days: "Number of days to retain web hook event records." retry_web_hook_events: "Automatically retry failed web hook events for 4 times. Time gaps between the retries are 1, 5, 25 and 125 minutes." + revoke_api_keys_days: "Number of days before an unused API key is automatically revoked (0 for never)" allow_user_api_keys: "Allow generation of user API keys" allow_user_api_key_scopes: "List of scopes allowed for user API keys" @@ -4572,6 +4573,12 @@ en: user_merged: "%{username} was merged into this account" user_delete_self: "Deleted by self from %{url}" webhook_deactivation_reason: "Your webhook has been automatically deactivated. We received multiple '%{status}' HTTP status failure responses." + api_key: + automatic_revoked: + one: "Automatically revoked, last activity more than %{count} day ago" + other: "Automatically revoked, last activity more than %{count} days ago" + revoked: Revoked + restored: Restored reviewables: priorities: diff --git a/config/routes.rb b/config/routes.rb index 81cc8d095ee..8fb57fab5e6 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -111,7 +111,6 @@ Discourse::Application.routes.draw do put "revoke_admin", constraints: AdminConstraint.new put "grant_admin", constraints: AdminConstraint.new post "generate_api_key", constraints: AdminConstraint.new - delete "revoke_api_key", constraints: AdminConstraint.new put "revoke_moderation", constraints: AdminConstraint.new put "grant_moderation", constraints: AdminConstraint.new put "approve" @@ -257,10 +256,12 @@ Discourse::Application.routes.draw do resources :api, only: [:index], constraints: AdminConstraint.new do collection do - get "keys" => "api#index" - post "key" => "api#create_master_key" - put "key" => "api#regenerate_key" - delete "key" => "api#revoke_key" + resources :keys, controller: 'api', only: [:index, :show, :update, :create, :destroy] do + member do + post "revoke" => "api#revoke_key" + post "undo-revoke" => "api#undo_revoke_key" + end + end resources :web_hooks get 'web_hook_events/:id' => 'web_hooks#list_events', as: :web_hook_events diff --git a/config/site_settings.yml b/config/site_settings.yml index a70e811b79a..63f1aecf1c1 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -2056,6 +2056,11 @@ api: default: 30 retry_web_hook_events: default: false + api_key_last_used_epoch: + default: "" # Value is added in a migration + hidden: true + revoke_api_keys_days: + default: 180 user_api: allow_user_api_keys: diff --git a/db/migrate/20191101113230_add_revoked_at_to_api_key.rb b/db/migrate/20191101113230_add_revoked_at_to_api_key.rb new file mode 100644 index 00000000000..c6453ab1593 --- /dev/null +++ b/db/migrate/20191101113230_add_revoked_at_to_api_key.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true +class AddRevokedAtToApiKey < ActiveRecord::Migration[5.2] + def up + add_column :api_keys, :revoked_at, :datetime + add_column :api_keys, :description, :text + + execute "INSERT INTO site_settings(name, data_type, value, created_at, updated_at) + VALUES ('api_key_last_used_epoch', 1, now(), now(), now())" + + remove_index :api_keys, :user_id # Remove unique index + add_index :api_keys, :user_id + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/lib/auth/default_current_user_provider.rb b/lib/auth/default_current_user_provider.rb index 24ed4122fef..d7ffdbeb3ec 100644 --- a/lib/auth/default_current_user_provider.rb +++ b/lib/auth/default_current_user_provider.rb @@ -281,7 +281,7 @@ class Auth::DefaultCurrentUserProvider end def lookup_api_user(api_key_value, request) - if api_key = ApiKey.where(key: api_key_value).includes(:user).first + if api_key = ApiKey.active.where(key: api_key_value).includes(:user).first api_username = header_api_key? ? @env[HEADER_API_USERNAME] : request[API_USERNAME] if api_key.allowed_ips.present? && !api_key.allowed_ips.any? { |ip| ip.include?(request.ip) } diff --git a/spec/components/auth/default_current_user_provider_spec.rb b/spec/components/auth/default_current_user_provider_spec.rb index 14715096a83..616de7b4c03 100644 --- a/spec/components/auth/default_current_user_provider_spec.rb +++ b/spec/components/auth/default_current_user_provider_spec.rb @@ -65,6 +65,24 @@ describe Auth::DefaultCurrentUserProvider do expect(key.last_used_at).to eq(nil) end + it "raises for a revoked key" do + user = Fabricate(:user) + key = ApiKey.create!(key: "hello") + expect( + provider("/?api_key=hello&api_username=#{user.username.downcase}").current_user.id + ).to eq(user.id) + + key.reload.update(revoked_at: Time.zone.now, last_used_at: nil) + expect(key.reload.last_used_at).to eq(nil) + + expect { + provider("/?api_key=hello&api_username=#{user.username.downcase}").current_user + }.to raise_error(Discourse::InvalidAccess) + + key.reload + expect(key.last_used_at).to eq(nil) + end + it "raises for a user with a mismatching ip" do user = Fabricate(:user) ApiKey.create!(key: "hello", user_id: user.id, created_by_id: -1, allowed_ips: ['10.0.0.0/24']) diff --git a/spec/models/api_key_spec.rb b/spec/models/api_key_spec.rb index fd6ac20303d..aaaf7196041 100644 --- a/spec/models/api_key_spec.rb +++ b/spec/models/api_key_spec.rb @@ -10,12 +10,59 @@ describe ApiKey do it { is_expected.to belong_to :created_by } it { is_expected.to validate_presence_of :key } - it 'validates uniqueness of user_id' do - Fabricate(:api_key, user: user) - api_key = Fabricate.build(:api_key, user: user) + it 'generates a key when saving' do + key = ApiKey.new + key.save! + initial_key = key.key + expect(initial_key.length).to eq(64) - expect(api_key.save).to eq(false) - expect(api_key.errors).to include(:user_id) + # Does not overwrite key when saving again + key.description = "My description here" + key.save! + expect(key.reload.key).to eq(initial_key) + end + + it "can calculate the epoch correctly" do + expect(ApiKey.last_used_epoch.to_datetime).to be_a(DateTime) + + SiteSetting.api_key_last_used_epoch = "" + expect(ApiKey.last_used_epoch).to eq(nil) + end + + it "can automatically revoke keys" do + now = Time.now + + SiteSetting.api_key_last_used_epoch = now - 2.years + SiteSetting.revoke_api_keys_days = 180 # 6 months + + freeze_time now - 1.year + never_used = Fabricate(:api_key) + used_previously = Fabricate(:api_key) + used_previously.update(last_used_at: Time.zone.now) + used_recently = Fabricate(:api_key) + + freeze_time now - 3.months + used_recently.update(last_used_at: Time.zone.now) + + freeze_time now + ApiKey.revoke_unused_keys! + + [never_used, used_previously, used_recently].each(&:reload) + expect(never_used.revoked_at).to_not eq(nil) + expect(used_previously.revoked_at).to_not eq(nil) + expect(used_recently.revoked_at).to eq(nil) + + # Restore them + [never_used, used_previously, used_recently].each { |a| a.update(revoked_at: nil) } + + # Move the epoch to 1 month ago + SiteSetting.api_key_last_used_epoch = now - 1.month + ApiKey.revoke_unused_keys! + + [never_used, used_previously, used_recently].each(&:reload) + expect(never_used.revoked_at).to eq(nil) + expect(used_previously.revoked_at).to eq(nil) + expect(used_recently.revoked_at).to eq(nil) end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 567f94f6df8..73eb8d7cacf 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1135,56 +1135,6 @@ describe User do end end - describe 'api keys' do - fab!(:admin) { Fabricate(:admin) } - fab!(:other_admin) { Fabricate(:admin) } - fab!(:user) { Fabricate(:user) } - - describe '.generate_api_key' do - - it "generates an api key when none exists, and regenerates when it does" do - expect(user.api_key).to be_blank - - # Generate a key - api_key = user.generate_api_key(admin) - expect(api_key.user).to eq(user) - expect(api_key.key).to be_present - expect(api_key.created_by).to eq(admin) - - user.reload - expect(user.api_key).to eq(api_key) - - # Regenerate a key. Keeps the same record, updates the key - new_key = user.generate_api_key(other_admin) - expect(new_key.id).to eq(api_key.id) - expect(new_key.key).to_not eq(api_key.key) - expect(new_key.created_by).to eq(other_admin) - end - - end - - describe '.revoke_api_key' do - - it "revokes an api key when exists" do - expect(user.api_key).to be_blank - - # Revoke nothing does nothing - user.revoke_api_key - user.reload - expect(user.api_key).to be_blank - - # When a key is present it is removed - user.generate_api_key(admin) - user.reload - user.revoke_api_key - user.reload - expect(user.api_key).to be_blank - end - - end - - end - describe "posted too much in topic" do let!(:user) { Fabricate(:user, trust_level: TrustLevel[0]) } let!(:topic) { Fabricate(:post).topic } diff --git a/spec/requests/admin/api_controller_spec.rb b/spec/requests/admin/api_controller_spec.rb index 4a20a474510..217ef19dc55 100644 --- a/spec/requests/admin/api_controller_spec.rb +++ b/spec/requests/admin/api_controller_spec.rb @@ -10,6 +10,9 @@ describe Admin::ApiController do fab!(:admin) { Fabricate(:admin) } + fab!(:key1) { Fabricate(:api_key, description: "my key") } + fab!(:key2) { Fabricate(:api_key, user: admin) } + context "as an admin" do before do sign_in(admin) @@ -19,60 +22,159 @@ describe Admin::ApiController do it "succeeds" do get "/admin/api/keys.json" expect(response.status).to eq(200) + expect(JSON.parse(response.body)["keys"].length).to eq(2) end end - describe '#regenerate_key' do - fab!(:api_key) { Fabricate(:api_key) } - - it "returns 404 when there is no key" do - put "/admin/api/key.json", params: { id: 1234 } - expect(response.status).to eq(404) - end - - it "delegates to the api key's `regenerate!` method" do - prev_value = api_key.key - put "/admin/api/key.json", params: { id: api_key.id } + describe '#show' do + it "succeeds" do + get "/admin/api/keys/#{key1.id}.json" expect(response.status).to eq(200) - - api_key.reload - expect(api_key.key).not_to eq(prev_value) - expect(api_key.created_by.id).to eq(admin.id) + data = JSON.parse(response.body)["key"] + expect(data["id"]).to eq(key1.id) + expect(data["key"]).to eq(key1.key) + expect(data["description"]).to eq("my key") end end - describe '#revoke_key' do - fab!(:api_key) { Fabricate(:api_key) } + describe '#update' do + it "allows updating the description" do + original_key = key1.key - it "returns 404 when there is no key" do - delete "/admin/api/key.json", params: { id: 1234 } - expect(response.status).to eq(404) + put "/admin/api/keys/#{key1.id}.json", params: { + key: { + description: "my new description", + key: "overridekey" + } + } + expect(response.status).to eq(200) + + key1.reload + expect(key1.description).to eq("my new description") + expect(key1.key).to eq(original_key) + + expect(UserHistory.last.action).to eq(UserHistory.actions[:api_key_update]) + expect(UserHistory.last.subject).to eq(key1.truncated_key) end - it "delegates to the api key's `regenerate!` method" do - delete "/admin/api/key.json", params: { id: api_key.id } + it "returns 400 for invalid payloads" do + put "/admin/api/keys/#{key1.id}.json", params: { + key: "string not a hash" + } + expect(response.status).to eq(400) + + put "/admin/api/keys/#{key1.id}.json", params: {} + expect(response.status).to eq(400) + end + end + + describe "#destroy" do + it "works" do + expect(ApiKey.exists?(key1.id)).to eq(true) + + delete "/admin/api/keys/#{key1.id}.json" + expect(response.status).to eq(200) - expect(ApiKey.where(key: api_key.key).count).to eq(0) + expect(ApiKey.exists?(key1.id)).to eq(false) + + expect(UserHistory.last.action).to eq(UserHistory.actions[:api_key_destroy]) + expect(UserHistory.last.subject).to eq(key1.truncated_key) + end + end + + describe "#create" do + it "can create a master key" do + post "/admin/api/keys.json", params: { + key: { + description: "master key description" + } + } + expect(response.status).to eq(200) + + data = JSON.parse(response.body) + + expect(data['key']['description']).to eq("master key description") + expect(data['key']['user']).to eq(nil) + expect(data['key']['key']).to_not eq(nil) + expect(data['key']['last_used_at']).to eq(nil) + + key = ApiKey.find(data['key']['id']) + expect(key.description).to eq("master key description") + expect(key.user).to eq(nil) + + expect(UserHistory.last.action).to eq(UserHistory.actions[:api_key_create]) + expect(UserHistory.last.subject).to eq(key.truncated_key) + end + + it "can create a user-specific key" do + user = Fabricate(:user) + post "/admin/api/keys.json", params: { + key: { + description: "restricted key description", + username: user.username + } + } + expect(response.status).to eq(200) + + data = JSON.parse(response.body) + + expect(data['key']['description']).to eq("restricted key description") + expect(data['key']['user']['username']).to eq(user.username) + expect(data['key']['key']).to_not eq(nil) + expect(data['key']['last_used_at']).to eq(nil) + + key = ApiKey.find(data['key']['id']) + expect(key.description).to eq("restricted key description") + expect(key.user.id).to eq(user.id) + + expect(UserHistory.last.action).to eq(UserHistory.actions[:api_key_create]) + expect(UserHistory.last.subject).to eq(key.truncated_key) + end + end + + describe "#revoke and #undo_revoke" do + it "works correctly" do + post "/admin/api/keys/#{key1.id}/revoke.json" + expect(response.status).to eq 200 + + key1.reload + expect(key1.revoked_at).to_not eq(nil) + expect(UserHistory.last.action).to eq(UserHistory.actions[:api_key_update]) + expect(UserHistory.last.subject).to eq(key1.truncated_key) + expect(UserHistory.last.details).to eq(I18n.t("staff_action_logs.api_key.revoked")) + + post "/admin/api/keys/#{key1.id}/undo-revoke.json" + expect(response.status).to eq 200 + + key1.reload + expect(key1.revoked_at).to eq(nil) + expect(UserHistory.last.action).to eq(UserHistory.actions[:api_key_update]) + expect(UserHistory.last.subject).to eq(key1.truncated_key) + expect(UserHistory.last.details).to eq(I18n.t("staff_action_logs.api_key.restored")) end end end - describe '#create_master_key' do - it "creates a record" do - sign_in(admin) - expect do - post "/admin/api/key.json" - end.to change(ApiKey, :count).by(1) - expect(response.status).to eq(200) - end - - it "doesn't allow moderators to create master keys" do + context "as a moderator" do + before do sign_in(Fabricate(:moderator)) - expect do - post "/admin/api/key.json" - end.to change(ApiKey, :count).by(0) - expect(response.status).to eq(404) end + it "doesn't allow access" do + get "/admin/api/keys.json" + expect(response.status).to eq(404) + + get "/admin/api/key/#{key1.id}.json" + expect(response.status).to eq(404) + + post "/admin/api/keys.json", params: { + key: { + description: "master key description" + } + } + expect(response.status).to eq(404) + + expect(ApiKey.count).to eq(2) + end end end diff --git a/spec/requests/admin/users_controller_spec.rb b/spec/requests/admin/users_controller_spec.rb index e6bf9831d20..724b3a0ad41 100644 --- a/spec/requests/admin/users_controller_spec.rb +++ b/spec/requests/admin/users_controller_spec.rb @@ -123,25 +123,6 @@ RSpec.describe Admin::UsersController do end end - describe '#generate_api_key' do - it 'calls generate_api_key' do - post "/admin/users/#{user.id}/generate_api_key.json" - expect(response.status).to eq(200) - json = JSON.parse(response.body) - expect(json["api_key"]["user"]["id"]).to eq(user.id) - expect(json["api_key"]["key"]).to be_present - end - end - - describe '#revoke_api_key' do - it 'calls revoke_api_key' do - ApiKey.create!(user: user, key: SecureRandom.hex) - delete "/admin/users/#{user.id}/revoke_api_key.json" - expect(response.status).to eq(200) - expect(ApiKey.where(user: user).count).to eq(0) - end - end - describe '#suspend' do fab!(:post) { Fabricate(:post) } let(:suspend_params) do @@ -269,15 +250,26 @@ RSpec.describe Admin::UsersController do expect(log.details).to match(/long reason/) end - it "also revokes any api keys" do - Fabricate(:api_key, user: user) - put "/admin/users/#{user.id}/suspend.json", params: suspend_params + it "also prevents use of any api keys" do + api_key = Fabricate(:api_key, user: user) + put "/posts/#{Fabricate(:post).id}/bookmark.json", params: { + bookmarked: "true", + api_key: api_key.key + } expect(response.status).to eq(200) - user.reload + put "/admin/users/#{user.id}/suspend.json", params: suspend_params + expect(response.status).to eq(200) + + user.reload expect(user).to be_suspended - expect(ApiKey.where(user_id: user.id).count).to eq(0) + + put "/posts/#{Fabricate(:post).id}/bookmark.json", params: { + bookmarked: "true", + api_key: api_key.key + } + expect(response.status).to eq(403) end end diff --git a/spec/requests/embed_controller_spec.rb b/spec/requests/embed_controller_spec.rb index 5f6abb61f4e..69692795028 100644 --- a/spec/requests/embed_controller_spec.rb +++ b/spec/requests/embed_controller_spec.rb @@ -42,7 +42,7 @@ describe EmbedController do context "with api key" do - let(:api_key) { ApiKey.create_master_key } + let(:api_key) { Fabricate(:api_key) } context "with valid embed url" do let(:topic_embed) { Fabricate(:topic_embed, embed_url: embed_url) } diff --git a/spec/requests/posts_controller_spec.rb b/spec/requests/posts_controller_spec.rb index a7e8c7d1a18..45e4327f3f8 100644 --- a/spec/requests/posts_controller_spec.rb +++ b/spec/requests/posts_controller_spec.rb @@ -524,8 +524,8 @@ describe PostsController do end context "api" do - let(:api_key) { user.generate_api_key(user) } - let(:master_key) { ApiKey.create_master_key } + let(:api_key) { Fabricate(:api_key, user: user) } + let(:master_key) { Fabricate(:api_key, user: nil) } # choosing an arbitrarily easy to mock trusted activity it 'allows users with api key to bookmark posts' do @@ -711,7 +711,7 @@ describe PostsController do raw = "this is a test post 123 #{SecureRandom.hash}" title = "this is a title #{SecureRandom.hash}" - master_key = ApiKey.create_master_key.key + master_key = Fabricate(:api_key).key post "/posts.json", params: { api_username: user.username, @@ -740,7 +740,7 @@ describe PostsController do Jobs.run_immediately! NotificationEmailer.enable post_1 = Fabricate(:post) - master_key = ApiKey.create_master_key.key + master_key = Fabricate(:api_key).key post "/posts.json", params: { api_username: user.username, @@ -796,7 +796,7 @@ describe PostsController do it 'will raise an error if specified category cannot be found' do user = Fabricate(:admin) - master_key = ApiKey.create_master_key.key + master_key = Fabricate(:api_key).key post "/posts.json", params: { api_username: user.username, diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index 357ce90fbbe..afbc542e3be 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -1759,7 +1759,7 @@ RSpec.describe TopicsController do end context 'and the user is not logged in' do - let(:api_key) { topic.user.generate_api_key(topic.user) } + let(:api_key) { Fabricate(:api_key, user: topic.user) } it 'redirects to the login page' do get "/t/#{topic.slug}/#{topic.id}.json" diff --git a/spec/requests/user_badges_controller_spec.rb b/spec/requests/user_badges_controller_spec.rb index aa03d6f7d8e..07613c81932 100644 --- a/spec/requests/user_badges_controller_spec.rb +++ b/spec/requests/user_badges_controller_spec.rb @@ -111,10 +111,10 @@ describe UserBadgesController do end it 'does not grant badges from regular api calls' do - Fabricate(:api_key, user: user) + api_key = Fabricate(:api_key, user: user) post "/user_badges.json", params: { - badge_id: badge.id, username: user.username, api_key: user.api_key.key + badge_id: badge.id, username: user.username, api_key: api_key.key } expect(response.status).to eq(403) diff --git a/spec/services/user_anonymizer_spec.rb b/spec/services/user_anonymizer_spec.rb index 3f9deb0e696..d534d8ca162 100644 --- a/spec/services/user_anonymizer_spec.rb +++ b/spec/services/user_anonymizer_spec.rb @@ -218,7 +218,7 @@ describe UserAnonymizer do ApiKey.create(user_id: user.id, key: "123123123") expect { make_anonymous }.to change { ApiKey.count }.by(-1) user.reload - expect(user.api_key).to eq(nil) + expect(user.api_keys).to be_empty end context "executes job" do diff --git a/test/javascripts/admin/models/admin-user-test.js.es6 b/test/javascripts/admin/models/admin-user-test.js.es6 deleted file mode 100644 index 729cd55d05c..00000000000 --- a/test/javascripts/admin/models/admin-user-test.js.es6 +++ /dev/null @@ -1,30 +0,0 @@ -import AdminUser from "admin/models/admin-user"; -import ApiKey from "admin/models/api-key"; - -QUnit.module("model:admin-user"); - -QUnit.test("generate key", function(assert) { - assert.expect(2); - - var adminUser = AdminUser.create({ id: 333 }); - assert.ok(!adminUser.get("api_key"), "it has no api key by default"); - return adminUser.generateApiKey().then(function() { - assert.present(adminUser.get("api_key"), "it has an api_key now"); - }); -}); - -QUnit.test("revoke key", function(assert) { - assert.expect(2); - - var apiKey = ApiKey.create({ id: 1234, key: "asdfasdf" }), - adminUser = AdminUser.create({ id: 333, api_key: apiKey }); - - assert.equal( - adminUser.get("api_key"), - apiKey, - "it has the api key in the beginning" - ); - return adminUser.revokeApiKey().then(function() { - assert.blank(adminUser.get("api_key"), "it cleared the api_key"); - }); -}); diff --git a/test/javascripts/helpers/create-pretender.js.es6 b/test/javascripts/helpers/create-pretender.js.es6 index d05a79a491f..74d93e38ff3 100644 --- a/test/javascripts/helpers/create-pretender.js.es6 +++ b/test/javascripts/helpers/create-pretender.js.es6 @@ -611,8 +611,6 @@ export default function() { }); }); - this.post("/admin/users/:user_id/generate_api_key", success); - this.delete("/admin/users/:user_id/revoke_api_key", success); this.delete("/admin/users/:user_id.json", () => response(200, { deleted: true }) );