Permissions api #1244

kristiandupont · 2017-08-24T07:44:47Z

What, How & Why?

This creates an abstraction on top of the permissions system, making it simpler to apply, query and offer permissions.

Reference to the issue(s) addressed by this PR:

This fixes #1204

☑️ ToDos

📝 Changelog 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

bigfish24 · 2017-08-26T06:39:39Z

How much is remaining?

kristiandupont · 2017-08-28T08:13:48Z

@bigfish24 it's done, save for some documentation. At least I think so -- I am having problems with the tests on my machine -- I hope I will have it done by today.

kneth

Looks good to me. But I only see tests for read permissions. Don't we need to test that write implies read, etc.?

kneth · 2017-08-29T07:05:47Z

CHANGELOG.md

@@ -8,6 +8,7 @@ X.Y.Z Release notes
 * Added property `Realm.isInTransaction` which indicates if write transaction is in progress.
 * Added `shouldCompactOnLaunch` to configuration (#507).
 * Added `Realm.compact()` for manually compacting Realm files.
+* Added various methods for permission management (#1204)


. missing

kneth · 2017-08-29T07:17:12Z

docs/sync.js

+     * @param {string} recipient the optional recipient of the permission. Can be either
+     * 'any' which is the default, or 'currentUser' or 'otherUser' if you want only permissions
+     * belonging to the user or *not* belonging to the user.
+     * @returns {Results} a queryable collection of permission objects that provide detailed 


"provide" -> "provides"?

kneth · 2017-08-29T07:19:43Z

docs/sync.js

+     * Changes the permissions of a Realm.
+     * @param {string} condition - A condition that will be used to match existing users against.
+     * This should be an object, containing either the key 'userId', or 'metadataKey' and 'metadataValue'.
+     * @param {string} realmUrl - The path to the realm that you want to apply permissions to.


"realm" -> "Realm"

kneth · 2017-08-29T07:27:55Z

docs/sync.js

+     * @param {string} realmUrl - The path to the realm that you want to apply permissions to.
+     * @param {string} accessLevel - The access level you want to set: 'none', 'read', 'write' or 'admin'.
+     * @returns {Promise} a Promise that, upon completion, indicates that the permissions have been 
+     * successfully applied by the server. It will be resolved with the PermissionChange object that refers


"PermissionChange" - why not crossreference using @link?

kneth · 2017-08-29T07:43:14Z

docs/sync.js

+     * @param {string} realmUrl - The Realm URL whose permissions settings should be changed. Use * to change 
+     * the permissions of all Realms managed by this user.
+     * @param {string} accessLevel - The access level to grant matching users. Note that the access level 
+     * setting is absolute, i.e. it may revoke permissions for users that previously had a higher access level. 


"that" -> "who"

kneth · 2017-08-29T07:51:53Z

lib/permission-api.js

+
+      // FIXME: I don't understand why, but removing the following setTimeout causes the subsequent
+      // setTimeout call (when resolving the promise) to hang on RN iOS. 
+      // This might be related to our general makeCallback issues.


Do we have a Github issue?

kneth · 2017-08-29T08:03:45Z

lib/permission-api.js

+      return Promise.reject(new Error('realmUrl must be specified'));
+    }
+
+    if (['none', 'read', 'write', 'admin'].indexOf(accessLevel) === -1) {


Declare as constant validAccessLevels and do validAccessLevels.join(',') in error message?

kneth · 2017-08-29T08:06:28Z

tests/js/permission-tests.js

@@ -0,0 +1,114 @@
+////////////////////////////////////////////////////////////////////////////
+//
+// Copyright 2016 Realm Inc.


"2016" -> "2017"

kneth · 2017-08-29T08:07:10Z

tests/js/permission-tests.js

+var Realm = require('realm');
+var TestCase = require('./asserts');
+
+function uuid() {


Should we promote it to a general utility method?

kneth · 2017-08-29T08:07:36Z

tests/js/permission-tests.js

+    Realm.Sync.User.register('http://localhost:9080', username, 'password', (error, user) => {
+      if (error) {
+        reject(error);
+      } else {


Line break after }?

nirinchev

Looks good. Probably needs a few more tests, but those can be added later on.

nirinchev · 2017-08-29T09:36:10Z

docs/sync.js

+
+    /**
+     * Changes the permissions of a Realm.
+     * @param {string} condition - A condition that will be used to match existing users against.


Should this be {object} rather than {string}?

nirinchev · 2017-08-29T09:39:06Z

docs/sync.js

+     * @param {string} realmUrl - The Realm URL whose permissions settings should be changed. Use * to change 
+     * the permissions of all Realms managed by this user.
+     * @param {string} accessLevel - The access level to grant matching users. Note that the access level 
+     * setting is absolute, i.e. it may revoke permissions for users that previously had a higher access level. 


My understanding is that the offerPermissions access level setting is complimentary rather than absolute.

This is copied from your documentation of the .NET equivalent: https://github.com/realm/realm-dotnet/blob/master/Realm/Realm.Sync/User.cs#L405

Oh, shoot. Filed an issue to fix it: realm/realm-dotnet#1540

nirinchev · 2017-08-29T09:39:58Z

docs/sync.js

+    offerPermissions(realmUrl, accessLevel, expiresAt) { }
+
+    /**
+     * Consumes a token generated by <see cref="OfferPermissions"/> to obtain permissions to a shared Realm.


Is the cref correct?

nirinchev · 2017-08-29T09:46:46Z

lib/permission-api.js

+const specialPurposeRealmsKey = '_specialPurposeRealms';
+
+function getSpecialPurposeRealm(user, realmName, schema) {
+  if (!user.hasOwnProperty(specialPurposeRealmsKey)) {


Perhaps there should be a comment explaining why we need to cache those realms (it's not clear to me and probably other people will wonder as well).

nirinchev · 2017-08-29T09:48:33Z

lib/permission-api.js

+              else {
+                const e = new Error(o.statusMessage);
+                e.statusCode = o.statusCode;
+                e.manamgementObject = o;


typo - manamgementObject

nirinchev · 2017-08-29T09:48:39Z

lib/permission-api.js

+            }
+          }
+
+          managementRealm.addListener('change', listener);


Doesn't js support object level notifications?

Not yet: #763

blagoev · 2017-08-29T12:23:17Z

lib/permission-api.js

+  const _Realm = user.constructor._realmConstructor;
+
+  return new Promise((resolve, reject) => {
+    _Realm._waitForDownload(config, (error) => {


why not use Realm.open. Seems this is duplicating the same logic

I guess I could, it's just that it hasn't been added at this point, so I would have to reference the function directly through extensions.js somehow. Seemed hacky anyway.

got it. 10x

kneth · 2017-08-29T13:19:54Z

@kristiandupont You can merge it if you create an issue about the failing tests on Chrome debugger.

kristiandupont · 2017-08-29T13:29:26Z

@kneth done: #1260

rodrigopivi · 2017-09-07T15:20:48Z

this change broke typescript definitions, TS will throw this error when importing realm now:

node_modules/realm/lib/index.d.ts(275,88): error TS2304: Cannot find name 'Permission'.
node_modules/realm/lib/index.d.ts(283,32): error TS2304: Cannot find name 'userId'.
node_modules/realm/lib/index.d.ts(284,32): error TS2304: Cannot find name 'metadataKey'.
node_modules/realm/lib/index.d.ts(284,45): error TS2374: Duplicate string index signature.
node_modules/realm/lib/index.d.ts(284,68): error TS2304: Cannot find name 'metadataValue'.

Issue: #1283

kneth · 2017-09-08T07:02:38Z

@rodrigopivi And #1285 should fix it.

kristiandupont added the T-Enhancement label Aug 24, 2017

kristiandupont self-assigned this Aug 24, 2017

kristiandupont added 14 commits August 25, 2017 11:41

Add basic permissions skeleton

585ffce

...

2d5e106

Update permissions api

85a03d2

Wait for server to process management realm, add offer api

4ff7ef9

Fix test of apply and get permissions, add offer test

6aa4849

Accept permission offer

c1e032f

Accept permission offer test

35d52c3

Invalidate permission offer

8234492

Add basic docs (still need some links etc)

d31a138

Refactor tests

c7cf3bf

Only run permission tests where sync is enabled

0816d21

Use legal syntax for user name generation

ffe864e

Add changelog entry

c347383

Streamline permission tests

c70c5e3

kristiandupont force-pushed the kd/permissions-api branch from cbed574 to c70c5e3 Compare August 25, 2017 13:01

kristiandupont added 2 commits August 25, 2017 15:47

Fix casing for access level names etc.

48d5391

Add basic definitions to index.d.ts

bd16430

kristiandupont added 4 commits August 28, 2017 13:52

Use settimeout for resolving promise from listener

007882f

Complete typescript defs

1c96664

Improve docs

cd0b895

Allow 'any' as default recipient in getGrantedPermissions

7785000

kristiandupont requested review from fealebenpae, kneth and blagoev August 28, 2017 12:49

fealebenpae requested a review from nirinchev August 28, 2017 12:57

Fix getSpecialPurposeRealm on iOS

497c07a

kneth reviewed Aug 29, 2017

View reviewed changes

bmunkholm added T-Feature and removed T-Enhancement labels Aug 29, 2017

nirinchev approved these changes Aug 29, 2017

View reviewed changes

kristiandupont added 4 commits August 29, 2017 13:31

Response to PR comments

c30b0ec

Respond to PR comments

1a7c771

Fix offer description

73701f8

Skip permission tests in chrome debugger :-/

42c381b

blagoev reviewed Aug 29, 2017

View reviewed changes

kristiandupont merged commit 402bf48 into master Aug 29, 2017

blagoev deleted the kd/permissions-api branch September 25, 2017 12:18

github-actions bot locked as resolved and limited conversation to collaborators Mar 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Permissions api #1244

Permissions api #1244

kristiandupont commented Aug 24, 2017 •

edited

Loading

bigfish24 commented Aug 26, 2017

kristiandupont commented Aug 28, 2017

kneth left a comment

kneth Aug 29, 2017

kneth Aug 29, 2017

kneth Aug 29, 2017

kneth Aug 29, 2017

kneth Aug 29, 2017

kneth Aug 29, 2017

kneth Aug 29, 2017

kneth Aug 29, 2017

kneth Aug 29, 2017

kneth Aug 29, 2017

nirinchev left a comment

nirinchev Aug 29, 2017

nirinchev Aug 29, 2017

kristiandupont Aug 29, 2017

nirinchev Aug 29, 2017

nirinchev Aug 29, 2017

nirinchev Aug 29, 2017

nirinchev Aug 29, 2017

nirinchev Aug 29, 2017

kneth Aug 29, 2017

kristiandupont Aug 29, 2017

nirinchev Aug 29, 2017

blagoev Aug 29, 2017

kristiandupont Aug 29, 2017

blagoev Aug 29, 2017

kneth commented Aug 29, 2017

kristiandupont commented Aug 29, 2017

rodrigopivi commented Sep 7, 2017

kneth commented Sep 8, 2017

Permissions api #1244

Permissions api #1244

Conversation

kristiandupont commented Aug 24, 2017 • edited Loading

What, How & Why?

☑️ ToDos

bigfish24 commented Aug 26, 2017

kristiandupont commented Aug 28, 2017

kneth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nirinchev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kneth commented Aug 29, 2017

kristiandupont commented Aug 29, 2017

rodrigopivi commented Sep 7, 2017

kneth commented Sep 8, 2017

kristiandupont commented Aug 24, 2017 •

edited

Loading