Review ACL Settings

[cheungpat]

I will draft a follow-up with the following information:

• Existing behavior (on master) across SDKs and Server
  • client-side default ACL settings
  • server-side default ACL settings
  • access control setting for newly created record with record ACL settings untouched
  • access control setting for newly created record with record ACL settings untouched after saving to server
  • access control setting when fetched record from server with _access being null
  • if fetching record from server and returned _access=null, what the client side assume is the effective ACL setting
  • access control setting for newly created record with record ACL settings modified
  • access control setting for existing record with record ACL settings modified
  • behavior when saving records to database directly (using SQL)
• Desired/proposed behavior

My proposed behavior:

• client side does not remember default ACL settings
• client side is able to save default ACL settings to the server and fetch it from the server
• null is always treated as public-read, null is also the state if the ACL setting is not explicitly modified
• client may send null to server, sending null to server signify that the server will apply default ACL settings
• when modifying record ACL and the ACL has null setting, the developer will modify the setting as if the ACL is public-read.

Rick’s proposed behavior:

• client remembers the default ACL settings
• client side is able to save default ACL settings to the server and fetch it from the server
• null in the DB is treated as public-read
• client may send null to server, sending null to server signify that the server will apply default ACL settings
• when modifying record ACL and the ACL has null setting, the developer will modify the setting as if the ACL is the default ACL settings.

Ben L’s proposed behavior:

• client remembers the default ACL settings
• client side will not save default ACL settings to the server
• null (in database SQL) is always treated as public-read
• client will never send null to server (will send default ACL settings)
• when modifying record ACL, the developer will modify the setting as if the ACL is the default ACL settings.

Louis’s proposed behavior use case and suggestions:

• ACL setting is set in lambda Louis’s use case is that he rarely set the ACL in client side SDK. In his project, he usually set the ACL in cloud code using lambda function.
• null column is treated public-read and the database column is not nullable
• prefer not to change the meaning of null

I will recommend a solution that is the most consistent with the existing behavior.

[cheungpat]

Current implementation

Skygear Server

Fetching

• If _access=null in database, the record ACL is considered as public-writable, regardless of Default ACL.
• Returning Record to client if _access=null in database:
  • return _access=null if Default ACL is not defined
  • return _access=(default-acl) if Default ACL is defined

Summary: When fetching, the Default ACL setting does not affect ACL evaluation. The Default ACL is returned (but not considered) if _access=null in database.

Saving

• If _access=null in database, the record ACL is considered as public-writable, regardless of Default ACL.
• When client tries to save _access=null:
  • if _access=null in database
    • Save as _access=null if no Default ACL is defined
    • Save as _access=(default-acl) if no Default ACL is defined
  • if _access=(some-acl) in database
    • Save as _access=(some-acl) if no Default ACL is defined
    • Save as _access=(default-acl) if Default ACL is defined
• When client tries to save without specifying _access, the behaviour is the same as if the client specified _access=null.
• When client specify _access=(some-acl) , save as _access=(some-acl) regardless of value in database or Default ACL

Summary: When saving, Default ACL setting does not affect ACL evaluation. If client save _access=null, the server will always save the Default ACL.

iOS SDK

• If Server sends _access=null, SDK parse as accessControl = nil
• Before 0.23:
  • There is a setting for default access control, which is only applied when new record is instantiated.
  • The default access control setting applies to all record types (it is not a per-record-type setting) (edit: the API is +[SKYAccessControl setDefaultAccessControl])
  • If there is no default access control setting, new record is instantiated with public-readable ACL
• In 0.23:
  • The default access control setting is only server-side (will not change client side behavior)
  • The default access control setting is per-record-type.
  • New record is always instantiated with accessControl = nil
• If accessControl = nil, the client send the Record without _access key in the JSON.

JS SDK

• If Server sends _access=null, SDK parse as public-readable
• Before 0.23:
  • Default access control setting is only client-side.
  • The default access control setting applies to all record types (it is not a per-record-type setting) (edit: the API is setDefaultACL)
  • New Record is instantiated with public-readable ACL
  • SDK will not send _access=null to the server
• In 0.23:
  • The default access control setting is only server-side (will not change client side behavior)
  • The default access control setting is per-record-type.
  • New Record is instantiated with _access=null
  • SDK will send _access=null to the server

Java SDK

• The Java SDK is not updated in 0.23 regarding to default ACL
• There is no default access control setting.
• New Record is instantiated with public-readable
• If Server sends _access=null, SDK parse as public-readable

My recommendation

This recommendations are made by considering the existing behaviour and to try to minimize breaking changes, while also considering what people expect and to avoid confusion in the future.

• Server should treat null ACL as public-readable instead of public-writable
  • From the people that I can reach, this is what people expect.
  • It is sad that the server treat null to be public-writable, but the SDK assumes public-readable. Therefore, if people use SDK to save records, changing the meaning of null ACL will not break these users.
  • Changing the meaning of null ACL will break those users who saved the record in cloud code and expect null to mean public writable.
