Add isAdmin flag to RLMSyncUser
#4699
Conversation
austinzheng
force-pushed
the
az/add-admin-check
branch
2 times, most recently
from
168b0ff
to
40d2ede
Compare
|
I've manually verified that this works, but without a way to programmatically create admin vs non-admin users we can't write a test for it. I've added a FIXME; maybe this is simple enough that we can put off writing the test. |
Realm/RLMSyncUser.mm
Outdated
| @@ -253,6 +254,7 @@ + (void)_performLogInForUser:(RLMSyncUser *)user | |||
| return; | |||
| } | |||
| user->_user = sync_user; | |||
| user.isAdmin = model.refreshToken.tokenData.isAdmin; | |||
There was a problem hiding this comment.
Is this state that should be tracked on object store's SyncUser class? I see that it already has SyncUser::is_admin(), which represents something different than this flag.
There was a problem hiding this comment.
Yes. There are actually two issues here:
- One is that, as you said, the flag should be tracked on the object store class instead. If the user fetches another
RLMSyncUserobject pointing to the same user, it won't have the proper flag set. - The other is that the terminology is really confusing. We have the notion of a "client side admin user", which is a user who uses the admin token to open Realms, and a "server side admin user", which is a user flagged by ROS as an 'admin'. Perhaps we should rename the public API to "isObjectServerAdmin" or something similar.
There was a problem hiding this comment.
ISTM that the concepts are really:
- A user can be an admin or a normal user. Distinguishing those is what this PR is about.
- The initial admin user (I think? Or is it more than just the initial user?) can authenticate via a special access token, bypassing the normal authentication flow. This is what
SyncUser'sm_is_admincurrently tracks. This concept should probably be renamed to make it clear that it's about how the authentication is handled.
austinzheng
force-pushed
the
az/add-admin-check
branch
4 times, most recently
from
d07ddc3
to
7a0ddbd
Compare
|
I rebased and merged onto latest master. This can probably be reviewed now. One thing I noticed was that git's conflict resolution algorithm might move changelog entries to random places in the changelog without complaining about merge conflicts. Something to watch out for (I suspect this may have happened to me in the past). |
Changes: - Sync users now expose a property indicating whether or not they are Realm Object Server administrator users - Updated object store pointer
austinzheng
force-pushed
the
az/add-admin-check
branch
from
24606a9
to
768d153
Compare
Realm/RLMSyncUser.h
Outdated
| @@ -70,6 +70,11 @@ NS_ASSUME_NONNULL_BEGIN | |||
| @property (nullable, nonatomic, readonly) NSURL *authenticationServer; | |||
|
|
|||
| /** | |||
| Whether the user is an Realm Object Server administrator user. | |||
There was a problem hiding this comment.
The extra "user" at the end feels redundant.
CHANGELOG.md
Outdated
|
|
||
| ### Bugfixes | ||
|
|
||
| * None. | ||
| * Fix an issue where the project would fail to compile with clang 4.0. |
There was a problem hiding this comment.
This isn't relevant to Cocoa at the moment since no Xcode version exists that uses Clang 4.0.
|
Object server tests pass and I verified things work manually as well. I'll merge it when CI finishes. |
Changes:
Requires: realm/realm-object-store#396
To do: