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.

Sign up for GitHub

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

Jump to bottom

Redesign user auth #2002

Merged
merged 9 commits into from
Sep 13, 2018
Merged

Redesign user auth #2002

merged 9 commits into from
Sep 13, 2018

Conversation

nirinchev
Copy link
Member

@nirinchev nirinchev commented Aug 31, 2018 •
edited
Loading

What, How & Why?

This aligns the authentication API with the other SDKs.

☑️ ToDos

  • 📝 Changelog entry
  • 📝 Compatibility label is updated or copied from previous entry
  • 🚦 Tests
  • 📝 Public documentation PR created or is not necessary
  • 💥 Breaking label has been applied or is not necessary

If this PR adds or changes public API's:

  • typescript definitions file is updated
  • jsdoc files updated
  • Chrome debug API is updated if API is available on React Native

@nirinchev nirinchev requested a review from kneth August 31, 2018 12:20
@realm-probot
Copy link

realm-probot bot commented Aug 31, 2018

Hey - looks like you forgot to add a T:* label - could you please add one? 👍

kraenhansen
kraenhansen approved these changes Aug 31, 2018
View reviewed changes
Copy link
Member

@kraenhansen kraenhansen left a comment

Choose a reason for hiding this comment

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

I think this looks good and valuable.

lib/user-methods.js Outdated Show resolved Hide resolved
lib/user-methods.js Outdated
},

jwt(token) {
return new Credentials('jwt', token);
Copy link
Member

Choose a reason for hiding this comment

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

Should this take an optional provider too?

kneth
kneth reviewed Aug 31, 2018
View reviewed changes
docs/sync.js Outdated Show resolved Hide resolved
@kneth
Copy link
Contributor

kneth commented Aug 31, 2018

@nirinchev I have connected your PR with #1579 which I believe is the correct issue. To me, it is important that we get something which is as close to our other SDKs as possible.

@nirinchev
Copy link
Member Author

I agree - this very closely mirrors the .NET API, which if I remember correctly is aligned with Cocoa and Java (maybe with a few casing differences). @cmelchior can you do a high level review and see if the new API diverges in significant ways from Java?

cmelchior
cmelchior reviewed Sep 3, 2018
View reviewed changes
lib/index.d.ts Outdated Show resolved Hide resolved
cmelchior
cmelchior reviewed Sep 3, 2018
View reviewed changes
lib/index.d.ts Outdated Show resolved Hide resolved
cmelchior
cmelchior reviewed Sep 3, 2018
View reviewed changes
Copy link
Contributor

@cmelchior cmelchior left a comment

Choose a reason for hiding this comment

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

There are a few naming differences compared to Java, but the API seems to work the same way. To be honest, I like the names you picked more, and Swift uses the same names you picked here. I'm guessing Java's names are a leftover from an old design that was never refactored and is a (poor) attempt at papering over the difference between a token and a username.

@bmunkholm
Copy link
Contributor

bmunkholm commented Sep 3, 2018 •
edited
Loading

@nirinchev The "pipeline" in zenhub tracks the state - doing that in the subject with "[WIP]" is redundant and not a scheme that is used by others either, so I've removed "[WIP]" :-)

Other than that nitpick - looks good!

@bmunkholm bmunkholm changed the title [WIP] Refactor user auth Refactor user auth Sep 3, 2018
@bmunkholm bmunkholm changed the title Refactor user auth Redesign user auth Sep 3, 2018
kneth
kneth approved these changes Sep 6, 2018
View reviewed changes
CHANGELOG.md Outdated
@@ -7,7 +7,23 @@ X.Y.Z Release notes
* Realm Object Server: 3.0.0 or later

### Breaking changes
* None.
* The authentication API have been completely revamped.
Copy link
Contributor

Choose a reason for hiding this comment

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

"have" -> "has"

Copy link
Member Author

Choose a reason for hiding this comment

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

I was using the plural version of API 🤓

CHANGELOG.md Outdated Show resolved Hide resolved
docs/sync.js Outdated

/**
* Creates credentials based on a JWT login.
* @param {string} token A Json Web Token, that will be validated against the server's configured rules.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Json" -> "JSON"

@kneth
Copy link
Contributor

kneth commented Sep 6, 2018 •
edited
Loading

The CI errors are The @deprecated, tag is not a known tag. File: sync.js. It appears that a , is not allowed.

@kneth kneth merged commit 80cde7a into master Sep 13, 2018
@kneth kneth deleted the ni/auth branch September 13, 2018 06:45
@kneth kneth mentioned this pull request Sep 21, 2018
Closed
@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.
Reviewers

@cmelchior cmelchior cmelchior left review comments

@kraenhansen kraenhansen kraenhansen approved these changes

@kneth kneth kneth approved these changes

Assignees

@nirinchev nirinchev

Labels
Breaking T-Enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nirinchev @kneth @bmunkholm @cmelchior @kraenhansen @realm-ci