-
Notifications
You must be signed in to change notification settings - Fork 80
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
Test Custom JWT Authentication to Credentials #715
Conversation
…to ds/jwt_tests
…to ds/jwt_tests
…to ds/jwt_tests
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.
My comments roughly reflect what we discussed at the roundtable yesterday - the code seems fine to me, but I believe we should simplify our tests.
/// "aud": "mongodb.com", | ||
/// "iss": "https://realm.io" | ||
/// } | ||
baasTest('JWT - login with existing user and edit profile', (configuration) async { |
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 test can't work like this. Users are distinguished by provider+providerId, so an email password user cannot authenticate with JWT, even if their identities are the same. What you can do is generate two tokens for the same user identity and different profile information. Then validate that both authenticate the same user and that after the second auth, the profile has been updated.
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.
I'm just rewriting this test with having one user with two identities for different providers.
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.
Does it make sense now?
Co-authored-by: Nikola Irinchev <irinchev@me.com>
Co-authored-by: Nikola Irinchev <irinchev@me.com>
Co-authored-by: Nikola Irinchev <irinchev@me.com>
…to ds/jwt_tests
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.
Looks good, let's just clean up/simplify that profile test.
final username = "jwt_user@#r@D@realm.io"; | ||
final authProvider = EmailPasswordAuthProvider(app); | ||
// Always register jwt_user@#r@D@realm.io as a new user. | ||
try { | ||
await authProvider.registerUser(username, strongPassword); | ||
} on RealmException catch (e) { | ||
{ | ||
if (e.message.contains("name already in use")) { | ||
// If the user exists, delete it and register a new one with the same name and empty profile | ||
final user1 = await loginWithRetry(app, Credentials.emailPassword(username, strongPassword)); | ||
await app.deleteUser(user1); | ||
await authProvider.registerUser(username, strongPassword); | ||
} | ||
} |
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.
Why hardcode the username here? We should be able to use a random email and avoid that extra logic.
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.
Because the token is already generated and it must contain the same username inside.
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 logic gets complicated but it will allow us to run the same test many times.
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.
I tried to generate a new username for email/password credentials and then to link to JWT credentials. The email and username of the user are updated with "jwt_user@#r@D@realm.io" that is coming from the token. If I run the test again it creates a new user, but when I try to link it to the credentials with the same token it fails, because "jwt_user@#r@D@realm.io" already exists and it can not rename the new user to the same name. I still have to delete "jwt_user@#r@D@realm.io" before to run the test again.
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, the issue is that the token is already consumed to create a user. The next time the test runs, it tries to link the new user to an existing user which is forbidden.
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.
Its all good, but I have some small suggestions before merging this
test/data/jwt_keys/README.md
Outdated
@@ -0,0 +1,57 @@ | |||
Custom JWT Authentication | |||
|
|||
The keys in this folder "test/data/jwt_keys" are used only for the purpose of the automatic tests. |
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.
Lets add these files to the .pubignore file. We will never need to publish them right?
Lets ignore the whole directory jwt_keys
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.
Same for the flutter package
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.
While here we should do this for the data/realm_files dir as well.
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.
I deleted the whole folder jwt_keys and moved the REAMDME.md content into test\README.md.
About the realm file into data/realm_files we already have in .pubignore an exclude for the file **/*.realm
but I will also add an exclude for the whole data folder.
test/data/jwt_keys/private_key.pem
Outdated
@@ -0,0 +1,27 @@ | |||
-----BEGIN RSA PRIVATE 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.
If these are not needed lets remove them. Right now they have not polluted the main branch so if we delete them they will not get into the master history
Co-authored-by: Nikola Irinchev <irinchev@me.com>
…to ds/jwt_tests
Co-authored-by: blagoev <lubo@blagoev.com>
Update Changelog.
Add tests for login with JWT credentials.
Enable custom-token auth provider for the cloud apps using baas_client.dart.
Fixes: #398
This supersedes #709