Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[WIP] API Key auth #9869

Closed
wants to merge 38 commits into from
Closed

[WIP] API Key auth #9869

wants to merge 38 commits into from

Conversation

kevinansfield
Copy link
Member

@kevinansfield kevinansfield commented Sep 13, 2018 •
edited
Loading

refs #9865

  • adds integrations and api_keys tables
  • adds Admin API Client role
  • adds Integration model
  • adds ApiKey model
    • creates default secret if not given
    • hardcodes associated role based on key type
      • admin = Admin API Client
      • content = no role
  • adds API Key authentication
  • sets req.api_key and context.api_key for API key requests
  • updates can-this and the Model.permissible functions to work with context.api_key
    • permissions providers.apiKey added to fetch role permissions associated with the api key
  • sets up integrations and api_keys test fixtures

TODO:

  • determine why post create/edit tests are failing with 422 'Value in [users.name] cannot be blank.'. Likely either or both:
    • API needs a user passed in because it can't retrieve it from context.user
    • created/updated_by is dependent on context.user
  • invalid accesstoken test is throwing 500 rather than 401
  • fix broken permissions tests
  • add user model permissions tests for api keys being unable to assign Owner permissions
  • review TODOs
    • how to handle invites created via api key
    • test for e-mail test not being trigger able via api key
    • review context.user usage in api/users.handlePermissions
    • log api key when making requests
    • always allow staticPages v2 Admin API requests (separate PR)
  • review naming
    • API vs Api
    • api_key vs apiKey

Separate PRs:

  • sync up middleware with session auth, rename authenticatePrivate
  • how to handle "internal" clients (scheduler + backup)
  • set created/updated/published_by columns to api key value
  • update api_key.lastSeenAt value for authenticated api key requests
  • integration and api_key routes and controllers

@kevinansfield kevinansfield changed the title API v2 - Client auth [WIP] API v2 - Client auth Sep 13, 2018
@kevinansfield kevinansfield force-pushed the api-key-auth branch 2 times, most recently from ac1057f to d94d210 Compare September 13, 2018 13:27
allouis
allouis reviewed Sep 13, 2018
View reviewed changes
core/server/data/migrations/versions/2.2/2-add-integrations-tables.js Outdated
context: {internal: true}
}, options);

// TODO: does this rule need adjusting in our eslint config?

This comment was marked as abuse.

Sign in to view

kevinansfield
kevinansfield commented Sep 13, 2018
View reviewed changes
core/server/data/schema/schema.js Outdated
nullable: false,
validations: {isIn: [['content', 'admin']]}
},
secret: {type: 'string', maxlength: 64, nullable: false, unique: true},

This comment was marked as abuse.

Sign in to view

kevinansfield
kevinansfield commented Sep 14, 2018
View reviewed changes
core/server/services/auth/api-key/admin.js Outdated
}));
}

// TODO: should we do Buffer.from(x, 'hex') or is using the secret as-is ok?

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

kevinansfield
kevinansfield commented Sep 14, 2018
View reviewed changes
core/server/services/auth/api-key/admin.js Outdated
try {
// TODO: grab the decoded payload and check if the payload endpoint
// matches the requested endpoint
jwt.verify(token, apiKey.get('secret'), JWT_OPTIONS);

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

allouis
allouis reviewed Sep 17, 2018
View reviewed changes
core/server/services/auth/api-key/admin.js Outdated
let [scheme, credentials] = header.split(' ');

if (/^Bearer$/i.test(scheme)) {
let [apiKeyId, token] = credentials.split('|');

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

allouis
allouis reviewed Sep 17, 2018
View reviewed changes
core/server/services/auth/api-key/admin.js Outdated
return next();
}

let {apiKeyId, token} = extractCredentialsFromHeader(req.headers.authorization);

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

allouis
allouis reviewed Sep 17, 2018
View reviewed changes
core/server/services/auth/api-key/admin.js Outdated
* @param {string} header
*/
const extractCredentialsFromHeader = function extractCredentialsFromHeader(header) {
let [scheme, credentials] = header.split(' ');

This comment was marked as abuse.

Sign in to view

@allouis allouis mentioned this pull request Sep 18, 2018
Closed
@kevinansfield kevinansfield force-pushed the api-key-auth branch 2 times, most recently from 8a725e2 to 6b95f5f Compare September 18, 2018 14:07
kevinansfield
kevinansfield commented Sep 24, 2018
View reviewed changes
core/server/services/permissions/can-this.js
@@ -91,11 +101,11 @@ CanThisResult.prototype.buildObjectTypeHandlers = function (objTypes, actType, c
// Offer a chance for the TargetModel to override the results
if (TargetModel && _.isFunction(TargetModel.permissible)) {
return TargetModel.permissible(
modelId, actType, context, unsafeAttrs, loadedPermissions, hasUserPermission, hasAppPermission
modelId, actType, context, unsafeAttrs, loadedPermissions, hasUserPermission, hasApiKeyPermission, hasAppPermission

This comment was marked as abuse.

Sign in to view

@kevinansfield
remove key ID from content auth query param
4f33d8e
@kevinansfield
start wiring up client auth for new v2 admin endpoints
f1c8272
@kevinansfield
WIP: adapt posts route tests for v2 Admin API w/client auth
81a869c
@kevinansfield
finish setup for running route tests
84b21aa
@kevinansfield
add apiKey to permissions providers
78d8f4a
@kevinansfield
add some todos for dealing with api key requests vs user requests
b03eddb
@kevinansfield
wire up can-this and permissible functions for context.api_key
79f0665
@kevinansfield
return 422 BadRequestError for invalid JWT instead of 500
b08fb1f
@kevinansfield
stick to common.errors.* rather than destructuring
ad6467e
@kevinansfield
fixed incorrect response code in invalid accesstoken request
4f60c22
@kevinansfield
API Key requests must specify post authors
aa52860
@kevinansfield
replace API with Api in method names for consistency
5621483
@kevinansfield
fix post.permissible tests
14007d0
@kevinansfield
fix handlePublicPermissions tests
6036f09
@kevinansfield
fix user.permissible tests
66f16f8
@kevinansfield
send invites as owner if added via an admin api key
f63963d
@kevinansfield kevinansfield changed the title [WIP] API v2 - Client auth [WIP] API v2 - API Key auth Sep 25, 2018
@kevinansfield kevinansfield changed the title [WIP] API v2 - API Key auth [WIP] API Key auth Sep 25, 2018
This was referenced Sep 25, 2018
Merged
Closed
This was referenced Oct 9, 2018
Merged
Merged
Merged
Merged
Closed
Closed
@kevinansfield
Copy link
Member Author

This has been superseded by separate PRs. Closing.

@kirrg001 kirrg001 deleted the api-key-auth branch February 5, 2019 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@letsjustfixit letsjustfixit letsjustfixit left review comments

@vikaspotluri123 vikaspotluri123 vikaspotluri123 left review comments

@allouis allouis allouis left review comments

@kirrg001 kirrg001 kirrg001 left review comments

@naz naz naz left review comments

Assignees

@kirrg001 kirrg001

@allouis allouis

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@kevinansfield @naz @kirrg001 @allouis @vikaspotluri123 @letsjustfixit