feat: Public Actor Permissions #2053
Conversation
sources/platform/actors/development/actor_definition/input_schema/specification.md
Outdated
Show resolved
Hide resolved
| - `["READ"]` — the Actor may read from the referenced resource(s). | ||
| - `["READ", "WRITE"]` — the Actor may read and write to the referenced resource(s). | ||
|
|
||
| Notes: |
There was a problem hiding this comment.
Let's mention that this will be communicated to the users via the form (the tooltip).
There was a problem hiding this comment.
This is the copy that needs to be updated. Let's not forget about updating this screenshot as well.
…y-docs into feat/public-actor-permissions
|
Preview for this PR was built for commit |
raethlo
requested review from
tobice
and removed request for
fnesveda and
janbuchar
|
Preview for this PR was built for commit |
|
|
||
|  | ||
|
|
||
| ### End-user experience |
There was a problem hiding this comment.
This is a repetition of the "Impact of Permission Level" section below. Maybe just remove this and move the screenshot to that section?
There was a problem hiding this comment.
3a0c308 Decided to leave the end user experience section and move the implications section(+your suggestions) to the warning admonition.
| } | ||
| ``` | ||
|
|
||
| Now when the platform runs your Actor, it’ll automatically add the user provided storage the Actor’s scope so that it can access it. |
|
|
||
| ::: | ||
|
|
||
| **If the existing contents of the named storage is critical for your Actor to keep functioning for the existing users** (as in, you can’t afford removing the storage), reach out to us. We can discuss the available options. |
There was a problem hiding this comment.
If the existing contents of the named storage are critical...
Also, is the emphasis necessary for such a long part of the sentence?
sources/platform/actors/development/permissions/migration_guide.md
Outdated
Show resolved
Hide resolved
|
Preview for this PR was built for commit |
|
Preview for this PR was built for commit |
|
Preview for this PR was built for commit |
|
🚧 Note: before this can go live we still need to update the screenshots. |
|
Preview for this PR was built for commit |
| - How your Actor uses storages: if it only writes to default storages, no changes needed. | ||
| - Whether it calls other Actors: targets must also use limited permissions. | ||
| - Whether it accesses user-provided storages: declare `resourceType` and `resourcePermissions` in `input_schema.json`. | ||
| - Whether it uses named storages: rename/recreate and migrate data to retain access under limited permissions. | ||
| - Whether it needs to know if the user is paying: use `APIFY_USER_IS_PAYING` or the SDK. | ||
| - Whether it needs the user's proxy password: use `APIFY_PROXY_PASSWORD` or the SDK. |
There was a problem hiding this comment.
I don't think this list is necessary - it simply duplicates what you have stated below. Could remove it entirely, including the sentence before it "To assess what, if anything, needs to change in your Actor, review the following:"
There was a problem hiding this comment.
Fair, I liked the idea of having a summary before the respective more detailed sections, but as the sections are not as detailed, I think we can drop it,
| :::warning | ||
|
|
||
| Actor permissions are a new feature. With the initial release, the distinction between full-permissions and limited-permissions Actors is primarily informational, but it will become increasingly significant over time. | ||
|
|
||
| Future changes may include: | ||
|
|
||
| - Stronger confirmations and/or warnings added when running full-permissions Actors | ||
| - Actors requiring full permissions may receive a lower [Actor Quality score](../../publishing/quality_score.mdx), which can reduce their ranking in the store. | ||
| - Some platform features and recommendations may prioritize limited-permissions Actors by default. | ||
|
|
||
| Whenever possible, design your Actors to use limited permissions and request only access they truly need. | ||
|
|
||
| ::: |
There was a problem hiding this comment.
Looking through the preview, this feels like quite a lot of content for a warning block. Maybe we could have just the first paragraph in the warning block, and the rest (Future changes may include...") as normal content.
There was a problem hiding this comment.
We should drop any mentions of future changes, they are not relevant to documentation. Documentation should focus only on now.
|
Preview for this PR was built for commit |
There was a problem hiding this comment.
Looks good 👍
Left few suggestions, more like nits and optional stuff, that might improve readability.
Main concern is that currently permissions and migration_guide a bit mixed up, there're some section that might be more logical in migration_guide, but it is only my opinion, we can discuss it 🙃
|
|
||
| See the full [input schema reference for details.](../actor_definition/input_schema/specification.md). | ||
|
|
||
| ### Requesting full permissions |
There was a problem hiding this comment.
Title doesn't correspond to what we write in the paragraph. Who is requesting full permissions? Sounds confusing.
Maybe Keeping full permissions?
There was a problem hiding this comment.
Also not sure, maybe move to migrations?
|
|
||
| Recommended minimum SDK versions: | ||
|
|
||
| - JavaScript SDK: [apify@3.4.4](https://github.com/apify/apify-sdk-js/releases/tag/apify%403.4.4) |
There was a problem hiding this comment.
(nit): why versions are inconsistent, let's just keep numbers ? 3.4.4 and 3.0.0
| ::: | ||
|
|
||
|
|
||
| ### Accessing user provided storages |
There was a problem hiding this comment.
(opt): This section feels strange for me. In the article we're talking about how Actor permissions work and than in this section we switch to just limited permissions, also we kind of repeat ourselves a bit, because there's a very similar information in the migration, maybe this section should be in migration?
But on the other hand this section would make sense for me, if we'll also speak about how it currently works for full permissions:
- no need to pass resourcePermissions
- some devs could avoid using inputs and just have it hardcoded in the code.
There was a problem hiding this comment.
Got it, I've added a bit about full permissions here b6d6eb8
|
|
||
| --- | ||
|
|
||
| Use this guide to migrate existing Actors to use [limited permissions](index.md#how-actor-permissions-work). The general prerequisite is to update to the latest [Apify SDK](https://docs.apify.com/sdk). |
There was a problem hiding this comment.
(nit):
| Use this guide to migrate existing Actors to use [limited permissions](index.md#how-actor-permissions-work). The general prerequisite is to update to the latest [Apify SDK](https://docs.apify.com/sdk). | |
| This guide explains how to migrate your existing Actors to use [limited permissions](https://github.com/apify/apify-docs/pull/2053/index.md#how-actor-permissions-work). Before you start, make sure your Actor uses the latest [Apify SDK](https://docs.apify.com/sdk) | |
| . |
|
|
||
| Actors sometimes use named storages for caching or persisting state across runs. With limited permissions, an Actor can create a named storage on its first run and will automatically retain access to it in all subsequent runs by the same user. | ||
|
|
||
| If your Actor previously relied on accessing a pre-existing named storage, you will need to rename it in your code. This will cause the Actor to recreate the storage under the new system on its next run. |
There was a problem hiding this comment.
(nit): oh, I am afraid that no one would do it, it seems complicated 😄
But I understand that there's not much we can do about it.
Maybe I would also add to this section something like avoid using hardcoded storages and just update the input, so user can provide it(and link to "The actor accesses storages provided by users" or to current "Accessing user provided storages")?
There was a problem hiding this comment.
That's a pretty good idea how to migrate existing storages 👍🏻
|
👋 With the detailed review from Dan and Nish, I don't think my voice is needed here... so Roman don't wait on an approval from me 😊 |
Co-authored-by: Daniil Poletaev <44584010+danpoletaev@users.noreply.github.com>
|
Preview for this PR was built for commit |
|
Preview for this PR was built for commit |
| ### Accessing user provided storages | ||
|
|
||
| Limited-permissions Actors can access storages that users explicitly provide via the Actor input. Use the input schema to add a storage picker and declare exactly which operations your Actor needs. | ||
| By default, limited-permissions Actors can't access users storages. However, they can access storages that users explicitly provide via the Actor input. To do so, use the input schema to add a storage picker and declare exactly which operations your Actor needs. |
There was a problem hiding this comment.
either user storages or users' storages. For consistency with other places, user storages - I see this used below.
| - Set `resourceType` to one of `dataset`, `keyValueStore`, or `requestQueue`. | ||
| - Specify `resourcePermissions` with the minimal required scope: `"READ"` or `"READ", "WRITE"`. | ||
|
|
||
| Actors running under full permissions are assumed to have full (i.e. read/write/manage) access to user storages, in that case the `resourcePermissions` field does not have to be set. |
There was a problem hiding this comment.
| Actors running under full permissions are assumed to have full (i.e. read/write/manage) access to user storages, in that case the `resourcePermissions` field does not have to be set. | |
| Actors running under full permissions are assumed to have full (i.e. read/write/manage) access to user storages. In that case the `resourcePermissions` field does not have to be set. |
| ::: | ||
|
|
||
| ### Declaring permissions | ||
| ### Configuring Actor permissions level |
There was a problem hiding this comment.
| ### Configuring Actor permissions level | |
| ### Configuring Actor permission level |
This PR adds docs for a new platfrom feature: Actor permissions.