• The _access column should be set to non-null (not implemented)
  • This is suggested by Louis and Steven
  • We can also set a SQL default to public-readable so cloud code will not break
• There should be a per-record-type default ACL setting on the server. (partially implemented)
  • We should have API to save default ACL setting
  • If the default ACL setting is not specified by a user, the default ACL setting should be public-readable
• The client may send _access=null to the server (partially implemented)
  • For new record, the server should treat this to mean the default ACL (or public-readable if no default ACL is specified)
  • For existing record, the server should treat this to mean the the access control setting will not change
  • The server should never send _access=null to the client
• The client should remember the default ACL setting (not implemented)
  • It is expected that the developer should always set the default ACL setting. This setting is remembered on the client side and also sent to server.
  • If there is no default ACL setting, the client should assume public-readable
• Record ACL will be copied from default ACL setting when Record ACL is being mutated (not implemented)
  • To mutate record ACL, the developer is expected to call (Record).set[Public][No|ReadOnly|Write]Access[For[User|Role]].
  • If Record ACL is null, copy the default ACL setting, then mutate it.
  • The mutated record ACL is sent to the server when saving
• If getting default access settings, return default ACL setting when Record ACL is null (not implemented)
  • To get record ACL, the developer is expected to call (Record).get[Public][No|ReadOnly|Write]Access[For[User|Role]].
  • Doing so will not change the Record ACL (still null)
• (iOS only) SKYRecord should implement [set|get][Public][No|ReadOnly|Write]Access[For[User|Role]] (not implemented)
  • these functions have the same behavior as above
  • the equivalent methods on SKYAccessControl should be either deprecated or discourage developer from calling
• (Android only) Should implement default ACL setting as described above (not implemented)
• Edit: (iOS and JS-only) the previous API (setDefaultACL and +[SKYAccessControl setDefaultAccessControl]) for setting default ACL will be deprecated. They will override the client-side default ACL if no per-record-type default ACL is defined (i.e. will override public-readable)

What will happen when the above is implemented

• When upgrading Skygear, it will run database migration so that _access becomes non-null. null values will be replaced by public-read
• Developer is expected to specify default ACL by calling setRecordDefaultAccess.
• Client SDK will remember the setting.
• When record is instantiated, record will have null ACL.
• When getting ACL from a newly instantiated record, the SDK returns the default ACL, or public-read if there is no default ACL. The record will still have null ACL.
• When modifying the ACL of a record having null ACL, the default ACL is copied, mutated and set as record ACL.
• When saving, SDK will send _access=null if record ACL is null.
• Server will apply the default ACL setting if _access=null if the record does not exists.
  • If the record exists, the Server will not change the ACL of the record.
• Server always a non-null ACL to the client.

Existing issues

• Previous discussions: Update old records by ID will override default ACL #309
• Initial bug report: Revisit default behavior for record ACL #283
• setReadWriteAccessForRole on new record result in TypeError: this.access is null skygear-SDK-JS#203
• Changing server-side creation access settings skygear-SDK-Android#70

New issues

These should have higher priority because there is a bug on the released version and people cannot modify ACL of records:

• Remember and apply default ACL settings to records skygear-SDK-iOS#96
• Remember and apply default ACL settings to records skygear-SDK-JS#208

These should have medium priority:

• Make _access column non-nullable #361
• ACL is incorrectly overwritten when client save with null ACL #362
• Apply default ACL and fallback ACL #363
• Allow client to remove default ACL previously set #364

These should have lower priority because they are new features:

• Changing server-side default ACL settings skygear-SDK-Android#126
• Remember and apply default ACL settings to records skygear-SDK-Android#127

[b123400]

Questions:

Louis’s proposed behavior:

• ACL setting is set in lambda

What does it mean to set ACL in lambda?

cheungpat's proposal:

• The client should remember the default ACL setting (not implemented)

When will client fetch the default ACL of record types from server?

[cheungpat]

What does it mean to set ACL in lambda?

Sorry I wrote it hastily. I was meaning to say this: Louis described his use case in which he rarely set the ACL in client side SDK. In his project, he usually set the ACL in cloud code using lambda function. Since lambda function set the ACL directly using SQL, his use case is not really relevant in our discussion of how the client SDK should behave. (but still interesting).

When will client fetch the default ACL of record types from server?

Good question. “The client should remember the default ACL setting” meaning that when the developer sets the default ACL using setRecordDefaultAccess, the client SDK will save this setting in somewhere like Container (in addition to sending it to server). It is also expected that the developer will always call setRecordDefaultAccess. Therefore, the client does not need to fetch default ACL of record types from the server.

[b123400]

It is also expected that the developer will always call setRecordDefaultAccess.

What happens if someone try to mutate ACL without calling setRecordDefaultAccess before?

[louischan-oursky]

Sorry for being a bit off topic but here is a problem that I came across before.

