-
Notifications
You must be signed in to change notification settings - Fork 7
Add deploy keys #44
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
base: master
Are you sure you want to change the base?
Add deploy keys #44
Conversation
This is a slightly bigger PR (that modifies the DB), so requesting a review from you Joshua! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces deploy keys as a new authentication method for developers to automatically publish app releases. Deploy keys are tied to developer accounts and provide access to a streamlined deployment endpoint that identifies apps by their PBW UUID rather than requiring knowledge of the app ID.
Key changes:
- Added database schema for storing deploy keys and tracking their usage
- Created utility functions for deploy key validation and authentication
- Implemented new
/api/dp/deploy
endpoint for automated release submission - Added deploy key management endpoint for key generation/regeneration
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
migrations/versions/e56d904098e5_add_deploy_key_col.py | Database migration adding deploy_key and deploy_key_last_used columns to developers table |
appstore/models.py | Added deploy_key and deploy_key_last_used fields to Developer model |
appstore/utils.py | Added is_valid_deploy_key_for_app function for deploy key validation |
appstore/developer_portal_api.py | Implemented deploy key management and deployment endpoints |
appstore/dev_portal_api.py | Updated my_apps endpoint to include deploy key status information |
# Update last used time | ||
dev = Developer.query.filter_by(id=app.developer_id).one() | ||
dev.deploy_key_last_used = datetime.datetime.now(datetime.timezone.utc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The deploy key last used time is being updated but the session is not committed until after the release is created. If the release creation fails, the last used time will still be updated. Consider moving this update after successful release creation or using a separate transaction.
# Update last used time | |
dev = Developer.query.filter_by(id=app.developer_id).one() | |
dev.deploy_key_last_used = datetime.datetime.now(datetime.timezone.utc) | |
# Prepare developer object for later update | |
dev = Developer.query.filter_by(id=app.developer_id).one() |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will take a look
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I'm not so sure I agree with the architecture of this, in a handful of ways.
- This has the idea of splitting a developer ID key from a user ID key in terms of authorization to use it. Indeed, at some point in the future, it would be good to have that! But this distinction does not exist elsewhere yet. If we are beginning to make that distinction (where there are many mappings from user IDs <-> developer IDs), should we document that somewhere?
- I'm not really thrilled about the idea of having a single deploy key for each developer ID -- architecturally, that seems like the wrong thing, and storing "secrets" and generating them inside rebble-appstore-api definitely seems like the wrong thing.
It feels like this is more-or-less reinventing the OAuth2 token. What if, instead, appstore-api -- when asked for a deploy key -- turned around and asked auth
to make a subscoped token, associated with that user, but subscoped to only allow deploys to one developer ID, or for that matter, to one app ID? Then if we did that, you could get all kinds of other nice OAuth behavior, like 'revoke tokens that are burned', 'have tokens require refreshing' (and, for free, 'have a refresh endpoint'), 'have a lifespan', etc. -- and also have only one place where we generate (and store!) authenticators.
Some good points!
These keys are deliberately not OAuth, because it was designed with the idea that they'll be inherently less secure (someone might accidentality leak their key).
Only allowing one key per developer was also by design, such that users are less likely to leave forgotten or abandoned keys out there if they only have one that must be regenerated to be recovered. Having this all be a reduced scope OAuth thing would be nice, and in retrospect it's a good idea to keep all the auth in... auth. But I believe we don't currently do stringent enough checking of scopes in parts of the API that support the OG mobile app, I will investigate but I believe that to be the case. We also don't want these deploy keys to require refreshing or a lifespan (a user can revoke but they don't expire after X time). This makes it easier to deploy into a CI/CD workflow. The security implications of a forever token are offset but the very limited scope of the token |
This PR adds the concept of deploy keys to the appstore-api.
Deploy keys:
The new deploy endpoint does not require the caller to know the app ID of the submitted app (unlike the standard release endpoint). Instead the pbw UUID is used to identify the app. All you need is your deploy key, pbw and release note.
It will require a DB update.