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

General API Keys #124

Merged
merged 14 commits into from
Feb 9, 2022
Merged

General API Keys #124

merged 14 commits into from
Feb 9, 2022

Conversation

ro-tex
Copy link
Contributor

@ro-tex ro-tex commented Feb 2, 2022 •
edited
Loading

PULL REQUEST

Overview

This PR implements the generic version of API keys:

  • created by the user on their dashboard
  • do not expire
  • revocable
  • valid as an authentication mechanism (via a header or a get parameter)
  • give full access to all user actions
  • private

Example for Visual Changes

Checklist

Review and complete the checklist to ensure that the PR is complete before assigned to an approver.

  • All new methods or updated methods have clear docstrings
  • Testing added or updated for new methods
  • Verify if any changes impact the WebPortal Health Checks
  • Approriate documentation updated
  • Changelog file created

Issues Closed

@ro-tex ro-tex self-assigned this Feb 2, 2022
@ro-tex ro-tex marked this pull request as ready for review February 3, 2022 14:54
peterjan
peterjan reviewed Feb 3, 2022
View reviewed changes
test/api/handlers_test.go Outdated Show resolved Hide resolved
MSevey
MSevey reviewed Feb 3, 2022
View reviewed changes
api/apikeys.go Show resolved Hide resolved
MSevey
MSevey reviewed Feb 3, 2022
View reviewed changes
api/apikeys.go Show resolved Hide resolved
MSevey
MSevey reviewed Feb 3, 2022
View reviewed changes
api/handlers.go Outdated Show resolved Hide resolved
peterjan
peterjan requested changes Feb 3, 2022
View reviewed changes
Copy link

@peterjan peterjan left a comment

Choose a reason for hiding this comment

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

Great start I think. Is there an API keys spec? I think the current format is quite loose? It doesn't really consider future extensions, like permissions for example. Should we think about stuff like that already? I don't mind it being loosely defined, I'm just asking.

test/api/apikeys_test.go Outdated Show resolved Hide resolved
test/api/apikeys_test.go Show resolved Hide resolved
test/api/handlers_test.go Outdated Show resolved Hide resolved
test/api/apikeys_test.go Outdated Show resolved Hide resolved
database/apikeys.go Outdated Show resolved Hide resolved
database/apikeys.go Outdated Show resolved Hide resolved
api/routes.go Outdated Show resolved Hide resolved
api/routes.go Show resolved Hide resolved
api/routes.go Show resolved Hide resolved
api/apikeys.go Show resolved Hide resolved
MSevey
MSevey reviewed Feb 3, 2022
View reviewed changes
api/routes.go Show resolved Hide resolved
MSevey
MSevey reviewed Feb 3, 2022
View reviewed changes
Copy link
Contributor

@MSevey MSevey left a comment

Choose a reason for hiding this comment

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

Generally lgtm

@ro-tex
Copy link
Contributor Author

ro-tex commented Feb 4, 2022

Great start I think. Is there an API keys spec? I think the current format is quite loose? It doesn't really consider future extensions, like permissions for example. Should we think about stuff like that already? I don't mind it being loosely defined, I'm just asking.

This is the spec: https://hackmd.io/tB6WDA73Q7iQGSFTqinVZg

My intention was to get the simple case going first and then build on top of it. We agreed that there won't be any permissions for the moment, as that would expand the scope a lot.

We want to have the so-called "sponsor" api keys which allow anyone to download a skylink with the owner of the api key paying for the traffic. I see those as a separate PR because there will be quite some code around them as well and I don't want this to be a 2000 lines PR. I'd rather iterate over the idea in 2, 3, even 5 PRs.

ro-tex added 2 commits February 4, 2022 11:40
@ro-tex
DELETE returns Bad Request if the API key is not found.
64661e8 
userLimitsGET returns anonymous limits and logs an error if the given API key is not valid.
Endpoints renamed from `/user/apikey` to `/user/apikeys`.
testAPIKeysUsage now validates user stats values.
@ro-tex
Rename all collection name constants to have the coll prefix and no…
e57500d 
…t the `db` prefix.
ChrisSchinnerl
ChrisSchinnerl requested changes Feb 4, 2022
View reviewed changes
api/apikeys.go Show resolved Hide resolved
api/apikeys.go Show resolved Hide resolved
api/apikeys.go Show resolved Hide resolved
api/apikeys.go Show resolved Hide resolved
api/handlers.go Outdated Show resolved Hide resolved
database/user.go Show resolved Hide resolved
database/database.go Outdated Show resolved Hide resolved
database/apikeys.go Outdated Show resolved Hide resolved
database/apikeys.go Outdated Show resolved Hide resolved
database/apikeys.go Outdated Show resolved Hide resolved
@ro-tex ro-tex mentioned this pull request Feb 4, 2022
Closed
@ro-tex ro-tex marked this pull request as draft February 4, 2022 15:34
@ro-tex ro-tex marked this pull request as ready for review February 4, 2022 16:32
ChrisSchinnerl
ChrisSchinnerl approved these changes Feb 7, 2022
View reviewed changes
Copy link
Contributor

@ChrisSchinnerl ChrisSchinnerl left a comment

Choose a reason for hiding this comment

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

Approved with some f/u comments.

api/routes_test.go Show resolved Hide resolved
cmd/crdb_transition/main.go Show resolved Hide resolved
database/apikeys.go Show resolved Hide resolved
test/api/apikeys_test.go Show resolved Hide resolved
peterjan
peterjan approved these changes Feb 8, 2022
View reviewed changes
Copy link

@peterjan peterjan left a comment

Choose a reason for hiding this comment

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

LGTM, but we should def. make sure to F/U with all of the F/U's

This was referenced Feb 9, 2022
Closed
Open
@ro-tex ro-tex merged commit 1ff1034 into main Feb 9, 2022
@ro-tex ro-tex deleted the ivo/api_keys_general branch February 9, 2022 13:36
This was referenced Feb 9, 2022
Merged
Merged
@kwypchlo kwypchlo mentioned this pull request Feb 17, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChrisSchinnerl ChrisSchinnerl ChrisSchinnerl approved these changes

@peterjan peterjan peterjan approved these changes

@MSevey MSevey Awaiting requested review from MSevey

Assignees

@ro-tex ro-tex

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ro-tex @peterjan @ChrisSchinnerl @MSevey