Putting access control logic in client (especially mobile client) is not a good idea.The access control of some table needs to be changed due to requirement change. The old client will keep saving record with old access control. I have to write before_save hook to alter the access control so that it matches latest requirement. Finally I decided that I just put access control logic in before_save hook to override whatever value the client specifies.

[cheungpat]

@b123400

What happens if someone try to mutate ACL without calling setRecordDefaultAccess before?

“If there is no default ACL setting, the client should assume public-readable”. To answer your question, if setRecordDefaultAccess is not called before, the default access is assumed to be public-readable.

In other words, calling record.setReadOnlyForRole(role) is the same as:

// this is for illustration purpose only
// code does not need to look like this
// for example, `recordDefaultAccess` maybe written as a function that returns `publicReadable` if there is no default access defined
if (!this.access) this.access = recordDefaultAccess[this.recordType] || publicReadable
this.access.setReadOnlyForRole(role)

[cheungpat]

@louischan-oursky

Sorry for being a bit off topic but here is a problem that I came across before.

No I think it is relevant. I think I agree with you that it is a good idea to put ACL logic server-side.

I just want to add that if the developer wants ACL to apply on the server side, they can achieve this by never modifying the ACL of a record on the client side. Server will apply the default ACL and the client will respect that.

The above recommendation is focused on developer who wants to put ACL logic client-side. If developer wants to put ACL logic server-side, I think they will be able to achieve that too.

[cheungpat]

@limouren said null in the database should means public-writable. His logic is that when _access is null, it means no access control is applied to the record and therefore there is no restriction on access.

[rickmak]

The recommendation aligns with my expectation.

[ben181231]

The recommendation looks good to me.

[b123400]

Look good to me

[chpapa]

Thanks for the thoughtful design. I want to bring up two issue:

1. There are current setDefaultACL and setRecordDefaultAccess in JS SDK (or in iOS defaultAccessControl), and was not mentioned, will we keep them? If not we need to obsolete it in client SDK.
2. I'm worried that this change would still break existing users (CloudPillar?) which we will need to be able to pin version for them before deploy the above update to Cloud.

[cheungpat]

@chpapa mentioned setDefaultACL in JS (pre-0.23 and 0.23), and +[SKYAccessControl setDefaultAccessControl] in iOS (pre-0.23 only). These are the API for the following mentioned point in the above comment:

The default access control setting applies to all record types (it is not a per-record setting)

These API applies to all record types and they are not a per-record setting. The existing (and also the proposed) API setRecordDefaultAccess is a per-record setting.

To make it backward compatible, we can deprecate setDefaultACL and +[SKYAccessControl setDefaultAccessControl] and keep them to override the client-side default ACL setting if no per-record setting is defined.

Since these two API existed before we have a server-side default ACL setting, I have no intention of updating these API to update the server-side default ACL setting.

I'm worried that this change would still break existing users (CloudPillar?) which we will need to be able to pin version for them before deploy the above update to Cloud.

How will the recommendation break existing user in your thought?

[chpapa]

How will the recommendation break existing user in your thought?

I know that account make heavy use of Cloud Functions, so would they really have some code that assume _access = null is public writable?

[cheungpat]

@chpapa thinks _access=null in the database should be treated as public-writable (at least at the point of database migration)

[chpapa]

Not sure, it seems a bad decision just for backward compatible for some rare edge case.

[rickmak]

The existing (and also the proposed) API setRecordDefaultAccess is a per-record setting.

I missed this. And I think setRecordDefaultAccess it NOT per-record setting. It is per-recordType setting.

[cheungpat]

I missed this. And I think setRecordDefaultAccess it NOT per-record setting. It is per-recordType setting.

Definitely. Sorry. I cannot be right all the time. 😆

[cheungpat]

In my document, all instances of “per-record” actually means “per-record-type”. The document is updated to fix this.

[cheungpat]

If there are no further comments on the recommendation, I will create new GitHub issues for the implementation of the above recommendation.

[cheungpat]

I opened new issues. Please see above.

I prefer that I don’t do the coding for these issues but I expect that all of the PR for these issues to be addressed to me. Make sure that you have adequate unit tests.

[rickmak]

Can I propose @Steven-Chan do it all after switching in and @cheungpat review all?

When will @Steven-Chan able to start?

[rickmak]

Turn out @Steven-Chan switch in and do the field ACL.

It results:

• No document or guide to review field ACL together with record ACL.
• We have no feature issue on record ACL for discussion reference.
• The issue raise by new comer @carmenlau again

I propose:

• Re-pickup this issue.
• Write a feature issue on record ACL. It a bit meta, but will help us to act as a reference during discussion.
• Consider the feature issue on record ACL together with field ACL. Produce a guide on how to setup both of them under common use-case.
• Review the default and modify it should we found a better default.

[chpapa]

I move this to Milestone 3, as I'm not sure if it is related with the High Level Review of ACL / Field ACL.