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.

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

Public API Keys #140

Merged
merged 31 commits into from
Mar 18, 2022
Merged

Public API Keys #140

merged 31 commits into from
Mar 18, 2022

Conversation

ro-tex
Copy link
Contributor

@ro-tex ro-tex commented Feb 25, 2022

PULL REQUEST

Overview

Public API keys are a special type of API keys which are meant to be shared with the public. Their validity is very limited - they only allow users to download skylinks and even that is limited to a pre-determined list of skylinks.

The main questions in this PR are:

  • Should we store the public API keys together with the regular ones or not? From my perspective, they are sufficiently different to require separate storage and handling.
  • Are we happy with the authentication flow? It seems a bit too complicated to me, maybe there are ways to simplify it.

Currently open TODOs:

  • handlers need to updated to support public api keys
  • we need integration tests
  • we need tests for everything new in auth.go

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 25, 2022
api/cache.go Outdated Show resolved Hide resolved
database/publicapikeys.go Outdated Show resolved Hide resolved
@ro-tex ro-tex mentioned this pull request Mar 8, 2022
5 tasks
@ro-tex ro-tex marked this pull request as ready for review March 11, 2022 12:46
api/apikeys.go Outdated Show resolved Hide resolved
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.

looks good overall, just requesting changes because I have couple of suggestions

api/apikeys.go Outdated Show resolved Hide resolved
api/apikeys.go Show resolved Hide resolved
api/apikeys.go Show resolved Hide resolved
api/apikeys.go Outdated Show resolved Hide resolved
api/apikeys.go Show resolved Hide resolved
database/apikeys.go Show resolved Hide resolved
database/user.go Show resolved Hide resolved
test/api/apikeys_test.go Outdated Show resolved Hide resolved
test/database/apikeys_test.go Show resolved Hide resolved
test/utils.go Show resolved Hide resolved
@ro-tex ro-tex mentioned this pull request Mar 15, 2022
7 tasks
@ro-tex ro-tex requested a review from peterjan March 15, 2022 16:35
peterjan
peterjan previously approved these changes Mar 17, 2022
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.

looks good to me!

api/apikeys.go Outdated Show resolved Hide resolved
api/apikeys.go Outdated Show resolved Hide resolved
api/apikeys.go Outdated Show resolved Hide resolved
api/apikeys.go Outdated Show resolved Hide resolved
api/apikeys.go Show resolved Hide resolved
api/apikeys.go Show resolved Hide resolved
api/auth.go Show resolved Hide resolved
Co-authored-by: Peter-Jan Brone <peter-jan@settlemint.com>
ro-tex and others added 4 commits March 17, 2022 14:10
Co-authored-by: Peter-Jan Brone <peter-jan@settlemint.com>
Co-authored-by: Peter-Jan Brone <peter-jan@settlemint.com>
# Conflicts:
#	database/upload.go
Cover other PR comments.
@ro-tex ro-tex requested a review from peterjan March 17, 2022 13:52
# Conflicts:
#	api/handlers.go
#	test/api/api_test.go
#	test/api/handlers_test.go
#	test/tester.go
@ro-tex ro-tex merged commit 6f88121 into main Mar 18, 2022
@ro-tex ro-tex deleted the ivo/pub_api_keys branch March 18, 2022 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants