Skip to content
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

Fix the json arguments of SyncUser.requestEmailConfirmation #5953

Merged
merged 3 commits into from
Oct 12, 2018

Conversation

nirinchev
Copy link
Member

@nirinchev nirinchev commented Oct 12, 2018

The provider_id should have been outside the data argument, which had caused ROS to silently ignore the request (as it was trying too hard to ensure that an attacker cannot figure out if an email is registered with the server).

Fixes https://github.com/realm/realm-object-server-private/issues/1283.

@tgoyne
Copy link
Member

tgoyne commented Oct 12, 2018

There's a test for this which passes. Is the test wrong, or is this just updating things to work with a newer ROS version that broke existing clients? If it's the latter, the changelog needs to mention that version.

@@ -371,8 +371,7 @@ + (void)requestEmailConfirmationForAuthServer:(NSURL *)serverURL
userEmail:(NSString *)email
completion:(RLMPasswordChangeStatusBlock)completion {
[RLMSyncUpdateAccountEndpoint sendRequestToServer:serverURL
JSON:@{@"data": @{@"provider_id": email,
@"action": @"request_email_confirmation"}}
JSON:@{@"provider_id": email, @"data": @{ @"action": @"request_email_confirmation"}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

excessive whitespace

@nirinchev
Copy link
Member Author

The ROS API has never changed, so there must be something wrong with the test. Unfortunately, I'm completely unfamiliar with the test setup in Cocoa, so it's very hard for me to understand what the test does - essentially, calling this API should generate a token and send it as an email at the ROS level - it's not clear to me if the Cocoa test framework has made any modifications to the ROS codebase to allow intercepting this token and passing it back to the test framework.

@tgoyne
Copy link
Member

tgoyne commented Oct 12, 2018

The test server script passes ROS a custom email handler which just write the token to a file. The test calls requestEmailConfirmation, reads the token from the file, and then passes it to confirmEmail. This all succeeds, which indicates that the current request made to ROS results in ROS invoking the email handler with a valid token. There's another test which calls requestEmailConfirmation with a user that doesn't exist and verifies that the email handler is not invoked.

@nirinchev
Copy link
Member Author

Ah... I think I know what' happening! ROS automatically "sends" an email confirmation email when the user registers, so I believe this is what the test is seeing and mistakenly believes that this was generated due to calling that method, when in reality it was there all along. I can modify the test slightly to assert that a token was generated immediately after logging the user in, then that a different one is generated after calling that method.

@nirinchev
Copy link
Member Author

@tgoyne I confirmed that this is the case. I also confirmed that the modified test fails without this fix - it fails on this line: https://github.com/realm/realm-cocoa/pull/5953/files#diff-c64398ac8b77c37143815b54e83e9427R497, because calling [self emailForAddress:userName] immediately after creating the user removes the token generated by ROS automatically.

@nirinchev
Copy link
Member Author

The failing tests seem to be unrelated to the changes in this PR, so I'll go ahead and merge it 🤞

@nirinchev nirinchev merged commit 57394d1 into master Oct 12, 2018
@nirinchev nirinchev deleted the ni/request-email-confirmation branch October 12, 2018 22:38
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants