Add scope and hashing to api keys #1962
Conversation
|
|
054da28 to
15435e7
Compare
|
Updated to add tests and rake task for migration to
EDIT: strike outdated comment. |
9f30c93 to
d7cea0b
Compare
|
I removed the expiry part after discussion in #rubygems-org. Manual update it too much of an ask and most implementations don't do this (Github access token, AWS etc). Changes in this PR:
Please let me know if it has further room for improvement. |
| private | ||
|
|
||
| def check_mfa(user) | ||
| if user&.mfa_api_authorized?(otp) |
There was a problem hiding this comment.
Consider tweaking so BaseController#verify_with_otp can be used here too, allowing this security-related code to be defined in a single place so vulnerabilities are less likely to slip through reviews.
There was a problem hiding this comment.
I had raised the same issue it was first introduced.
verify_with_otp requires @api_user set.
We could pass user as an optional argument to verify_with_otp, however, it would end up returning an incorrect response when authentication fails return if user&.mfa_api_authorized?(otp) (user being nil).
I am sure there is a way we can handle all case correctly, but it would be too complicated/risky to fix here. We can take this up in a separate PR.
| api_key = request.headers["Authorization"] || params.permit(:api_key).fetch(:api_key, "") | ||
| @api_user = User.find_by_api_key(api_key) | ||
| def find_api_key | ||
| params_key = request.headers["Authorization"] || params.permit(:api_key).fetch(:api_key, "") |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
25ff404 to
d8710d7
Compare
|
Thank you for review @eliotsykes ❤️ |
There was a problem hiding this comment.
Hi @sonalkr132, just one minor bit of feedback about the rake task. Thanks for being cool about the review, this is looking even better.
|
This is a great improvement, thank you! 🙌 IMO, we should provide a ~1-2 year deprecation period where the current RubyGems
What do you think? |
|
@sonalkr132 awesome, thank you! Is there an easy way we can test running |
These two are already part of the changes.
This is deployed on staging now. I tested with the following commands: |
| if @api_key.save | ||
| Mailer.delay.api_key_created(@api_key.id) | ||
|
|
||
| flash[:notice] = t(".save_key", key: key) |
There was a problem hiding this comment.
I tested this PR on my dev machine and didn't notice that the API key was in the notification bar (which IMO is not great already) until i clicked to go to another page.
Can we instead make a panel above the API keys that shows this message instead, and with the API key specifically highlighted/bold
| belongs_to :user | ||
| validates :user, :name, :hashed_key, presence: true | ||
| validate :exclusive_show_dashboard_scope, if: :show_dashboard? | ||
|
|
There was a problem hiding this comment.
Multiple API keys are able to have the same nickname, I think we should have a unique validation on name to avoid confusion for users and avoid accidentally deleting the wrong API key. Thoughts?
There was a problem hiding this comment.
Inforcing name uniqueness will mean the user will have to face some frustration when signing in from cli (where they can't see the existing names) and keep getting asked to enter email and password. Also, we would have to add random bits to the default key name we are suggesting in cli.
If you think these issues don't compare to what you are pointing out, we can add name uniqueness. I don't have any strong opinion about this, I was just leaning towards not making users fail their signin attempts.
|
@colby-swandale I have incorporated all of your UI suggestions. Please let me know if there is anything else. |
6a39b02 to
326df46
Compare
12d51df to
e2df5ec
Compare
e84961f to
87804e0
Compare
b6d599a to
620ca75
Compare
permissions
Our api key implementation had room for improvements, namely:
- set expiry time
- stored hashed token in DB instead of plain text
- support multiple api keys
- limit permission per api key
api token will be shown to user only once and will expire after 180
days. users can pick which permissions do they want to enable per
token.
Add controller and view for api keys
Verify api key authorization for api requests
Update existing test to use api keys model
deprecates api endpoint to get api keys. users can see api key
only once during creation.
removes related tests for get api keys and unyank endpoints.
Add i18n keys
Add tests for api keys
Add test for missing api key scope
Add rake task to migrate api keys
Update before action to use redirect_to_signin
Remove expires_at column
Remove unused i18n keys and api reset option
fix tests and add prefix to api keys
Rever changes to throttling test for api keys
Add create action to api keys endpoint
This will be used by new rubygems client when user runs:
`gem signin`. Older clients will continue to get `user.api_key`.
Add mail to api key creation
Update rack attack tests to use api_keys
Fix casing of API in i18n
Improve naming of methods used in api keys
Make show dashboard an exclusive scope
Increase API key byte length
Use seperate api_key_params method for api and ui endpoints. Rails
was getting confused about nested params being string when encoded
with `URI.encode_www_form` (in the client).
legacy API keys were not necessarily insecure with 16 bytes of
randomness. Colby recommended that it was a good time to increase the
byte length along with other changes.
Don't add show_dashboard scope to legacy-key
Add action to reset all API keys
Update GET api_key endpoint to create new key on each request
this would ensure that clients which can't migrate to using POST
endpoint continue to work as is and allow us not to store any
unercypted keys.
Use partial for compromised instructions
UI improvements
Changes mostly as per colby's suggestions on the PR.
Add method to udpate api_key and add validation on name
In cli we want to start with minimum possible permission and update
scope as they execute additional commands.
Add functional test for api_key update
Fix news tests added after rebase
Migrate css to existing owners table
Update method names to can prefix and add last access
This allows the user to create multiple API keys and set scope/permission per API key. The key will be shown to the user only once after creation.
todo