-
Notifications
You must be signed in to change notification settings - Fork 26
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
API keys support on the backend #379
base: dev
Are you sure you want to change the base?
Conversation
…ect /api/watched endpoint using the APIKeyMiddleware
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.
Hey @titanventura, thanks for making a PR (and sorry for taking so long getting to it)!
I had a quick look and noticed some changes we could make, let me know if you have any thoughts on the feedback.
I noticed the APIKeyMiddlewareWrapper
, did adding the header check to the AuthRequired
middleware not work out? I think that way would be preferable, just need to move the x-api-key
header check into AuthRequired
and if we find it, we can process it as an API key instead (in APIKeyMiddlewareWrapper, if you'd like, but probably renamed).
I know this is a draft, so you may have been getting to this, but the route handlers seem to have all the logic in them. We could separate most of it out to 'service' functions.
Thanks again for taking the time!
ID uint `gorm:"primarykey" json:"id"` | ||
CreatedAt time.Time `json:"createdAt"` | ||
Value string `gorm:"not null" json:"value"` | ||
UserID uint `gorm:"not null" json:"-"` |
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.
We may want to make this a foreign key that maps to a User to ensure data integrity.
"gorm.io/gorm" | ||
) | ||
|
||
type ApiKey struct { |
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.
An UpdatedAt
field might be worth adding in here too.
…s and change middleware structure
@IRHM , please go through the updated PR. Thanks ! |
Hi @titanventura, I'm happy to get this merged. Thanks for the good work! There are a few minor things, but It's probably best I do them after deciding how the UI will work so it isn't wasted effort incase it needs changing. Thanks! |
Fixes #360 partially
Changes made
/api/watched
endpoint