-
Notifications
You must be signed in to change notification settings - Fork 35
Make /api/v1/users/me, and add api to extension tracking #704
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: main
Are you sure you want to change the base?
Conversation
| api_key = request.headers["Authorization"]&.remove("Bearer ") | ||
|
|
||
| if api_key.present? | ||
| @current_user = User.find_by(api_key: api_key) |
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.
This is dangerous- we're writing to @current_user using api_key here. this means API keys aren't just for hitting api endpoints, it can now be used to sign in as someone.
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.
yeah i wasnt sure about this, would assigning current_user/user from api key to a local variable and change the rest of the function to refrence that work?
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.
Let's not even call it current_user- it's not the current user. I think user is acceptable here. This is also running on every controller so I'd be careful of doing anything that overwrites the current setup if it has to do with only the api. Are we sure that projects can't just set the header? That'd mean there's only one way for projects to get their users tracked.
I agree with the other change in this PR but I don't totally understand why we need this.
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.
there are some projects that integrate with flavortown but only use the api, and dont ask for user cookies, such as https://flavortown.hackclub.com/projects/1865
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.
changed it to not change @current_user
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 existing setup doesn't use cookies-- it tracks by request headers
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.
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.
but to set current_user they have to be authenticated, which is done via cookies and not an api key
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.
and it only tracks if current_user is set, hence the only way for an non-browser extension that integrates with flavortown to track, would be to require cookies so it can authenticate and it would be counted
No description provided.