Add system authentication for use with API clients #1522
Conversation
lagoan
changed the title
lagoan
changed the title
lagoan
changed the title
Extract basic functionality from omniauth-identity for system authentication. This simple implementation assumes system accounts will be created by a rake task.
Tests wheather system accounts can log in and if accounts with other identities can create a system identity
Local system accounts will be sendomly used. Their purpose right now is solely for API clients and their creation should not be open to public, for this reason the accounts are created on server side.
Move password information to User model Change mentions of api to system Remove unused gem highline Add migration to add system user if it is not found by email
After adding system accounts for use with APIs we need to check if these accounts are actually accepted by the entities policies
As we add a system user from a migration we need to assume the user may already exist. This approch will provide a new/existing user a password the application is expectin. On a migration removal we don't do anything in case the system user account was in fact there.
The User model has special requirements for passwords. A user can be valid without a password when loggin in with omniauth. In case the User logs in with a password the password needs to be confirmed by instantiating the User with 2 password inputs.
lagoan
force-pushed
the
feature/basic-authentication-simple
branch
from
8f4f23c
to
4d62e32
Compare
|
Rewrote history to remove rebase/merge problems. |
There was a problem hiding this comment.
Looks good overall, I just have a couple of notes on maybe opening up the version pinning on bcrypt, and some general readability/safety concerns around the terminology "password" and the elimination of the system boolean column.
One thing I think needs to be added to the PR is modification to SessionsController similar to https://github.com/ualbertalib/jupiter/blob/integration_postmigration/app/controllers/sessions_controller.rb#L32 to reject any SAML login targeting an account with the 'system' boolean set.
+ Rename password_digest to api_key_digest + Open up the version pinning for bcrypt + Validate User only when both or neither system and api_key_digest are present
app/models/user.rb
Outdated
|
|
||
| # Check that the api key is not present if it is not a system account | ||
| validates :api_key_digest, | ||
| absence: { message: 'must be blank if System value is false' }, |
There was a problem hiding this comment.
Looks pretty good!
Minor, but Perhaps move these strings to I18n (as they could be potentially front facing to users)?
| api_key: '70d800e9-5fe8-49e4-86ed-eefc11ebfa52' | ||
| ) | ||
|
|
||
| assert_not user.valid? |
There was a problem hiding this comment.
Maybe just enhance these tests to double check the error message coming back and its correct?
e.g:
assert_equal user.errors[:api_key_digest].first, "can't be blank"
There was a problem hiding this comment.
@murny In your example you included the string in English. I see the benefits to this. However, would it be problematic when testing with different locales ?
There was a problem hiding this comment.
Yeah was just an example. Since these are custom error messages and from my other comment, it be nice if they are defined in I18n, you can reuse that for your test. (So that if we ever change it, its changed in both places)
Example from a real test in jupiter:
assert_includes collection.errors[:community_id],
I18n.t('activerecord.errors.models.collection.attributes.community_id.community_not_found',
id: community_id)
So assert_equal user.errors[:api_key_digest].first, I18n.t('path.to.locale.message')
There was a problem hiding this comment.
Testing with locales is a bit ... tricky. But you can assume the tests run in the English locale.
The benefit of testing for the message is that you avoid a false positive where the user isn't valid, but it's not because of the test you expect, but because of some unrelated validation that just happens to have failed.
As for hardcoding the message in the test versus an i19n lookup – the school of thought I subscribe to is that the tests should look for the concrete expectation, not duplicate the logic of the code you're testing. So if you're testing a method that does some math, the test should compare the result to an expected hardcoded number, not a math expression. Similar idea with things like this, we compare to a hardcoded string instead of comparing to lookup logic.
There was a problem hiding this comment.
@murny would you do that? to my mind you're just introducing a potential copy paste error on the i18n key. Doesn't that test just check that rails is sane?
There was a problem hiding this comment.
I can see it both ways. I'm okay with either way personally (In my own experience I typically hardcode the model errors but reuse I18n for system tests/controller tests (e.g: click on this Link (name comes from I18n instead of hardcoding it). No particular reason why I do this. I think It's probably view code changes far far far more then model code and model code you really really want to have everything locked down). So doesn't matter to me, I see pros and cons of both.
You are correct though if we hardcode the expected message in tests this makes like you said, guaranteed and prevents false positives. Like if we accidentally edit i18n, then you would never catch that. But if you explicitly said the error message, then you would have a failing test to catch that. Plus its more self documenting... Reading my example I18n.t('activerecord.errors.models.collection.attributes.community_id.community_not_found', id: community_id) It's not super clear what this error message is in this context of the test. You would have to go to the en.yml and read it to find out what it actually is. If you explicitly hard code it then its right there.
My counter argument for if it just checks if rails is sane...I think its a bit different in this case, as we still checking that hey...this string from i18n is being displayed when this validation fires. So we still checking something more then just if ruby/rails did what its suppose to. (testing if a = b then assert a == b is what I consider just checking rails (i guess ruby in this case) is sane).
Whatever the team wants to do I'm okay. Think the biggest thing is just remaining consistent with the approach we choose throughout the test suite. (Sounds like we want to hardcode this and that's okay!)
And tests will always run in the default locale (English in our case) so this isn't a worry
There was a problem hiding this comment.
Do you know if Rails will raise an exception if a translation is missing in test mode? I'm wondering if tests along the lines of assert_equal user.errors[:api_key_digest].first, I18n.t('path.to.locale.message') could false positive with "translation missing" == "translation missing"
There was a problem hiding this comment.
Yep, good catch. We have enabled this in environments/test.rb
https://github.com/ualbertalib/jupiter/blob/integration_postmigration/config/environments/test.rb#L48-L49
By default its turned off (which would give exactly what your example would give 😆 )
There was a problem hiding this comment.
Eh, I'm probably fine with continuing this pattern, then. Like you said, we're not quite duplicating the same logic, and it is a bit less fragile than hardcoding when random wording changes come in.
Check system users so they dont log in with omniauth. All system accounts should only be able to log in through system authentication; this is true for new accounts and old accounts that were changed to system accounts.
The validation error messages for the attribute api_key digest may become front facing so they need to be able to be localized.
User api_key_digest validation errors return I18n messages, we should check if they are indeed what we are expecting.
…o feature/basic-authentication-simple
Validation errors for the User api_key_digest attribute validation will use values from I18n verifying the text on the defined there is actually\ what we are receiving.
| @@ -29,6 +29,9 @@ def create | |||
| user.identities.create(provider: auth_hash.provider, uid: auth_hash.uid) | |||
| end | |||
|
|
|||
| # System user accounts should not be able to access application through | |||
| # omniauth authentication | |||
| # | | | True | | ||
| # | X | | False | | ||
| # | | X | False | | ||
| # ---------------------------- |
There was a problem hiding this comment.
Nice, I really like the ASCII truth table for making this understandable.
|
|
Context
Jupiter will provide a way to access its APIs to clients using accounts with special privileges. For now these accounts will only use local system authentication to simplify things. This PR addresses #1480
Changes
Ongoing work
apitosystem