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

feat(api key management) Extract api key model #2734

Merged
merged 2 commits into from
Oct 30, 2024
Merged

feat(api key management) Extract api key model #2734

merged 2 commits into from
Oct 30, 2024

Conversation

floganz
Copy link
Contributor

@floganz floganz commented Oct 23, 2024

Context

One tiny step towards advanced API key management.

Description

Introduces new model ApiKey that will replace api_key column on the Organization model during next steps.
Migrate existing values for api_key to the dedicated model.
Adjust REST API authorization to use lookup for ApiKey model and temporary keep previous lookup for Organization model.

@floganz floganz self-assigned this Oct 23, 2024
@floganz floganz force-pushed the feat-api-key-management-extract-api-key-model branch 2 times, most recently from 6701cca to d16271d Compare October 23, 2024 13:49
ancorcruz
ancorcruz requested changes Oct 23, 2024
View reviewed changes
Copy link
Contributor

@ancorcruz ancorcruz left a comment

Choose a reason for hiding this comment

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

Good stuff, I have left you a few comments.

Let me know if you would like to have a chat about my comments.

app/controllers/api/base_controller.rb Outdated Show resolved Hide resolved
app/models/api_key.rb Outdated Show resolved Hide resolved
app/services/organizations/create_service.rb Outdated Show resolved Hide resolved
db/migrate/20241021140054_create_api_keys.rb Outdated Show resolved Hide resolved
db/migrate/20241021140054_create_api_keys.rb Show resolved Hide resolved
app/models/api_key.rb Show resolved Hide resolved
spec/factories/organizations.rb Outdated Show resolved Hide resolved
spec/models/api_key_spec.rb Outdated Show resolved Hide resolved
spec/models/api_key_spec.rb Outdated Show resolved Hide resolved
@floganz floganz changed the title Feat api key management extract api key model feat(api key management) Extract api key model Oct 24, 2024
vincent-pochet
vincent-pochet requested changes Oct 24, 2024
View reviewed changes
app/services/users_service.rb Outdated Show resolved Hide resolved
db/migrate/20241021140054_create_api_keys.rb Outdated Show resolved Hide resolved
spec/models/organization_spec.rb Outdated Show resolved Hide resolved
@floganz floganz force-pushed the feat-api-key-management-extract-api-key-model branch from d16271d to c57ccf2 Compare October 25, 2024 08:44
@floganz
Copy link
Contributor Author

floganz commented Oct 25, 2024

@ancorcruz @vincent-pochet Thanks for your reviews!
I've pushed changes, can you take another look?

vincent-pochet
vincent-pochet reviewed Oct 25, 2024
View reviewed changes
Copy link
Collaborator

@vincent-pochet vincent-pochet left a comment

Choose a reason for hiding this comment

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

Looks great! 🚀

Just one comment around the fallback for the lookup of the API key at organization level in the base controller

ancorcruz
ancorcruz approved these changes Oct 29, 2024
View reviewed changes
Copy link
Contributor

@ancorcruz ancorcruz left a comment

Choose a reason for hiding this comment

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

LGTM. thanks!

@floganz floganz force-pushed the feat-api-key-management-extract-api-key-model branch from c57ccf2 to 926f8cb Compare October 30, 2024 08:52
@floganz floganz merged commit 550e792 into main Oct 30, 2024
6 checks passed
@floganz floganz deleted the feat-api-key-management-extract-api-key-model branch October 30, 2024 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vincent-pochet vincent-pochet vincent-pochet approved these changes

@ancorcruz ancorcruz ancorcruz approved these changes

Assignees

@floganz floganz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@floganz @ancorcruz @vincent-pochet