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

Add support for API key provider #919

Merged
merged 25 commits into from
Oct 3, 2022
Merged

Add support for API key provider #919

merged 25 commits into from
Oct 3, 2022

Conversation

nirinchev
Copy link
Member

@nirinchev nirinchev commented Sep 22, 2022

Adds support for API key provider and authentication.

  • Adds support for User.apiKeys. This is a client that exposes functionality for managing user API keys.
  • Adds support for Credentials.apiKey and Credentials.serverApiKey.
  • Exposes User.accessToken and User.refreshToken.

I had to downgrade Core due to realm/realm-core#5890.

Fixes #393
Fixes #385
Fixes #386
Fixes #387
Fixes #388
Fixes #389
Fixes #390
Fixes #435
Fixes #433
Fixes #432
Fixes #394

@cla-bot cla-bot bot added the cla: yes label Sep 22, 2022
src/realm_dart_sync.cpp Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Sep 22, 2022

Pull Request Test Coverage Report for Build 3161647487

  • 123 of 129 (95.35%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 87.175%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/src/app.dart 2 4 50.0%
lib/src/native/realm_core.dart 90 92 97.83%
lib/src/user.dart 29 31 93.55%
Totals Coverage Status
Change from base Build 3161270558: 0%
Covered Lines: 1910
Relevant Lines: 2191

💛 - Coveralls

@nirinchev nirinchev marked this pull request as ready for review September 23, 2022 00:38
@nirinchev nirinchev self-assigned this Sep 23, 2022
Copy link
Contributor

@nielsenko nielsenko left a comment

Choose a reason for hiding this comment

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

LGTM

test/app_test.dart Show resolved Hide resolved
lib/src/app.dart Outdated Show resolved Hide resolved
lib/src/native/realm_core.dart Outdated Show resolved Hide resolved
lib/src/native/realm_core.dart Outdated Show resolved Hide resolved
lib/src/user.dart Outdated Show resolved Hide resolved
lib/src/native/realm_core.dart Outdated Show resolved Hide resolved
lib/src/user.dart Outdated Show resolved Hide resolved
lib/src/user.dart Outdated Show resolved Hide resolved
lib/src/user.dart Outdated Show resolved Hide resolved
src/realm_dart_sync.cpp Outdated Show resolved Hide resolved
test/user_test.dart Show resolved Hide resolved
lib/src/user.dart Show resolved Hide resolved
src/realm_dart_sync.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@desistefanova desistefanova left a comment

Choose a reason for hiding this comment

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

Added some comments.
Changelog is missing.

lib/src/native/realm_core.dart Outdated Show resolved Hide resolved
lib/src/native/realm_core.dart Outdated Show resolved Hide resolved
Co-authored-by: Desislava Stefanova <95419820+desistefanova@users.noreply.github.com>
Copy link
Contributor

@desistefanova desistefanova left a comment

Choose a reason for hiding this comment

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

Some awaits were missing.

test/credentials_test.dart Outdated Show resolved Hide resolved
test/credentials_test.dart Outdated Show resolved Hide resolved
test/credentials_test.dart Outdated Show resolved Hide resolved
test/credentials_test.dart Outdated Show resolved Hide resolved
Co-authored-by: Desislava Stefanova <95419820+desistefanova@users.noreply.github.com>
test/user_test.dart Outdated Show resolved Hide resolved
test/user_test.dart Outdated Show resolved Hide resolved
Copy link
Contributor

@desistefanova desistefanova left a comment

Choose a reason for hiding this comment

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

I didn't find any test for serverApiKey. It would be nice to have. I am not sure if it is possible to generate any key. Maybe we can write a manual test, just to have it.

test/user_test.dart Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
nirinchev and others added 4 commits September 30, 2022 15:58
Co-authored-by: Desislava Stefanova <95419820+desistefanova@users.noreply.github.com>
# Conflicts:
#	src/realm_dart_sync.cpp
@nirinchev
Copy link
Member Author

nirinchev commented Sep 30, 2022

@desistefanova I've decided not to expose server API key since it's the same as user api key - see @ironage's comment here: realm/realm-core#5914 (comment). LMK if that makes sense to you. I can confirm that the server api key tests pass with the api key provider, so seems superficial indeed.

@blagoev I merged this with master, so it's no longer touching the native implementation.

@desistefanova
Copy link
Contributor

desistefanova commented Oct 1, 2022

@desistefanova I've decided not to expose server API key since it's the same as user api key - see @ironage's comment here: realm/realm-core#5914 (comment). LMK if that makes sense to you. I can confirm that the server api key tests pass with the api key provider, so seems superficial indeed.

@blagoev I merged this with master, so it's no longer touching the native implementation.

@nirinchev But it is possible in the Atlas web UI to create API key that doesn't belong to any user. How the developers will be able to use this key? See the documentation
https://www.mongodb.com/docs/atlas/app-services/authentication/api-key/

@nirinchev
Copy link
Member Author

They can use Credentials.apiKey - apparently there's no difference in the two credential types. You can see the last couple of tests in user_test.dart are logging in a user with a server API key.

Copy link
Contributor

@blagoev blagoev left a comment

Choose a reason for hiding this comment

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

Not sure about the expectLater calls. Should we always await the action or that is not mandatory?

test/user_test.dart Show resolved Hide resolved
test/user_test.dart Show resolved Hide resolved
test/user_test.dart Show resolved Hide resolved
test/user_test.dart Show resolved Hide resolved
test/user_test.dart Show resolved Hide resolved
@nirinchev nirinchev merged commit 126f1e1 into master Oct 3, 2022
@nirinchev nirinchev deleted the ni/api-keys branch October 3, 2022 12:46
@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.