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

Credentials.usernamePassword with multiple options #3698

Closed
cmelchior opened this issue Oct 26, 2016 · 6 comments
Closed

Credentials.usernamePassword with multiple options #3698

cmelchior opened this issue Oct 26, 2016 · 6 comments
Assignees
Milestone

Comments

@cmelchior
Copy link
Contributor

cmelchior commented Oct 26, 2016

Pr. internal discussion on API alignment on Sync API's, we need to supply a createAccountIfNeeded or createUser|useExistingAccount option.

// Current
Credentials.usernamePassword("username", "password", true);

// Last parameters should be a flag supporting the following:
// createUser, useExistingAccount and createUser|useExistingAccount ~ createUserIfNeeded
// Natural suggestion would either be enum or bitwise flags, but I think just having another constructor could be simpler:

Credentials.usernamePassword("user", "password"); // no boolean param = createifNeeded
Credentials.usernamePassword("user", "password", false); // Choose to create or not

// Alternatives
Credentials.usernamePassword("user", "password", Credentials.Options.CREATE_IF_NEEDED);
Credentials.usernamePassword("user", "password", Credentials.CREATE_ACCOUNT | Credentials.USE_EXISITING_ACCOUNT);

Thoughts @realm/java ?

@nhachicha
Copy link
Collaborator

Option 1

I think using the third parameter will allow to define different policies (I rule out the boolean option)

we can have the default method Credentials.usernamePassword("username", "password") which should try to sign-in an existing account. Then an overload that specify the different strategies ex:

  • Credentials.usernamePassword("username", "password", Credentials.CREATE_ACCOUNT) this will create (and sign-in) an account if the email address doesn't collision with an exisitng account
  • Credentials.usernamePassword("username", "password", Credentials.USE_EXISITING_ACCOUNT) equivalent to Credentials.usernamePassword("user", "password")
  • Credentials.usernamePassword("username", "password", Credentials.ABORT_IF_EXISTING)
    etc.

Option 2

The other option is to simply & distinguish between sign-in (using an existing user) or register

  • Sign-In should throw when the user doesn't exist. Credentials.signInWithUsernamePassword("user", "password");
  • Register should throw when there's a collision. Credentials.registerWithUsernamePassword("user", "password");

@cmelchior cmelchior modified the milestones: RMP 1.0, 2.0 Jan 3, 2017
@cmelchior
Copy link
Contributor Author

Primary purpose of this PR is making the API comparable with the Swift API.
SyncCredentials.accessToken() will be provided as part of #4005.

Tasks:

  • Compare current SyncCredentials with RLMSyncCredentials to see if we are missing anything
  • Add any missing options.
  • Add unit tests for those.

@bmeike
Copy link
Contributor

bmeike commented Jan 10, 2017

It looks to me as if the biggest difference is that Java doesn't support CloudKit authentication. After that there are some minor discrepancies:

  • iOS exposes the constructor. The Java constructor is private but a static constructor with identical params, delegates to it. The Java params are mis-ordered.
  • iOS exposes accessToken which creates a SyncCredentials with data in the Info map
  • password authentication would be identical if we overloaded the current Java method with one that had only two args and delegated to the existing method with the third arg "false"

Please advise.

@cmelchior
Copy link
Contributor Author

#3698 (comment) is out-dated at this point, so the solution sketched there is not relevant anymore.

  1. We don't want to expose the constructor, all object creation should go through the static constructors.

  2. I'm adding accessToken as part of another PR as it is needed to do integration tests.

  3. We should add the variant of usernamePassword with only two args + unit tests

  4. The order of params in private methods doesn't matter, but we should probably align them for the public methods, unless it feels very out of place on Android.

  5. I intentionally left out Cloudkit when originally adding the IdentityProviders. Mostly because I wasn't sure how an Android device would acquire such a token. However, a quick investigation shows that it looks possible to use Cloudkit WebServices to log into it from an Android Device, so we should probably add it just for completeness.

@bmeike
Copy link
Contributor

bmeike commented Jan 10, 2017

Ok. Sounds good. I will, exactly:

  1. change the order of args for the custom method
  2. add the overloaded 2 arg usernamePassword method (note: this is option 3: it is not either of the 2 discussed above)
  3. add unit tests as appropriate
  4. conditionally: SyncCredentials.IdentityProvider begs to be an enum. I will convert it, if folks are ok with that and it is not too disruptive.

@bmeike
Copy link
Contributor

bmeike commented Jan 10, 2017

Upon examination, the existence of a custom authentication type suggests that there are not a known set of IdentityProviders and that IdentityProviders should not, therefore, be represented with an enum. Abandoning #4.

@bmeike bmeike closed this as completed in 7482e99 Jan 12, 2017
@bmeike bmeike removed the S:Review label Jan 12, 2017
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants