You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@openwhisk.apache.org by GitBox <gi...@apache.org> on 2020/09/08 08:54:30 UTC
[GitHub] [openwhisk] ningyougang opened a new pull request #4058: Add protect feature to avoid update or delete actions by mistake
ningyougang opened a new pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058
As more and more production system uses openwhisk,
Users will need some features to protect their service safe from mistake.
If we have this feature, user can protect their actions from deletion by mistake.
When users create action use like below(add annotation "lock":true)
```
curl -X PUT -H "Content-type: application/json" --user 23bc46b1-71f6-4ed5-8c54-816aa4f8c502:123zO3xZCLrMN6v2BKK1dXYFpXlPkccOFqm12CdAsMgRU4VrNZ9lyGVCGuMDGIwP
-d '{"namespace":"guest","name":"hello","annotations":[{"key":"lock","value":true}],"exec":{"kind":"nodejs:default","code":"// Licensed to the Apache Software Foundation (ASF) under one or more contributor\r\n// license agreements; and to You under the Apache License, Version 2.0.\r\n\r\n/**\r\n * Hello, world.\r\n */\r\nfunction main() {\r\n return {\"payload\":\"Hello world\"};\r\n}\r\n"}}'
'http://xxx.xxx.xxx.xxx:port/api/v1/namespaces/guest/actions/hello?'
```
The lock field will be added in couchdb's annotation, like below
```
...
"annotations": [
{
"key": "exec",
"value": "nodejs:6"
},
{
"key": "lock",
"value": true
}
...
```
So this action can't be deleted/updated until the protection is updated to `false`
<!--- Provide a concise summary of your changes in the Title -->
## Description
<!--- Provide a detailed description of your changes. -->
<!--- Include details of what problem you are solving and how your changes are tested. -->
## Related issue and scope
<!--- Please include a link to a related issue if there is one. -->
- [ ] I opened an issue to propose and discuss this change (#????)
## My changes affect the following components
<!--- Select below all system components are affected by your change. -->
<!--- Enter an `x` in all applicable boxes. -->
- [x] API
- [x] Controller
- [ ] Message Bus (e.g., Kafka)
- [ ] Loadbalancer
- [ ] Invoker
- [ ] Intrinsic actions (e.g., sequences, conductors)
- [ ] Data stores (e.g., CouchDB)
- [ ] Tests
- [ ] Deployment
- [ ] CLI
- [ ] General tooling
- [ ] Documentation
## Types of changes
<!--- What types of changes does your code introduce? Use `x` in all the boxes that apply: -->
- [ ] Bug fix (generally a non-breaking change which closes an issue).
- [ ] Enhancement or new feature (adds new functionality).
- [ ] Breaking change (a bug fix or enhancement which changes existing behavior).
## Checklist:
<!--- Please review the points below which help you make sure you've covered all aspects of the change you're making. -->
- [x] I signed an [Apache CLA](https://github.com/apache/incubator-openwhisk/blob/master/CONTRIBUTING.md).
- [x] I reviewed the [style guides](https://github.com/apache/incubator-openwhisk/wiki/Contributing:-Git-guidelines#code-readiness) and followed the recommendations (Travis CI will check :).
- [x] I added tests to cover my changes.
- [ ] My changes require further changes to the documentation.
- [x] I updated the documentation where necessary.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang edited a comment on pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang edited a comment on pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#issuecomment-626607066
Hi, guys, i push a temp commit here in this patch, just check my direction(permission unix sytle) is whether right. https://github.com/apache/openwhisk/pull/4058/commits/d77a80fd0348d05f3733f95dbc0e016429e96753
After check in my local, i found that, there has some differences between our own permission management between unix permission management.
### 1. user type is different
for one action, just have 2 type users.
* the action's owner
* the user (not the owner) who used the shared action directly(eg. get, invoke)
As we all already know, unix's permission has `3` user types, this is the different point.
### 2. our own permission value is restricted
* for one action, anyway, the user(not the owner) can't modify/delete the action on any situation, just the owner has the `write` permission.
* the owner's permission can affect the permission as well, e.g. if the owner is not given `read` permission, i think the user(not the owner) can't have `read` permission as well. if the owner is not give `execute` permission, the user(not the owner) can't have `execute` permission as well.
How do you guys think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang edited a comment on pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang edited a comment on pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#issuecomment-851116337
According to review comments
It seems there has another option to do `granular permission management`
#### 1. There is no need to add permissions controls for action's "owner" since he/she can anyway update/remove/invoke his/her action
#### 2. It is necessary to control other shared user's permission (e.g. whether has permission to read/download the codes, whether has permission to execute the codes), this separate document in db: `xxx_permission` may like below
```
{
"_id": "whisk.system/helloAction",
"_rev": "1-1a14818c1083bc12b6d3fd1774fcc956",
"default": {
{
"read" : false
"execute": false
}
}
"permission": {
"theSharedUserA": {
"read" : yes
"execute": yes
},
"theSharedUserB": {
"read" : yes
"execute": false
}
}
```
Regarding above permission document of `whisk.system/helloAction`
* when create action: `whisk.system/helloAction`, the owner can set default read/execute permission. e.g.
```
"default": {
{
"read" : false
"execute": false
}
}
```
* the shared user will inherits the default permissions if not explicitly specified in above document. e.g.
not explicitly specified for theSharedUserC, so theSharedUserC will have default permission
* if explicitly specified in above document for the shared user, the shared user will have specified permission, e.g.
already explicitly specified for theSharedUserA, so theSharedUserA's permission is
```
"theSharedUserA": {
"read" : yes
"execute": yes
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang commented on a change in pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang commented on a change in pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#discussion_r641189638
##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/entitlement/Entitlement.scala
##########
@@ -231,6 +234,157 @@ protected[core] abstract class EntitlementProvider(
.getOrElse(Future.successful(()))
}
+ /**
+ * Checks if action operation(get/write/execute) whether feasible
+ *
+ * @param operation the action operation, e.g. get/write/execute
+ * @param user the user who get/write/execute the action
+ * @param entityStore store to write the action to
+ * @param entityName entityName
+ * @param permissions the passed permission code
+ * @return a promise that completes with success iff action operation is feasible
+ */
+ protected[core] def checkActionPermissions(
+ operation: String,
+ user: Identity,
+ entityStore: ArtifactStore[WhiskEntity],
+ entityName: FullyQualifiedEntityName,
+ get: (ArtifactStore[WhiskEntity], DocId, DocRevision, Boolean) => Future[WhiskAction],
+ permissions: Option[String] = None)(implicit transid: TransactionId): Future[Unit] = {
+ if (operation == "create") {
+ permissions
+ .map { value =>
+ if (WhiskAction.permissionList.contains(value)) {
+ Future.successful(())
+ } else {
+ val errorInfo =
+ s"give error permission code: ${value}, available permission is in ${WhiskAction.permissionList}"
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ }
+ }
+ .getOrElse(Future.successful(()))
+ } else if (operation == "update") {
+ get(entityStore, entityName.toDocId, DocRevision.empty, true)
+ .flatMap { whiskAction =>
+ val currentPermissions = whiskAction.annotations
+ .get(WhiskAction.permissionsFieldName)
+ .map(value => value.convertTo[String])
+ .getOrElse(WhiskAction.defaultPermissions)
+
+ val errorInfo = s"have no permission to ${operation} this action"
+ permissions match {
+ case Some(value) =>
+ if (!WhiskAction.permissionList.contains(value)) {
+ val errorInfo =
+ s"give error permission code: ${value}, available permission is in ${WhiskAction.permissionList}"
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ } else {
+ val passedUpdatePermission = value.charAt(1)
+ if (passedUpdatePermission == 'w') { // make it to modifiable
+ Future.successful(())
Review comment:
Yes, you are right, after changed the write permission to available, it can be modified again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang edited a comment on pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang edited a comment on pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#issuecomment-850162422
> one of my thought is that there is no need to add permissions controls for action's "owner" since he/she can anyway update/remove/invoke his/her action
It seems just need to control the permission for the shared user. (e.g. the shared user whether has permission to download code, execute the codes)
But for this pr, our initial target is `avoid the owner update or delete actions by mistake`, it seems if don't add permission to action's owner, can't reach the initial target
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang edited a comment on pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang edited a comment on pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#issuecomment-626607066
Hi, guys, i push a commit here in this patch, just check my direction(permission unix sytle) is whether right. https://github.com/apache/openwhisk/pull/4058/commits/d77a80fd0348d05f3733f95dbc0e016429e96753
After check in my local, i found that, there has some differences between our own permission management between unix permission management.
### 1. user type is different
for one action, just have 2 type users.
* the action's owner
* the user (not the owner) who used the shared action directly(eg. get, invoke)
As we all already know, unix's permission has `3` user types, this is the different point.
### 2. owr own permission value is restricted
* for one action, anyway, the user(not the owner) can't write the action on any situation, just the owner has the write permission.
* the owner's permission can affect the permission as well, e.g. if the owner is not given `read` permission, i think the user(not the owner) can't have `read` permission as well. if the owner is not give `execute` permission, the user(not the owner) can't have `execute` permission as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang commented on a change in pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang commented on a change in pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#discussion_r504413705
##########
File path: common/scala/src/main/scala/org/apache/openwhisk/core/entity/WhiskAction.scala
##########
@@ -351,6 +361,49 @@ object WhiskAction extends DocumentFactory[WhiskAction] with WhiskEntityQueries[
val execFieldName = "exec"
val requireWhiskAuthHeader = "x-require-whisk-auth"
+ // annotation permission key name
+ val permissionsFieldName = "permissions"
Review comment:
yes, i also feel it is not a good idea to use annotation for permission management.
Currently, have no better idea for it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] style95 commented on a change in pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
style95 commented on a change in pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#discussion_r496399630
##########
File path: common/scala/src/main/scala/org/apache/openwhisk/core/entity/WhiskAction.scala
##########
@@ -351,6 +361,49 @@ object WhiskAction extends DocumentFactory[WhiskAction] with WhiskEntityQueries[
val execFieldName = "exec"
val requireWhiskAuthHeader = "x-require-whisk-auth"
+ // annotation permission key name
+ val permissionsFieldName = "permissions"
+
+ val defaultPermissions = "rwxr-x"
+
+ // notes on users, just have 2 type users,
+ // 1. the action's owner
+ // 2. the user (not the owner) who used the shared action directly(e.g. get, invoke), we call it "the shared user"
+ //
+ // Notes on permission control
+ // 1. the owner has read(or download) permission on any situation, but for the shared user,
+ // in spite of has read permission on any situation, but can set it undownloadable or downloadable
+ // 2. the shared user can't update/delete the action on any situation.
+ // 3. the owner's permission can affect the shared user's permission, e.g
+ // if the owner is not given execute permission, the shared user can't have execute permission as well.
+ //
+ // Notes on permission values, include below permission value
+ // 1. permission code:rwxr-x: owner:read(yes)/write(yes)/execute(yes)|the shared action's user:download(yes)/write(no)/execute(yes), this is default
+ // 2. permission code:rwxr--: owner:read(yes)/write(yes)/execute(yes)|the shared action's user:download(yes)/write(no)/execute(no)
+ // 3. permission code:r-xr-x: owner:read(yes)/write(no)/execute(yes)|the shared action's user:download(yes)/write(no)/execute(yes)
+ // 4. permission code:r-xr--: owner:read(yes)/write(no)/execute(yes)|the shared action's user:download(yes)/write(no)/execute(no)
+ // 5. permission code:r--r--: owner:read(yes)/write(no)/execute(no)|the shared action's user:download(yes)/write(no)/execute(no)
+ // 6. permission code:rw-r--: owner:read(yes)/write(yes)/execute(no)|the shared action's user:download(yes)/write(no)/execute(no)
+ // 7. permission code:rwx--x: owner:read(yes)/write(yes)/execute(yes)|the shared action's user:download(no)/write(no)/execute(yes)
+ // 8. permission code:rwx---: owner:read(yes)/write(yes)/execute(yes)|the shared action's user:download(no)/write(no)/execute(no)
+ // 9. permission code:r-x--x: owner:read(yes)/write(no)/execute(yes)|the shared action's user:download(no)/write(no)/execute(yes)
+ // 10. permission code:r-x---: owner:read(yes)/write(no)/execute(yes)|the shared action's user:download(no)/write(no)/execute(no)
+ // 11. permission code:r-----: owner:read(yes)/write(no)/execute(no)|the shared action's user:download(no)/write(no)/execute(no)
+ // 12. permission code:rw----: owner:read(yes)/write(yes)/execute(no)|the shared action's user:download(no)/write(no)/execute(no)
+ val permissionList = List(
+ defaultPermissions,
+ "rwxr--",
+ "r-xr-x",
+ "r-xr--",
+ "r--r--",
Review comment:
Is there any case where an owner does not need to either update or execute the action?
If so, how can the user update the action permission and the action itself?
##########
File path: common/scala/src/main/scala/org/apache/openwhisk/core/entity/WhiskAction.scala
##########
@@ -351,6 +361,49 @@ object WhiskAction extends DocumentFactory[WhiskAction] with WhiskEntityQueries[
val execFieldName = "exec"
val requireWhiskAuthHeader = "x-require-whisk-auth"
+ // annotation permission key name
+ val permissionsFieldName = "permissions"
+
+ val defaultPermissions = "rwxr-x"
+
+ // notes on users, just have 2 type users,
+ // 1. the action's owner
+ // 2. the user (not the owner) who used the shared action directly(e.g. get, invoke), we call it "the shared user"
+ //
+ // Notes on permission control
+ // 1. the owner has read(or download) permission on any situation, but for the shared user,
+ // in spite of has read permission on any situation, but can set it undownloadable or downloadable
+ // 2. the shared user can't update/delete the action on any situation.
+ // 3. the owner's permission can affect the shared user's permission, e.g
+ // if the owner is not given execute permission, the shared user can't have execute permission as well.
+ //
+ // Notes on permission values, include below permission value
+ // 1. permission code:rwxr-x: owner:read(yes)/write(yes)/execute(yes)|the shared action's user:download(yes)/write(no)/execute(yes), this is default
+ // 2. permission code:rwxr--: owner:read(yes)/write(yes)/execute(yes)|the shared action's user:download(yes)/write(no)/execute(no)
+ // 3. permission code:r-xr-x: owner:read(yes)/write(no)/execute(yes)|the shared action's user:download(yes)/write(no)/execute(yes)
+ // 4. permission code:r-xr--: owner:read(yes)/write(no)/execute(yes)|the shared action's user:download(yes)/write(no)/execute(no)
+ // 5. permission code:r--r--: owner:read(yes)/write(no)/execute(no)|the shared action's user:download(yes)/write(no)/execute(no)
+ // 6. permission code:rw-r--: owner:read(yes)/write(yes)/execute(no)|the shared action's user:download(yes)/write(no)/execute(no)
+ // 7. permission code:rwx--x: owner:read(yes)/write(yes)/execute(yes)|the shared action's user:download(no)/write(no)/execute(yes)
+ // 8. permission code:rwx---: owner:read(yes)/write(yes)/execute(yes)|the shared action's user:download(no)/write(no)/execute(no)
+ // 9. permission code:r-x--x: owner:read(yes)/write(no)/execute(yes)|the shared action's user:download(no)/write(no)/execute(yes)
+ // 10. permission code:r-x---: owner:read(yes)/write(no)/execute(yes)|the shared action's user:download(no)/write(no)/execute(no)
+ // 11. permission code:r-----: owner:read(yes)/write(no)/execute(no)|the shared action's user:download(no)/write(no)/execute(no)
+ // 12. permission code:rw----: owner:read(yes)/write(yes)/execute(no)|the shared action's user:download(no)/write(no)/execute(no)
+ val permissionList = List(
Review comment:
I'd apply a regex match to confirm the permission value rather than just checking the existence of the value in a list.
##########
File path: common/scala/src/main/scala/org/apache/openwhisk/core/entity/WhiskAction.scala
##########
@@ -351,6 +361,49 @@ object WhiskAction extends DocumentFactory[WhiskAction] with WhiskEntityQueries[
val execFieldName = "exec"
val requireWhiskAuthHeader = "x-require-whisk-auth"
+ // annotation permission key name
+ val permissionsFieldName = "permissions"
Review comment:
I am not quite sure it is a good idea to use annotations for permission management.
I feel like we need a new protocol.
##########
File path: common/scala/src/main/scala/org/apache/openwhisk/core/entity/WhiskAction.scala
##########
@@ -351,6 +361,49 @@ object WhiskAction extends DocumentFactory[WhiskAction] with WhiskEntityQueries[
val execFieldName = "exec"
val requireWhiskAuthHeader = "x-require-whisk-auth"
+ // annotation permission key name
+ val permissionsFieldName = "permissions"
+
+ val defaultPermissions = "rwxr-x"
+
+ // notes on users, just have 2 type users,
Review comment:
I'd rather keep this information in some markdown file and just leave the link here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang edited a comment on pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang edited a comment on pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#issuecomment-626607066
Hi, guys, i push a commit here in this patch: https://github.com/apache/openwhisk/pull/4058/commits/d77a80fd0348d05f3733f95dbc0e016429e96753
Just check whether my direction(permission unix sytle) is right.
After check in my local, i found that, we can use some parts of unix-sytle. e.g.
for one action, just have 2 type users.
* the action's owner
* the user (not the owner) who used the shared action directly(eg. get, invoke)
As we all already know, unix's permission controller has `3` user types, this is the different point.
Another different point the permission values is restricted
* for one action, anyway, the user(not the owner) can't write the action, just the owner has the write permission.
* the owner's permission can affect the permission as well, e.g. if the owner is not given `read` permission, i think the user(not the owner) can't have `read` permission as well. if the owner is not give `execute` permission, the user(not the owner) can't have `execute` permission as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang edited a comment on pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang edited a comment on pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#issuecomment-626607066
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] codecov-commenter edited a comment on pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#issuecomment-850276876
# [Codecov](https://codecov.io/gh/apache/openwhisk/pull/4058?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#4058](https://codecov.io/gh/apache/openwhisk/pull/4058?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (92b2a38) into [master](https://codecov.io/gh/apache/openwhisk/commit/b818c3b3e8bd3fa9ac7742d1b8c051ec09b76ae2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b818c3b) will **decrease** coverage by `1.47%`.
> The diff coverage is `90.62%`.
[](https://codecov.io/gh/apache/openwhisk/pull/4058?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #4058 +/- ##
==========================================
- Coverage 77.26% 75.79% -1.48%
==========================================
Files 219 226 +7
Lines 11205 11578 +373
Branches 487 498 +11
==========================================
+ Hits 8658 8776 +118
- Misses 2547 2802 +255
```
| [Impacted Files](https://codecov.io/gh/apache/openwhisk/pull/4058?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...cala/org/apache/openwhisk/http/ErrorResponse.scala](https://codecov.io/gh/apache/openwhisk/pull/4058/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2h0dHAvRXJyb3JSZXNwb25zZS5zY2FsYQ==) | `88.77% <ø> (+1.02%)` | :arrow_up: |
| [...pache/openwhisk/core/entitlement/Entitlement.scala](https://codecov.io/gh/apache/openwhisk/pull/4058/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9jb250cm9sbGVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvZW50aXRsZW1lbnQvRW50aXRsZW1lbnQuc2NhbGE=) | `83.67% <86.66%> (+1.85%)` | :arrow_up: |
| [...org/apache/openwhisk/core/controller/Actions.scala](https://codecov.io/gh/apache/openwhisk/pull/4058/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9jb250cm9sbGVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvY29udHJvbGxlci9BY3Rpb25zLnNjYWxh) | `90.90% <94.11%> (+0.12%)` | :arrow_up: |
| [...org/apache/openwhisk/core/entity/WhiskAction.scala](https://codecov.io/gh/apache/openwhisk/pull/4058/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvZW50aXR5L1doaXNrQWN0aW9uLnNjYWxh) | `93.06% <100.00%> (+0.71%)` | :arrow_up: |
| [...core/database/cosmosdb/RxObservableImplicits.scala](https://codecov.io/gh/apache/openwhisk/pull/4058/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvZGF0YWJhc2UvY29zbW9zZGIvUnhPYnNlcnZhYmxlSW1wbGljaXRzLnNjYWxh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ore/database/cosmosdb/cache/CacheInvalidator.scala](https://codecov.io/gh/apache/openwhisk/pull/4058/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9jb3Ntb3NkYi9jYWNoZS1pbnZhbGlkYXRvci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL29wZW53aGlzay9jb3JlL2RhdGFiYXNlL2Nvc21vc2RiL2NhY2hlL0NhY2hlSW52YWxpZGF0b3Iuc2NhbGE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...e/database/cosmosdb/cache/ChangeFeedConsumer.scala](https://codecov.io/gh/apache/openwhisk/pull/4058/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9jb3Ntb3NkYi9jYWNoZS1pbnZhbGlkYXRvci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL29wZW53aGlzay9jb3JlL2RhdGFiYXNlL2Nvc21vc2RiL2NhY2hlL0NoYW5nZUZlZWRDb25zdW1lci5zY2FsYQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...core/database/cosmosdb/CosmosDBArtifactStore.scala](https://codecov.io/gh/apache/openwhisk/pull/4058/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvZGF0YWJhc2UvY29zbW9zZGIvQ29zbW9zREJBcnRpZmFjdFN0b3JlLnNjYWxh) | `0.00% <0.00%> (-95.85%)` | :arrow_down: |
| [...sk/core/database/cosmosdb/CosmosDBViewMapper.scala](https://codecov.io/gh/apache/openwhisk/pull/4058/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvZGF0YWJhc2UvY29zbW9zZGIvQ29zbW9zREJWaWV3TWFwcGVyLnNjYWxh) | `0.00% <0.00%> (-93.90%)` | :arrow_down: |
| [...tabase/cosmosdb/cache/CacheInvalidatorConfig.scala](https://codecov.io/gh/apache/openwhisk/pull/4058/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9jb3Ntb3NkYi9jYWNoZS1pbnZhbGlkYXRvci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL29wZW53aGlzay9jb3JlL2RhdGFiYXNlL2Nvc21vc2RiL2NhY2hlL0NhY2hlSW52YWxpZGF0b3JDb25maWcuc2NhbGE=) | `0.00% <0.00%> (-92.31%)` | :arrow_down: |
| ... and [49 more](https://codecov.io/gh/apache/openwhisk/pull/4058/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/openwhisk/pull/4058?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/openwhisk/pull/4058?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [b818c3b...92b2a38](https://codecov.io/gh/apache/openwhisk/pull/4058?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] jiangpengcheng commented on a change in pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
jiangpengcheng commented on a change in pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#discussion_r639375736
##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/entitlement/Entitlement.scala
##########
@@ -231,6 +234,157 @@ protected[core] abstract class EntitlementProvider(
.getOrElse(Future.successful(()))
}
+ /**
+ * Checks if action operation(get/write/execute) whether feasible
+ *
+ * @param operation the action operation, e.g. get/write/execute
+ * @param user the user who get/write/execute the action
+ * @param entityStore store to write the action to
+ * @param entityName entityName
+ * @param permissions the passed permission code
+ * @return a promise that completes with success iff action operation is feasible
+ */
+ protected[core] def checkActionPermissions(
+ operation: String,
+ user: Identity,
+ entityStore: ArtifactStore[WhiskEntity],
+ entityName: FullyQualifiedEntityName,
+ get: (ArtifactStore[WhiskEntity], DocId, DocRevision, Boolean) => Future[WhiskAction],
+ permissions: Option[String] = None)(implicit transid: TransactionId): Future[Unit] = {
+ if (operation == "create") {
Review comment:
I think
```
operation match {
case "create" =>
...
```
should be better
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang edited a comment on pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang edited a comment on pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#issuecomment-858366875
@style95 ,
Current system permission already existed design solution:
* the owner have any permissions(read/update/execute) for this own actions.
* if a package is binded to a `shared` package, all users except the owner(we call it shared users) just have read/execute permission for that shared package's actions but do not have update permission.
* if package is not binded to any package, all users(except the owner) do not have any permissions(e.g. read/update/execute) for that package's actions
New design solution: https://github.com/apache/openwhisk/pull/4058#issuecomment-851116337
The difference between the new design solution and `the current system design` is:
* the new design solution can control every shared user's permission
hm..actually, for now, i don't know what's the direction of this pr.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang commented on a change in pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang commented on a change in pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#discussion_r641184253
##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/entitlement/Entitlement.scala
##########
@@ -231,6 +234,157 @@ protected[core] abstract class EntitlementProvider(
.getOrElse(Future.successful(()))
}
+ /**
+ * Checks if action operation(get/write/execute) whether feasible
+ *
+ * @param operation the action operation, e.g. get/write/execute
+ * @param user the user who get/write/execute the action
+ * @param entityStore store to write the action to
+ * @param entityName entityName
+ * @param permissions the passed permission code
+ * @return a promise that completes with success iff action operation is feasible
+ */
+ protected[core] def checkActionPermissions(
+ operation: String,
+ user: Identity,
+ entityStore: ArtifactStore[WhiskEntity],
+ entityName: FullyQualifiedEntityName,
+ get: (ArtifactStore[WhiskEntity], DocId, DocRevision, Boolean) => Future[WhiskAction],
+ permissions: Option[String] = None)(implicit transid: TransactionId): Future[Unit] = {
+ if (operation == "create") {
+ permissions
+ .map { value =>
+ if (WhiskAction.permissionList.contains(value)) {
+ Future.successful(())
+ } else {
+ val errorInfo =
+ s"give error permission code: ${value}, available permission is in ${WhiskAction.permissionList}"
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ }
+ }
+ .getOrElse(Future.successful(()))
+ } else if (operation == "update") {
+ get(entityStore, entityName.toDocId, DocRevision.empty, true)
+ .flatMap { whiskAction =>
+ val currentPermissions = whiskAction.annotations
+ .get(WhiskAction.permissionsFieldName)
+ .map(value => value.convertTo[String])
+ .getOrElse(WhiskAction.defaultPermissions)
+
+ val errorInfo = s"have no permission to ${operation} this action"
+ permissions match {
+ case Some(value) =>
+ if (!WhiskAction.permissionList.contains(value)) {
+ val errorInfo =
+ s"give error permission code: ${value}, available permission is in ${WhiskAction.permissionList}"
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ } else {
+ val passedUpdatePermission = value.charAt(1)
+ if (passedUpdatePermission == 'w') { // make it to modifiable
+ Future.successful(())
+ } else {
+ val currentUpdatePermission = currentPermissions.charAt(1)
+ if (currentUpdatePermission == '-') {
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ } else {
+ Future.successful(())
+ }
+ }
+ }
+ case None =>
+ val currentUpdatePermission = currentPermissions.charAt(1)
+ if (currentUpdatePermission == '-') {
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ } else {
+ Future.successful(())
+ }
+ }
+ }
+ .recoverWith {
+ case t: RejectRequest =>
+ Future.failed(t)
+ case _ =>
+ Future.successful(())
+ }
+ } else if (operation == "remove") {
+ get(entityStore, entityName.toDocId, DocRevision.empty, true)
+ .flatMap { whiskAction =>
+ val currentPermissions = whiskAction.annotations
+ .get(WhiskAction.permissionsFieldName)
+ .map(value => value.convertTo[String])
+ .getOrElse(WhiskAction.defaultPermissions)
+
+ val currentUpdatePermission = currentPermissions.charAt(1)
+ if (currentUpdatePermission == '-') {
+ val errorInfo = s"have no permission to ${operation} this action"
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ } else {
+ Future.successful(())
+ }
+ }
+ .recoverWith {
+ case t: RejectRequest =>
+ Future.failed(t)
+ case _ =>
+ Future.successful(())
+ }
+ } else if (operation == "invoke") {
+ get(entityStore, entityName.toDocId, DocRevision.empty, true)
+ .flatMap { whiskAction =>
+ val currentPermissions = whiskAction.annotations
+ .get(WhiskAction.permissionsFieldName)
+ .map(value => value.convertTo[String])
+ .getOrElse(WhiskAction.defaultPermissions)
+
+ // the user who is owner by default
+ var currentExecutePermission = currentPermissions.charAt(2)
+ var errorInfo = s"have no permission to ${operation} this action"
+ if (user.namespace.name.asString != entityName.path.root.asString) { // the user who invoke the shared action
+ currentExecutePermission = currentPermissions.charAt(5)
+ errorInfo = s"have no permission to ${operation} this shared action"
+ }
+ if (currentExecutePermission == '-') { //have no permission
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ } else {
+ Future.successful(())
+ }
+ }
+ .recoverWith {
+ case t: RejectRequest =>
+ Future.failed(t)
+ case _ =>
+ Future.successful(())
+ }
+ } else { // download the code
+ get(entityStore, entityName.toDocId, DocRevision.empty, true)
+ .flatMap { whiskAction =>
+ val currentPermissions = whiskAction.annotations
+ .get(WhiskAction.permissionsFieldName)
+ .map(value => value.convertTo[String])
+ .getOrElse(WhiskAction.defaultPermissions)
+
+ val errorInfo = s"have no permission to download this shared action"
+ val currentDownloadPermission = currentPermissions.charAt(3)
Review comment:
It is for `the shared user`, so currentPermissions.charAt(3)
If the user is owner, the owner has download permission on any situation
hm...i will optimize the codes like below
```
if (user.namespace.name.asString != entityName.path.root.asString) { // the shared user who download the action
val errorInfo = s"have no permission to download this shared action"
val downloadPermissionOfSharedUser = currentPermissions.charAt(3)
if (downloadPermissionOfSharedUser == '-') {
Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
} else {
Future.successful(())
}
} else {
// the owner has download permission on any situation
Future.successful(())
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang closed pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang closed pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang commented on pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang commented on pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#issuecomment-858366875
@style95 ,
Current system permission already existed design solution:
* the owner have any permissions(read/update/execute) for this own actions.
* if a package is binded to a `shared` package, all users except the owner(we call it shared users) just have read/execute permission for that shared package's actions but do not have update permission.
* if package is not binded to any package, all users(except the owner) do not have any permissions(e.g. read/update/execute) for that package's actions
New design solution: https://github.com/apache/openwhisk/pull/4058#issuecomment-851116337
The difference between the new design solution and `the current system design` is:
* the new design solution can control every shared user's permission
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang edited a comment on pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang edited a comment on pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#issuecomment-626607066
Hi, guys, i push a commit here in this patch, just check my direction(permission unix sytle) is whether right. https://github.com/apache/openwhisk/pull/4058/commits/d77a80fd0348d05f3733f95dbc0e016429e96753
After check in my local, i found that, there has some differences between our own permission management between unix permission management.
### 1. user type is different
for one action, just have 2 type users.
* the action's owner
* the user (not the owner) who used the shared action directly(eg. get, invoke)
As we all already know, unix's permission has `3` user types, this is the different point.
### 2. our own permission value is restricted
* for one action, anyway, the user(not the owner) can't modify/delete the action on any situation, just the owner has the `write` permission.
* the owner's permission can affect the permission as well, e.g. if the owner is not given `read` permission, i think the user(not the owner) can't have `read` permission as well. if the owner is not give `execute` permission, the user(not the owner) can't have `execute` permission as well.
How do you guys think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] style95 commented on a change in pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
style95 commented on a change in pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#discussion_r637704803
##########
File path: common/scala/src/main/scala/org/apache/openwhisk/core/entity/WhiskAction.scala
##########
@@ -351,6 +361,49 @@ object WhiskAction extends DocumentFactory[WhiskAction] with WhiskEntityQueries[
val execFieldName = "exec"
val requireWhiskAuthHeader = "x-require-whisk-auth"
+ // annotation permission key name
+ val permissionsFieldName = "permissions"
Review comment:
I think we can store permission information in DB and fetch it along with `Identity` information.
Some proper tools to manage permission are required and the same change should be applied in CLI(`wsk` or at least `wskadmin`) as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang commented on a change in pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang commented on a change in pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#discussion_r641189638
##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/entitlement/Entitlement.scala
##########
@@ -231,6 +234,157 @@ protected[core] abstract class EntitlementProvider(
.getOrElse(Future.successful(()))
}
+ /**
+ * Checks if action operation(get/write/execute) whether feasible
+ *
+ * @param operation the action operation, e.g. get/write/execute
+ * @param user the user who get/write/execute the action
+ * @param entityStore store to write the action to
+ * @param entityName entityName
+ * @param permissions the passed permission code
+ * @return a promise that completes with success iff action operation is feasible
+ */
+ protected[core] def checkActionPermissions(
+ operation: String,
+ user: Identity,
+ entityStore: ArtifactStore[WhiskEntity],
+ entityName: FullyQualifiedEntityName,
+ get: (ArtifactStore[WhiskEntity], DocId, DocRevision, Boolean) => Future[WhiskAction],
+ permissions: Option[String] = None)(implicit transid: TransactionId): Future[Unit] = {
+ if (operation == "create") {
+ permissions
+ .map { value =>
+ if (WhiskAction.permissionList.contains(value)) {
+ Future.successful(())
+ } else {
+ val errorInfo =
+ s"give error permission code: ${value}, available permission is in ${WhiskAction.permissionList}"
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ }
+ }
+ .getOrElse(Future.successful(()))
+ } else if (operation == "update") {
+ get(entityStore, entityName.toDocId, DocRevision.empty, true)
+ .flatMap { whiskAction =>
+ val currentPermissions = whiskAction.annotations
+ .get(WhiskAction.permissionsFieldName)
+ .map(value => value.convertTo[String])
+ .getOrElse(WhiskAction.defaultPermissions)
+
+ val errorInfo = s"have no permission to ${operation} this action"
+ permissions match {
+ case Some(value) =>
+ if (!WhiskAction.permissionList.contains(value)) {
+ val errorInfo =
+ s"give error permission code: ${value}, available permission is in ${WhiskAction.permissionList}"
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ } else {
+ val passedUpdatePermission = value.charAt(1)
+ if (passedUpdatePermission == 'w') { // make it to modifiable
+ Future.successful(())
Review comment:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang edited a comment on pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang edited a comment on pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#issuecomment-851116337
According to review comments
It seems there has another option to do `granular permission management`
#### 1. There is no need to add permissions controls for action's "owner" since he/she can anyway update/remove/invoke his/her action
#### 2. It is necessary to control other shared user's permission (e.g. whether has permission to read/download the codes, whether has permission to execute the codes), this separate document in db: `xxx_permission` may like below
```
{
"_id": "whisk.system/helloAction",
"_rev": "1-1a14818c1083bc12b6d3fd1774fcc956",
"default": {
{
"read" : false
"execute": false
}
}
"permission": {
"theSharedUserA": {
"read" : yes
"execute": yes
},
"theSharedUserB": {
"read" : yes
"execute": false
}
}
```
Regarding above permission document of `whisk.system/helloAction`
* when create action: `whisk.system/helloAction`, the owner can set default read/execute permission for all shared user. e.g.
```
"default": {
{
"read" : false
"execute": false
}
}
```
* the shared user will inherits the default permissions if not explicitly specified in above document. e.g.
not explicitly specified for theSharedUserC, so theSharedUserC will have default permission
* if explicitly specified in above document for the shared user, the shared user will have specified permission, e.g.
already explicitly specified for theSharedUserA, so theSharedUserA's permission is
```
"theSharedUserA": {
"read" : yes
"execute": yes
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] style95 commented on a change in pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
style95 commented on a change in pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#discussion_r637705135
##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/entitlement/Entitlement.scala
##########
@@ -231,6 +234,157 @@ protected[core] abstract class EntitlementProvider(
.getOrElse(Future.successful(()))
}
+ /**
+ * Checks if action operation(get/write/execute) whether feasible
+ *
+ * @param operation the action operation, e.g. get/write/execute
+ * @param user the user who get/write/execute the action
+ * @param entityStore store to write the action to
+ * @param entityName entityName
+ * @param permissions the passed permission code
+ * @return a promise that completes with success iff action operation is feasible
+ */
+ protected[core] def checkActionPermissions(
+ operation: String,
+ user: Identity,
+ entityStore: ArtifactStore[WhiskEntity],
+ entityName: FullyQualifiedEntityName,
+ get: (ArtifactStore[WhiskEntity], DocId, DocRevision, Boolean) => Future[WhiskAction],
+ permissions: Option[String] = None)(implicit transid: TransactionId): Future[Unit] = {
+ if (operation == "create") {
+ permissions
+ .map { value =>
+ if (WhiskAction.permissionList.contains(value)) {
+ Future.successful(())
+ } else {
+ val errorInfo =
+ s"give error permission code: ${value}, available permission is in ${WhiskAction.permissionList}"
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ }
+ }
+ .getOrElse(Future.successful(()))
+ } else if (operation == "update") {
+ get(entityStore, entityName.toDocId, DocRevision.empty, true)
+ .flatMap { whiskAction =>
+ val currentPermissions = whiskAction.annotations
+ .get(WhiskAction.permissionsFieldName)
Review comment:
We can depend on permission data in DB rather than relying on action fields.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang commented on a change in pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang commented on a change in pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#discussion_r641189638
##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/entitlement/Entitlement.scala
##########
@@ -231,6 +234,157 @@ protected[core] abstract class EntitlementProvider(
.getOrElse(Future.successful(()))
}
+ /**
+ * Checks if action operation(get/write/execute) whether feasible
+ *
+ * @param operation the action operation, e.g. get/write/execute
+ * @param user the user who get/write/execute the action
+ * @param entityStore store to write the action to
+ * @param entityName entityName
+ * @param permissions the passed permission code
+ * @return a promise that completes with success iff action operation is feasible
+ */
+ protected[core] def checkActionPermissions(
+ operation: String,
+ user: Identity,
+ entityStore: ArtifactStore[WhiskEntity],
+ entityName: FullyQualifiedEntityName,
+ get: (ArtifactStore[WhiskEntity], DocId, DocRevision, Boolean) => Future[WhiskAction],
+ permissions: Option[String] = None)(implicit transid: TransactionId): Future[Unit] = {
+ if (operation == "create") {
+ permissions
+ .map { value =>
+ if (WhiskAction.permissionList.contains(value)) {
+ Future.successful(())
+ } else {
+ val errorInfo =
+ s"give error permission code: ${value}, available permission is in ${WhiskAction.permissionList}"
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ }
+ }
+ .getOrElse(Future.successful(()))
+ } else if (operation == "update") {
+ get(entityStore, entityName.toDocId, DocRevision.empty, true)
+ .flatMap { whiskAction =>
+ val currentPermissions = whiskAction.annotations
+ .get(WhiskAction.permissionsFieldName)
+ .map(value => value.convertTo[String])
+ .getOrElse(WhiskAction.defaultPermissions)
+
+ val errorInfo = s"have no permission to ${operation} this action"
+ permissions match {
+ case Some(value) =>
+ if (!WhiskAction.permissionList.contains(value)) {
+ val errorInfo =
+ s"give error permission code: ${value}, available permission is in ${WhiskAction.permissionList}"
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ } else {
+ val passedUpdatePermission = value.charAt(1)
+ if (passedUpdatePermission == 'w') { // make it to modifiable
+ Future.successful(())
Review comment:
hm...it seems need to change the permission firstly.
after changed the write permission to available, it can be modified again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang commented on a change in pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang commented on a change in pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#discussion_r504412314
##########
File path: common/scala/src/main/scala/org/apache/openwhisk/core/entity/WhiskAction.scala
##########
@@ -351,6 +361,49 @@ object WhiskAction extends DocumentFactory[WhiskAction] with WhiskEntityQueries[
val execFieldName = "exec"
val requireWhiskAuthHeader = "x-require-whisk-auth"
+ // annotation permission key name
+ val permissionsFieldName = "permissions"
+
+ val defaultPermissions = "rwxr-x"
+
+ // notes on users, just have 2 type users,
+ // 1. the action's owner
+ // 2. the user (not the owner) who used the shared action directly(e.g. get, invoke), we call it "the shared user"
+ //
+ // Notes on permission control
+ // 1. the owner has read(or download) permission on any situation, but for the shared user,
+ // in spite of has read permission on any situation, but can set it undownloadable or downloadable
+ // 2. the shared user can't update/delete the action on any situation.
+ // 3. the owner's permission can affect the shared user's permission, e.g
+ // if the owner is not given execute permission, the shared user can't have execute permission as well.
+ //
+ // Notes on permission values, include below permission value
+ // 1. permission code:rwxr-x: owner:read(yes)/write(yes)/execute(yes)|the shared action's user:download(yes)/write(no)/execute(yes), this is default
+ // 2. permission code:rwxr--: owner:read(yes)/write(yes)/execute(yes)|the shared action's user:download(yes)/write(no)/execute(no)
+ // 3. permission code:r-xr-x: owner:read(yes)/write(no)/execute(yes)|the shared action's user:download(yes)/write(no)/execute(yes)
+ // 4. permission code:r-xr--: owner:read(yes)/write(no)/execute(yes)|the shared action's user:download(yes)/write(no)/execute(no)
+ // 5. permission code:r--r--: owner:read(yes)/write(no)/execute(no)|the shared action's user:download(yes)/write(no)/execute(no)
+ // 6. permission code:rw-r--: owner:read(yes)/write(yes)/execute(no)|the shared action's user:download(yes)/write(no)/execute(no)
+ // 7. permission code:rwx--x: owner:read(yes)/write(yes)/execute(yes)|the shared action's user:download(no)/write(no)/execute(yes)
+ // 8. permission code:rwx---: owner:read(yes)/write(yes)/execute(yes)|the shared action's user:download(no)/write(no)/execute(no)
+ // 9. permission code:r-x--x: owner:read(yes)/write(no)/execute(yes)|the shared action's user:download(no)/write(no)/execute(yes)
+ // 10. permission code:r-x---: owner:read(yes)/write(no)/execute(yes)|the shared action's user:download(no)/write(no)/execute(no)
+ // 11. permission code:r-----: owner:read(yes)/write(no)/execute(no)|the shared action's user:download(no)/write(no)/execute(no)
+ // 12. permission code:rw----: owner:read(yes)/write(yes)/execute(no)|the shared action's user:download(no)/write(no)/execute(no)
+ val permissionList = List(
+ defaultPermissions,
+ "rwxr--",
+ "r-xr-x",
+ "r-xr--",
+ "r--r--",
Review comment:
Yes, there has an case that the owner doesn't have permission to update or execute the action, e. g. the annotation of permission for that action is: `r--r--`
in this case, if the owner wants to update the action codes, need to modify the action's permissions annotation to executeable, e.g. `wsk -i action update $action --annotation permissions rw-r--`, then, user can update their action now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang commented on a change in pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang commented on a change in pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#discussion_r641189638
##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/entitlement/Entitlement.scala
##########
@@ -231,6 +234,157 @@ protected[core] abstract class EntitlementProvider(
.getOrElse(Future.successful(()))
}
+ /**
+ * Checks if action operation(get/write/execute) whether feasible
+ *
+ * @param operation the action operation, e.g. get/write/execute
+ * @param user the user who get/write/execute the action
+ * @param entityStore store to write the action to
+ * @param entityName entityName
+ * @param permissions the passed permission code
+ * @return a promise that completes with success iff action operation is feasible
+ */
+ protected[core] def checkActionPermissions(
+ operation: String,
+ user: Identity,
+ entityStore: ArtifactStore[WhiskEntity],
+ entityName: FullyQualifiedEntityName,
+ get: (ArtifactStore[WhiskEntity], DocId, DocRevision, Boolean) => Future[WhiskAction],
+ permissions: Option[String] = None)(implicit transid: TransactionId): Future[Unit] = {
+ if (operation == "create") {
+ permissions
+ .map { value =>
+ if (WhiskAction.permissionList.contains(value)) {
+ Future.successful(())
+ } else {
+ val errorInfo =
+ s"give error permission code: ${value}, available permission is in ${WhiskAction.permissionList}"
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ }
+ }
+ .getOrElse(Future.successful(()))
+ } else if (operation == "update") {
+ get(entityStore, entityName.toDocId, DocRevision.empty, true)
+ .flatMap { whiskAction =>
+ val currentPermissions = whiskAction.annotations
+ .get(WhiskAction.permissionsFieldName)
+ .map(value => value.convertTo[String])
+ .getOrElse(WhiskAction.defaultPermissions)
+
+ val errorInfo = s"have no permission to ${operation} this action"
+ permissions match {
+ case Some(value) =>
+ if (!WhiskAction.permissionList.contains(value)) {
+ val errorInfo =
+ s"give error permission code: ${value}, available permission is in ${WhiskAction.permissionList}"
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ } else {
+ val passedUpdatePermission = value.charAt(1)
+ if (passedUpdatePermission == 'w') { // make it to modifiable
+ Future.successful(())
Review comment:
It seems need to change the permission firstly.
after changed the write permission to available, it can be modified again.
hm..i will check you said whether right.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] codecov-commenter commented on pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#issuecomment-850276876
# [Codecov](https://codecov.io/gh/apache/openwhisk/pull/4058?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#4058](https://codecov.io/gh/apache/openwhisk/pull/4058?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (92b2a38) into [master](https://codecov.io/gh/apache/openwhisk/commit/b818c3b3e8bd3fa9ac7742d1b8c051ec09b76ae2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b818c3b) will **decrease** coverage by `1.82%`.
> The diff coverage is `90.62%`.
[](https://codecov.io/gh/apache/openwhisk/pull/4058?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #4058 +/- ##
==========================================
- Coverage 77.26% 75.44% -1.83%
==========================================
Files 219 226 +7
Lines 11205 11578 +373
Branches 487 498 +11
==========================================
+ Hits 8658 8735 +77
- Misses 2547 2843 +296
```
| [Impacted Files](https://codecov.io/gh/apache/openwhisk/pull/4058?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...cala/org/apache/openwhisk/http/ErrorResponse.scala](https://codecov.io/gh/apache/openwhisk/pull/4058/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2h0dHAvRXJyb3JSZXNwb25zZS5zY2FsYQ==) | `87.75% <ø> (ø)` | |
| [...pache/openwhisk/core/entitlement/Entitlement.scala](https://codecov.io/gh/apache/openwhisk/pull/4058/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9jb250cm9sbGVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvZW50aXRsZW1lbnQvRW50aXRsZW1lbnQuc2NhbGE=) | `83.67% <86.66%> (+1.85%)` | :arrow_up: |
| [...org/apache/openwhisk/core/controller/Actions.scala](https://codecov.io/gh/apache/openwhisk/pull/4058/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9jb250cm9sbGVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvY29udHJvbGxlci9BY3Rpb25zLnNjYWxh) | `90.90% <94.11%> (+0.12%)` | :arrow_up: |
| [...org/apache/openwhisk/core/entity/WhiskAction.scala](https://codecov.io/gh/apache/openwhisk/pull/4058/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvZW50aXR5L1doaXNrQWN0aW9uLnNjYWxh) | `93.06% <100.00%> (+0.71%)` | :arrow_up: |
| [...core/database/cosmosdb/RxObservableImplicits.scala](https://codecov.io/gh/apache/openwhisk/pull/4058/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvZGF0YWJhc2UvY29zbW9zZGIvUnhPYnNlcnZhYmxlSW1wbGljaXRzLnNjYWxh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ore/database/cosmosdb/cache/CacheInvalidator.scala](https://codecov.io/gh/apache/openwhisk/pull/4058/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9jb3Ntb3NkYi9jYWNoZS1pbnZhbGlkYXRvci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL29wZW53aGlzay9jb3JlL2RhdGFiYXNlL2Nvc21vc2RiL2NhY2hlL0NhY2hlSW52YWxpZGF0b3Iuc2NhbGE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...e/database/cosmosdb/cache/ChangeFeedConsumer.scala](https://codecov.io/gh/apache/openwhisk/pull/4058/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9jb3Ntb3NkYi9jYWNoZS1pbnZhbGlkYXRvci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL29wZW53aGlzay9jb3JlL2RhdGFiYXNlL2Nvc21vc2RiL2NhY2hlL0NoYW5nZUZlZWRDb25zdW1lci5zY2FsYQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...core/database/cosmosdb/CosmosDBArtifactStore.scala](https://codecov.io/gh/apache/openwhisk/pull/4058/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvZGF0YWJhc2UvY29zbW9zZGIvQ29zbW9zREJBcnRpZmFjdFN0b3JlLnNjYWxh) | `0.00% <0.00%> (-95.85%)` | :arrow_down: |
| [...sk/core/database/cosmosdb/CosmosDBViewMapper.scala](https://codecov.io/gh/apache/openwhisk/pull/4058/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvZGF0YWJhc2UvY29zbW9zZGIvQ29zbW9zREJWaWV3TWFwcGVyLnNjYWxh) | `0.00% <0.00%> (-93.90%)` | :arrow_down: |
| [...tabase/cosmosdb/cache/CacheInvalidatorConfig.scala](https://codecov.io/gh/apache/openwhisk/pull/4058/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9jb3Ntb3NkYi9jYWNoZS1pbnZhbGlkYXRvci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL29wZW53aGlzay9jb3JlL2RhdGFiYXNlL2Nvc21vc2RiL2NhY2hlL0NhY2hlSW52YWxpZGF0b3JDb25maWcuc2NhbGE=) | `0.00% <0.00%> (-92.31%)` | :arrow_down: |
| ... and [44 more](https://codecov.io/gh/apache/openwhisk/pull/4058/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/openwhisk/pull/4058?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/openwhisk/pull/4058?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [b818c3b...92b2a38](https://codecov.io/gh/apache/openwhisk/pull/4058?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang edited a comment on pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang edited a comment on pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#issuecomment-851116337
According to review comments
It seems there has another option to do `granular permission management`
#### 1. There is no need to add permissions controls for action's "owner" since he/she can anyway update/remove/invoke his/her action
#### 2. It is necessary to control other shared user's permission (e.g. whether has permission to read/download the codes, whether has permission to execute the codes), this separate document in db: `xxx_permission` may like below
```
{
"_id": "whisk.system/helloAction",
"_rev": "1-1a14818c1083bc12b6d3fd1774fcc956",
"default": {
{
"read" : false
"execute": false
}
}
"permission": {
"theSharedUserA": {
"read" : yes
"execute": yes
},
"theSharedUserB": {
"read" : yes
"execute": false
}
}
```
Regarding above permission document of `whisk.system/helloAction`
* when create action: `whisk.system/helloAction`, the owner can set default read/execute permission. e.g.
```
"default": {
{
"read" : false
"execute": false
}
}
```
* the shared user will inherits the default permissions if not explicitly specified in above document. e.g.
not explicitly specified for theSharedUserC, so theSharedUserC will have default permission
* if explicitly specified in above document for the shared user, the shared user will have, e.g.
already explicitly specified for theSharedUserA, so theSharedUserA's permission is
```
"theSharedUserA": {
"read" : yes
"execute": yes
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang commented on a change in pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang commented on a change in pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#discussion_r641184253
##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/entitlement/Entitlement.scala
##########
@@ -231,6 +234,157 @@ protected[core] abstract class EntitlementProvider(
.getOrElse(Future.successful(()))
}
+ /**
+ * Checks if action operation(get/write/execute) whether feasible
+ *
+ * @param operation the action operation, e.g. get/write/execute
+ * @param user the user who get/write/execute the action
+ * @param entityStore store to write the action to
+ * @param entityName entityName
+ * @param permissions the passed permission code
+ * @return a promise that completes with success iff action operation is feasible
+ */
+ protected[core] def checkActionPermissions(
+ operation: String,
+ user: Identity,
+ entityStore: ArtifactStore[WhiskEntity],
+ entityName: FullyQualifiedEntityName,
+ get: (ArtifactStore[WhiskEntity], DocId, DocRevision, Boolean) => Future[WhiskAction],
+ permissions: Option[String] = None)(implicit transid: TransactionId): Future[Unit] = {
+ if (operation == "create") {
+ permissions
+ .map { value =>
+ if (WhiskAction.permissionList.contains(value)) {
+ Future.successful(())
+ } else {
+ val errorInfo =
+ s"give error permission code: ${value}, available permission is in ${WhiskAction.permissionList}"
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ }
+ }
+ .getOrElse(Future.successful(()))
+ } else if (operation == "update") {
+ get(entityStore, entityName.toDocId, DocRevision.empty, true)
+ .flatMap { whiskAction =>
+ val currentPermissions = whiskAction.annotations
+ .get(WhiskAction.permissionsFieldName)
+ .map(value => value.convertTo[String])
+ .getOrElse(WhiskAction.defaultPermissions)
+
+ val errorInfo = s"have no permission to ${operation} this action"
+ permissions match {
+ case Some(value) =>
+ if (!WhiskAction.permissionList.contains(value)) {
+ val errorInfo =
+ s"give error permission code: ${value}, available permission is in ${WhiskAction.permissionList}"
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ } else {
+ val passedUpdatePermission = value.charAt(1)
+ if (passedUpdatePermission == 'w') { // make it to modifiable
+ Future.successful(())
+ } else {
+ val currentUpdatePermission = currentPermissions.charAt(1)
+ if (currentUpdatePermission == '-') {
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ } else {
+ Future.successful(())
+ }
+ }
+ }
+ case None =>
+ val currentUpdatePermission = currentPermissions.charAt(1)
+ if (currentUpdatePermission == '-') {
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ } else {
+ Future.successful(())
+ }
+ }
+ }
+ .recoverWith {
+ case t: RejectRequest =>
+ Future.failed(t)
+ case _ =>
+ Future.successful(())
+ }
+ } else if (operation == "remove") {
+ get(entityStore, entityName.toDocId, DocRevision.empty, true)
+ .flatMap { whiskAction =>
+ val currentPermissions = whiskAction.annotations
+ .get(WhiskAction.permissionsFieldName)
+ .map(value => value.convertTo[String])
+ .getOrElse(WhiskAction.defaultPermissions)
+
+ val currentUpdatePermission = currentPermissions.charAt(1)
+ if (currentUpdatePermission == '-') {
+ val errorInfo = s"have no permission to ${operation} this action"
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ } else {
+ Future.successful(())
+ }
+ }
+ .recoverWith {
+ case t: RejectRequest =>
+ Future.failed(t)
+ case _ =>
+ Future.successful(())
+ }
+ } else if (operation == "invoke") {
+ get(entityStore, entityName.toDocId, DocRevision.empty, true)
+ .flatMap { whiskAction =>
+ val currentPermissions = whiskAction.annotations
+ .get(WhiskAction.permissionsFieldName)
+ .map(value => value.convertTo[String])
+ .getOrElse(WhiskAction.defaultPermissions)
+
+ // the user who is owner by default
+ var currentExecutePermission = currentPermissions.charAt(2)
+ var errorInfo = s"have no permission to ${operation} this action"
+ if (user.namespace.name.asString != entityName.path.root.asString) { // the user who invoke the shared action
+ currentExecutePermission = currentPermissions.charAt(5)
+ errorInfo = s"have no permission to ${operation} this shared action"
+ }
+ if (currentExecutePermission == '-') { //have no permission
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ } else {
+ Future.successful(())
+ }
+ }
+ .recoverWith {
+ case t: RejectRequest =>
+ Future.failed(t)
+ case _ =>
+ Future.successful(())
+ }
+ } else { // download the code
+ get(entityStore, entityName.toDocId, DocRevision.empty, true)
+ .flatMap { whiskAction =>
+ val currentPermissions = whiskAction.annotations
+ .get(WhiskAction.permissionsFieldName)
+ .map(value => value.convertTo[String])
+ .getOrElse(WhiskAction.defaultPermissions)
+
+ val errorInfo = s"have no permission to download this shared action"
+ val currentDownloadPermission = currentPermissions.charAt(3)
Review comment:
It is for `the shared user`, so currentPermissions.charAt(3) is right.
If the user is owner, the owner has download permission on any situation
hm...anyway, you said problem is also a problem, it is not readable. i will optimize the codes like below
```
if (user.namespace.name.asString != entityName.path.root.asString) { // the shared user who download the action
val errorInfo = s"have no permission to download this shared action"
val downloadPermissionOfSharedUser = currentPermissions.charAt(3)
if (downloadPermissionOfSharedUser == '-') {
Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
} else {
Future.successful(())
}
} else {
// the owner has download permission on any situation
Future.successful(())
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] jiangpengcheng commented on pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
jiangpengcheng commented on pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#issuecomment-848429860
one of my thought is that there is no need to add permissions controls for action's "owner" since he/she can anyway update/remove/invoke his/her action
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang edited a comment on pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang edited a comment on pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#issuecomment-850162422
After read you guys review suggestion, 2 important points
* For the permission data storage, we can depend on permission data in DB rather than relying on action fields.
Actually i don't know how to design this(e.g. how to storage and where to storage) and make it more better.
* one of my thought is that there is no need to add permissions controls for action's "owner" since he/she can anyway update/remove/invoke his/her action
It seems just need to control the permission for the shared user. (e.g. the shared user whether has permission to download code, execute the codes)
But for this pr, our initial target is `avoid the owner update or delete actions by mistake`, it seems if don't add permission to action's owner, can't reach the initial target
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang edited a comment on pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang edited a comment on pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#issuecomment-850162422
After read you guys review suggestion, 2 important points
* For the permission data storage, we can depend on permission data in DB rather than relying on action fields.
Actually i don't know how to design this and make it more better.
* one of my thought is that there is no need to add permissions controls for action's "owner" since he/she can anyway update/remove/invoke his/her action
It seems just need to control the permission for the shared user. (e.g. the shared user whether has permission to download code, execute the codes)
But for this pr, our initial target is `avoid the owner update or delete actions by mistake`, it seems if don't add permission to action's owner, can't reach the initial target
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang commented on a change in pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang commented on a change in pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#discussion_r504413860
##########
File path: common/scala/src/main/scala/org/apache/openwhisk/core/entity/WhiskAction.scala
##########
@@ -351,6 +361,49 @@ object WhiskAction extends DocumentFactory[WhiskAction] with WhiskEntityQueries[
val execFieldName = "exec"
val requireWhiskAuthHeader = "x-require-whisk-auth"
+ // annotation permission key name
+ val permissionsFieldName = "permissions"
+
+ val defaultPermissions = "rwxr-x"
+
+ // notes on users, just have 2 type users,
Review comment:
Updated accordingly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang edited a comment on pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang edited a comment on pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#issuecomment-626607066
Hi, guys, i push a commit here in this patch: https://github.com/apache/openwhisk/pull/4058/commits/d77a80fd0348d05f3733f95dbc0e016429e96753
Just check whether my direction(permission unix sytle) is right.
After check in my local, i found that, we can use some parts of unix-sytle. e.g.
for one action, just have 2 type users.
* the action's owner
* the user (not the owner) who used the shared action directly(eg. get, invoke)
As we all already know, unix's permission has `3` user types, this is the different point.
Another different point the permission values is restricted
* for one action, anyway, the user(not the owner) can't write the action, just the owner has the write permission.
* the owner's permission can affect the permission as well, e.g. if the owner is not given `read` permission, i think the user(not the owner) can't have `read` permission as well. if the owner is not give `execute` permission, the user(not the owner) can't have `execute` permission as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] style95 commented on a change in pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
style95 commented on a change in pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#discussion_r639393933
##########
File path: common/scala/src/main/scala/org/apache/openwhisk/core/entity/WhiskAction.scala
##########
@@ -351,6 +361,49 @@ object WhiskAction extends DocumentFactory[WhiskAction] with WhiskEntityQueries[
val execFieldName = "exec"
val requireWhiskAuthHeader = "x-require-whisk-auth"
+ // annotation permission key name
+ val permissionsFieldName = "permissions"
Review comment:
I meant, we can fetch the permission information in a similar way with `Identity`.
This information can be stored in the cache too.
Permission needs to be checked before actual operations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang commented on a change in pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang commented on a change in pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#discussion_r484716259
##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/entitlement/Entitlement.scala
##########
@@ -231,6 +234,157 @@ protected[core] abstract class EntitlementProvider(
.getOrElse(Future.successful(()))
}
+ /**
+ * Checks if action operation(get/write/execute) whether feasible
+ *
+ * @param operation the action operation, e.g. get/write/execute
+ * @param user the user who get/write/execute the action
+ * @param entityStore store to write the action to
+ * @param entityName entityName
+ * @param permissions the passed permission code
+ * @return a promise that completes with success iff action operation is feasible
+ */
+ protected[core] def checkActionPermissions(
+ operation: String,
+ user: Identity,
+ entityStore: ArtifactStore[WhiskEntity],
+ entityName: FullyQualifiedEntityName,
+ get: (ArtifactStore[WhiskEntity], DocId, DocRevision, Boolean) => Future[WhiskAction],
+ permissions: Option[String] = None)(implicit transid: TransactionId): Future[Unit] = {
+ if (operation == "create") {
+ permissions
+ .map { value =>
+ if (WhiskAction.permissionList.contains(value)) {
+ Future.successful(())
+ } else {
+ val errorInfo =
+ s"give error permission code: ${value}, available permission is in ${WhiskAction.permissionList}"
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ }
+ }
+ .getOrElse(Future.successful(()))
+ } else if (operation == "update") {
+ get(entityStore, entityName.toDocId, DocRevision.empty, true)
+ .flatMap { whiskAction =>
+ val currentPermissions = whiskAction.annotations
+ .get(WhiskAction.permissionsFieldName)
+ .map(value => value.convertTo[String])
+ .getOrElse(WhiskAction.defaultPermissions)
+
+ val errorInfo = s"have no permission to ${operation} this action"
+ permissions match {
+ case Some(value) =>
+ if (!WhiskAction.permissionList.contains(value)) {
+ val errorInfo =
+ s"give error permission code: ${value}, available permission is in ${WhiskAction.permissionList}"
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ } else {
+ val passedUpdatePermission = value.charAt(1)
+ if (passedUpdatePermission == 'w') { // make it to modifiable
+ Future.successful(())
+ } else {
+ val currentUpdatePermission = currentPermissions.charAt(1)
+ if (currentUpdatePermission == '-') {
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ } else {
+ Future.successful(())
+ }
+ }
+ }
+ case None =>
+ val currentUpdatePermission = currentPermissions.charAt(1)
+ if (currentUpdatePermission == '-') {
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ } else {
+ Future.successful(())
+ }
+ }
+ }
+ .recoverWith {
+ case t: RejectRequest =>
+ Future.failed(t)
+ case _ =>
+ Future.successful(())
Review comment:
In order to make some test cases successfully .e.g
```
./gradlew :tests:test --tests="org.apache.openwhisk.core.controller.test.PackageActionsApiTests"
./gradlew :tests:test --tests="org.apache.openwhisk.core.controller.test.ActionsApiTests"
```
Need to add above recoverWith logic, concrete content is
* if the excpetion is `RejectRequest`, return `Future.failed(t)`
make normal logic passed
* For some excpetion which generated by test above cases, should return `Future.successful(())`
Make above tests cases passed, e.g. https://github.com/apache/openwhisk/blob/master/tests/src/test/scala/org/apache/openwhisk/core/controller/test/PackageActionsApiTests.scala#L130
here, it will pass a package paramter to `get(entityStore, entityName.toDocId, DocRevision.empty, true)`'s second param, actually, this method needs `action`, so will throw NoDocument exception, in order to make the test case passed, need to make it return `Future.successful(())`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang commented on a change in pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang commented on a change in pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#discussion_r641297320
##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/entitlement/Entitlement.scala
##########
@@ -231,6 +234,157 @@ protected[core] abstract class EntitlementProvider(
.getOrElse(Future.successful(()))
}
+ /**
+ * Checks if action operation(get/write/execute) whether feasible
+ *
+ * @param operation the action operation, e.g. get/write/execute
+ * @param user the user who get/write/execute the action
+ * @param entityStore store to write the action to
+ * @param entityName entityName
+ * @param permissions the passed permission code
+ * @return a promise that completes with success iff action operation is feasible
+ */
+ protected[core] def checkActionPermissions(
+ operation: String,
+ user: Identity,
+ entityStore: ArtifactStore[WhiskEntity],
+ entityName: FullyQualifiedEntityName,
+ get: (ArtifactStore[WhiskEntity], DocId, DocRevision, Boolean) => Future[WhiskAction],
+ permissions: Option[String] = None)(implicit transid: TransactionId): Future[Unit] = {
+ if (operation == "create") {
Review comment:
Updated accordingly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang commented on a change in pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang commented on a change in pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#discussion_r641189638
##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/entitlement/Entitlement.scala
##########
@@ -231,6 +234,157 @@ protected[core] abstract class EntitlementProvider(
.getOrElse(Future.successful(()))
}
+ /**
+ * Checks if action operation(get/write/execute) whether feasible
+ *
+ * @param operation the action operation, e.g. get/write/execute
+ * @param user the user who get/write/execute the action
+ * @param entityStore store to write the action to
+ * @param entityName entityName
+ * @param permissions the passed permission code
+ * @return a promise that completes with success iff action operation is feasible
+ */
+ protected[core] def checkActionPermissions(
+ operation: String,
+ user: Identity,
+ entityStore: ArtifactStore[WhiskEntity],
+ entityName: FullyQualifiedEntityName,
+ get: (ArtifactStore[WhiskEntity], DocId, DocRevision, Boolean) => Future[WhiskAction],
+ permissions: Option[String] = None)(implicit transid: TransactionId): Future[Unit] = {
+ if (operation == "create") {
+ permissions
+ .map { value =>
+ if (WhiskAction.permissionList.contains(value)) {
+ Future.successful(())
+ } else {
+ val errorInfo =
+ s"give error permission code: ${value}, available permission is in ${WhiskAction.permissionList}"
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ }
+ }
+ .getOrElse(Future.successful(()))
+ } else if (operation == "update") {
+ get(entityStore, entityName.toDocId, DocRevision.empty, true)
+ .flatMap { whiskAction =>
+ val currentPermissions = whiskAction.annotations
+ .get(WhiskAction.permissionsFieldName)
+ .map(value => value.convertTo[String])
+ .getOrElse(WhiskAction.defaultPermissions)
+
+ val errorInfo = s"have no permission to ${operation} this action"
+ permissions match {
+ case Some(value) =>
+ if (!WhiskAction.permissionList.contains(value)) {
+ val errorInfo =
+ s"give error permission code: ${value}, available permission is in ${WhiskAction.permissionList}"
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ } else {
+ val passedUpdatePermission = value.charAt(1)
+ if (passedUpdatePermission == 'w') { // make it to modifiable
+ Future.successful(())
Review comment:
Yes, you are right. e.g.
```
# wsk -i action create hello ~/hello.js --annotation permissions r-xr-x
ok: created action hello
# wsk -i action update hello ~/hello.js
error: Unable to create action 'hello': have no permission to update this action (code iAKv0FTZLaURFtLzoSzryZhLpemxDell)
# wsk -i action update hello ~/hello.js --annotation permissions rwxr-x
ok: updated action hello
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] jiangpengcheng commented on a change in pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
jiangpengcheng commented on a change in pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#discussion_r639368738
##########
File path: common/scala/src/main/scala/org/apache/openwhisk/core/entity/WhiskAction.scala
##########
@@ -351,6 +361,49 @@ object WhiskAction extends DocumentFactory[WhiskAction] with WhiskEntityQueries[
val execFieldName = "exec"
val requireWhiskAuthHeader = "x-require-whisk-auth"
+ // annotation permission key name
+ val permissionsFieldName = "permissions"
+
+ val defaultPermissions = "rwxr-x"
+
+ // notes on users, just have 2 type users,
+ // 1. the action's owner
+ // 2. the user (not the owner) who used the shared action directly(e.g. get, invoke), we call it "the shared user"
+ //
+ // Notes on permission control
+ // 1. the owner has read(or download) permission on any situation, but for the shared user,
+ // in spite of has read permission on any situation, but can set it undownloadable or downloadable
+ // 2. the shared user can't update/delete the action on any situation.
+ // 3. the owner's permission can affect the shared user's permission, e.g
+ // if the owner is not given execute permission, the shared user can't have execute permission as well.
+ //
+ // Notes on permission values, include below permission value
+ // 1. permission code:rwxr-x: owner:read(yes)/write(yes)/execute(yes)|the shared action's user:download(yes)/write(no)/execute(yes), this is default
+ // 2. permission code:rwxr--: owner:read(yes)/write(yes)/execute(yes)|the shared action's user:download(yes)/write(no)/execute(no)
+ // 3. permission code:r-xr-x: owner:read(yes)/write(no)/execute(yes)|the shared action's user:download(yes)/write(no)/execute(yes)
+ // 4. permission code:r-xr--: owner:read(yes)/write(no)/execute(yes)|the shared action's user:download(yes)/write(no)/execute(no)
+ // 5. permission code:r--r--: owner:read(yes)/write(no)/execute(no)|the shared action's user:download(yes)/write(no)/execute(no)
+ // 6. permission code:rw-r--: owner:read(yes)/write(yes)/execute(no)|the shared action's user:download(yes)/write(no)/execute(no)
+ // 7. permission code:rwx--x: owner:read(yes)/write(yes)/execute(yes)|the shared action's user:download(no)/write(no)/execute(yes)
+ // 8. permission code:rwx---: owner:read(yes)/write(yes)/execute(yes)|the shared action's user:download(no)/write(no)/execute(no)
+ // 9. permission code:r-x--x: owner:read(yes)/write(no)/execute(yes)|the shared action's user:download(no)/write(no)/execute(yes)
+ // 10. permission code:r-x---: owner:read(yes)/write(no)/execute(yes)|the shared action's user:download(no)/write(no)/execute(no)
+ // 11. permission code:r-----: owner:read(yes)/write(no)/execute(no)|the shared action's user:download(no)/write(no)/execute(no)
+ // 12. permission code:rw----: owner:read(yes)/write(yes)/execute(no)|the shared action's user:download(no)/write(no)/execute(no)
+ val permissionList = List(
Review comment:
agree
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] rabbah commented on a change in pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
rabbah commented on a change in pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#discussion_r413020945
##########
File path: docs/actions.md
##########
@@ -598,6 +598,28 @@ You can clean up by deleting actions that you do not want to use.
actions
/guest/mySequence private sequence
```
+## Protect action updated or deleted by mistake
Review comment:
I will suggest some updates to this text once the API change and implementation are finalized.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang commented on pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang commented on pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#issuecomment-689235845
Already rebased and solved relative test cases error
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] jiangpengcheng commented on a change in pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
jiangpengcheng commented on a change in pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#discussion_r639368487
##########
File path: common/scala/src/main/scala/org/apache/openwhisk/core/entity/WhiskAction.scala
##########
@@ -351,6 +361,49 @@ object WhiskAction extends DocumentFactory[WhiskAction] with WhiskEntityQueries[
val execFieldName = "exec"
val requireWhiskAuthHeader = "x-require-whisk-auth"
+ // annotation permission key name
+ val permissionsFieldName = "permissions"
Review comment:
I think such permission information is applied to actions instead of `Identity`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang commented on pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang commented on pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#issuecomment-850162422
> one of my thought is that there is no need to add permissions controls for action's "owner" since he/she can anyway update/remove/invoke his/her action
It seems just need to control the permission for the shared user. (e.g. whether has permission to download code, execute the codes)
But for this pr, our initial target is `avoid the owner update or delete actions by mistake`, it seems if don't add permission to action's owner, can't reach the initial target
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang commented on a change in pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang commented on a change in pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#discussion_r641184253
##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/entitlement/Entitlement.scala
##########
@@ -231,6 +234,157 @@ protected[core] abstract class EntitlementProvider(
.getOrElse(Future.successful(()))
}
+ /**
+ * Checks if action operation(get/write/execute) whether feasible
+ *
+ * @param operation the action operation, e.g. get/write/execute
+ * @param user the user who get/write/execute the action
+ * @param entityStore store to write the action to
+ * @param entityName entityName
+ * @param permissions the passed permission code
+ * @return a promise that completes with success iff action operation is feasible
+ */
+ protected[core] def checkActionPermissions(
+ operation: String,
+ user: Identity,
+ entityStore: ArtifactStore[WhiskEntity],
+ entityName: FullyQualifiedEntityName,
+ get: (ArtifactStore[WhiskEntity], DocId, DocRevision, Boolean) => Future[WhiskAction],
+ permissions: Option[String] = None)(implicit transid: TransactionId): Future[Unit] = {
+ if (operation == "create") {
+ permissions
+ .map { value =>
+ if (WhiskAction.permissionList.contains(value)) {
+ Future.successful(())
+ } else {
+ val errorInfo =
+ s"give error permission code: ${value}, available permission is in ${WhiskAction.permissionList}"
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ }
+ }
+ .getOrElse(Future.successful(()))
+ } else if (operation == "update") {
+ get(entityStore, entityName.toDocId, DocRevision.empty, true)
+ .flatMap { whiskAction =>
+ val currentPermissions = whiskAction.annotations
+ .get(WhiskAction.permissionsFieldName)
+ .map(value => value.convertTo[String])
+ .getOrElse(WhiskAction.defaultPermissions)
+
+ val errorInfo = s"have no permission to ${operation} this action"
+ permissions match {
+ case Some(value) =>
+ if (!WhiskAction.permissionList.contains(value)) {
+ val errorInfo =
+ s"give error permission code: ${value}, available permission is in ${WhiskAction.permissionList}"
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ } else {
+ val passedUpdatePermission = value.charAt(1)
+ if (passedUpdatePermission == 'w') { // make it to modifiable
+ Future.successful(())
+ } else {
+ val currentUpdatePermission = currentPermissions.charAt(1)
+ if (currentUpdatePermission == '-') {
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ } else {
+ Future.successful(())
+ }
+ }
+ }
+ case None =>
+ val currentUpdatePermission = currentPermissions.charAt(1)
+ if (currentUpdatePermission == '-') {
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ } else {
+ Future.successful(())
+ }
+ }
+ }
+ .recoverWith {
+ case t: RejectRequest =>
+ Future.failed(t)
+ case _ =>
+ Future.successful(())
+ }
+ } else if (operation == "remove") {
+ get(entityStore, entityName.toDocId, DocRevision.empty, true)
+ .flatMap { whiskAction =>
+ val currentPermissions = whiskAction.annotations
+ .get(WhiskAction.permissionsFieldName)
+ .map(value => value.convertTo[String])
+ .getOrElse(WhiskAction.defaultPermissions)
+
+ val currentUpdatePermission = currentPermissions.charAt(1)
+ if (currentUpdatePermission == '-') {
+ val errorInfo = s"have no permission to ${operation} this action"
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ } else {
+ Future.successful(())
+ }
+ }
+ .recoverWith {
+ case t: RejectRequest =>
+ Future.failed(t)
+ case _ =>
+ Future.successful(())
+ }
+ } else if (operation == "invoke") {
+ get(entityStore, entityName.toDocId, DocRevision.empty, true)
+ .flatMap { whiskAction =>
+ val currentPermissions = whiskAction.annotations
+ .get(WhiskAction.permissionsFieldName)
+ .map(value => value.convertTo[String])
+ .getOrElse(WhiskAction.defaultPermissions)
+
+ // the user who is owner by default
+ var currentExecutePermission = currentPermissions.charAt(2)
+ var errorInfo = s"have no permission to ${operation} this action"
+ if (user.namespace.name.asString != entityName.path.root.asString) { // the user who invoke the shared action
+ currentExecutePermission = currentPermissions.charAt(5)
+ errorInfo = s"have no permission to ${operation} this shared action"
+ }
+ if (currentExecutePermission == '-') { //have no permission
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ } else {
+ Future.successful(())
+ }
+ }
+ .recoverWith {
+ case t: RejectRequest =>
+ Future.failed(t)
+ case _ =>
+ Future.successful(())
+ }
+ } else { // download the code
+ get(entityStore, entityName.toDocId, DocRevision.empty, true)
+ .flatMap { whiskAction =>
+ val currentPermissions = whiskAction.annotations
+ .get(WhiskAction.permissionsFieldName)
+ .map(value => value.convertTo[String])
+ .getOrElse(WhiskAction.defaultPermissions)
+
+ val errorInfo = s"have no permission to download this shared action"
+ val currentDownloadPermission = currentPermissions.charAt(3)
Review comment:
It seems it is for `the shared user`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang removed a comment on pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang removed a comment on pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#issuecomment-850162422
After read you guys review suggestion, 2 important points
* For the permission data storage, we can depend on permission data in DB rather than relying on action fields.
Actually i don't know how to design this(e.g. how to storage and where to storage) and make it more better.
* one of my thought is that there is no need to add permissions controls for action's "owner" since he/she can anyway update/remove/invoke his/her action
It seems just need to control the permission for the shared user. (e.g. the shared user whether has permission to download code, execute the codes)
But for this pr, our initial target is `avoid the owner update or delete actions by mistake`, it seems if don't add permission to action's owner, can't reach the initial target
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang commented on pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang commented on pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#issuecomment-851116337
According to review comments
It seems there has another option to do `granular permission management`
#### 1. There is no need to add permissions controls for action's "owner" since he/she can anyway update/remove/invoke his/her action
#### 2. It is necessary to control other shared user's permission (e.g. whether has permission to read/download the codes, whether has permission to execute the codes), this separate document may like below
```
{
"_id": "whisk.system/helloAction",
"_rev": "1-1a14818c1083bc12b6d3fd1774fcc956",
"default": {
{
"read" : false
"execute": false
}
}
"permission": {
"theSharedUserA": {
"read" : yes
"execute": yes
},
"theSharedUserB": {
"read" : yes
"execute": false
}
}
```
Regarding above permission document of `whisk.system/helloAction`
* when create action: `whisk.system/helloAction`, the owner can set default read/execute permission. e.g.
```
"default": {
{
"read" : false
"execute": false
}
}
```
* the shared user will inherits the default permissions if not explicitly specified in above document. e.g.
not explicitly specified for theSharedUserC, so theSharedUserC will have default permission
* if explicitly specified in above document for the shared user, the shared user will have, e.g.
already explicitly specified for theSharedUserA, so theSharedUserA's permission is
```
"theSharedUserA": {
"read" : yes
"execute": yes
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang edited a comment on pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang edited a comment on pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#issuecomment-626607066
Hi, guys, i push a commit here in this patch: https://github.com/apache/openwhisk/pull/4058/commits/d77a80fd0348d05f3733f95dbc0e016429e96753
Just check whether my direction(permission unix sytle) is right.
After check in my local, i found that, we can use some parts of unix-sytle. e.g.
for one action, just have 2 type users.
* the action's owner
* the user (not the owner) who used the shared action directly(eg. get, invoke)
As we all already know, unix's permission has `3` user types, this is the different point.
Another different point the permission values is restricted
* for one action, anyway, the user(not the owner) can't write the action, just the owner has the write permission.
* the owner's permission can affect the permission as well, e.g. if the owner is not given `read` permission, i think the user(not the owner) can't have `read` permission as well. if the owner is not give `execute` permission, the user(not the owner) can't have `execute` permission as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] rabbah commented on a change in pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
rabbah commented on a change in pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#discussion_r413017425
##########
File path: common/scala/src/main/scala/org/apache/openwhisk/core/entity/WhiskAction.scala
##########
@@ -55,7 +55,8 @@ case class WhiskActionPut(exec: Option[Exec] = None,
limits: Option[ActionLimitsOption] = None,
version: Option[SemVer] = None,
publish: Option[Boolean] = None,
- annotations: Option[Parameters] = None) {
+ annotations: Option[Parameters] = None,
+ unlock: Option[Boolean] = None) {
Review comment:
concretely my suggestion is to change `unlock: Option[Boolean]` to `permissions: Option[Int]` and use bit operations to toggle permissions. The first set of permissions to respect is whether the resource is locked locked or unlocked. This can be abstracted with utility methods.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang commented on pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang commented on pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#issuecomment-626607066
Hi, guys, i push a commit here in this patch: https://github.com/apache/openwhisk/pull/4058/commits/d77a80fd0348d05f3733f95dbc0e016429e96753
Just check whether my direction(permission unix sytle) is right.
After check in my local, i found that, we can use some parts of unix-sytle. e.g.
for one action, just have 2 type users.
* the action's owner
* if the action is shared, the user (not the owner) who used the action directly(eg. get, invoke)
As we all already know, unix's permission controller has `3` user types, this is the different point.
Another different point the permission values is restricted
* for one action, anyway, the user(not the owner) can't write the action, just the owner has the write permission.
* the owner's permission can affect the permission as well, e.g. if the owner is not given `read` permission, i think the user(not the owner) can't have `read` permission as well. if the owner is not give `execute` permission, the user(not the owner) can't have `execute` permission as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] jiangpengcheng commented on a change in pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
jiangpengcheng commented on a change in pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#discussion_r639377253
##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/entitlement/Entitlement.scala
##########
@@ -231,6 +234,157 @@ protected[core] abstract class EntitlementProvider(
.getOrElse(Future.successful(()))
}
+ /**
+ * Checks if action operation(get/write/execute) whether feasible
+ *
+ * @param operation the action operation, e.g. get/write/execute
+ * @param user the user who get/write/execute the action
+ * @param entityStore store to write the action to
+ * @param entityName entityName
+ * @param permissions the passed permission code
+ * @return a promise that completes with success iff action operation is feasible
+ */
+ protected[core] def checkActionPermissions(
+ operation: String,
+ user: Identity,
+ entityStore: ArtifactStore[WhiskEntity],
+ entityName: FullyQualifiedEntityName,
+ get: (ArtifactStore[WhiskEntity], DocId, DocRevision, Boolean) => Future[WhiskAction],
+ permissions: Option[String] = None)(implicit transid: TransactionId): Future[Unit] = {
+ if (operation == "create") {
+ permissions
+ .map { value =>
+ if (WhiskAction.permissionList.contains(value)) {
+ Future.successful(())
+ } else {
+ val errorInfo =
+ s"give error permission code: ${value}, available permission is in ${WhiskAction.permissionList}"
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ }
+ }
+ .getOrElse(Future.successful(()))
+ } else if (operation == "update") {
+ get(entityStore, entityName.toDocId, DocRevision.empty, true)
+ .flatMap { whiskAction =>
+ val currentPermissions = whiskAction.annotations
+ .get(WhiskAction.permissionsFieldName)
+ .map(value => value.convertTo[String])
+ .getOrElse(WhiskAction.defaultPermissions)
+
+ val errorInfo = s"have no permission to ${operation} this action"
+ permissions match {
+ case Some(value) =>
+ if (!WhiskAction.permissionList.contains(value)) {
+ val errorInfo =
+ s"give error permission code: ${value}, available permission is in ${WhiskAction.permissionList}"
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ } else {
+ val passedUpdatePermission = value.charAt(1)
+ if (passedUpdatePermission == 'w') { // make it to modifiable
+ Future.successful(())
+ } else {
+ val currentUpdatePermission = currentPermissions.charAt(1)
+ if (currentUpdatePermission == '-') {
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ } else {
+ Future.successful(())
+ }
+ }
+ }
+ case None =>
+ val currentUpdatePermission = currentPermissions.charAt(1)
+ if (currentUpdatePermission == '-') {
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ } else {
+ Future.successful(())
+ }
+ }
+ }
+ .recoverWith {
+ case t: RejectRequest =>
+ Future.failed(t)
+ case _ =>
+ Future.successful(())
+ }
+ } else if (operation == "remove") {
+ get(entityStore, entityName.toDocId, DocRevision.empty, true)
+ .flatMap { whiskAction =>
+ val currentPermissions = whiskAction.annotations
+ .get(WhiskAction.permissionsFieldName)
+ .map(value => value.convertTo[String])
+ .getOrElse(WhiskAction.defaultPermissions)
+
+ val currentUpdatePermission = currentPermissions.charAt(1)
+ if (currentUpdatePermission == '-') {
+ val errorInfo = s"have no permission to ${operation} this action"
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ } else {
+ Future.successful(())
+ }
+ }
+ .recoverWith {
+ case t: RejectRequest =>
+ Future.failed(t)
+ case _ =>
+ Future.successful(())
+ }
+ } else if (operation == "invoke") {
+ get(entityStore, entityName.toDocId, DocRevision.empty, true)
+ .flatMap { whiskAction =>
+ val currentPermissions = whiskAction.annotations
+ .get(WhiskAction.permissionsFieldName)
+ .map(value => value.convertTo[String])
+ .getOrElse(WhiskAction.defaultPermissions)
+
+ // the user who is owner by default
+ var currentExecutePermission = currentPermissions.charAt(2)
+ var errorInfo = s"have no permission to ${operation} this action"
+ if (user.namespace.name.asString != entityName.path.root.asString) { // the user who invoke the shared action
+ currentExecutePermission = currentPermissions.charAt(5)
+ errorInfo = s"have no permission to ${operation} this shared action"
+ }
+ if (currentExecutePermission == '-') { //have no permission
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ } else {
+ Future.successful(())
+ }
+ }
+ .recoverWith {
+ case t: RejectRequest =>
+ Future.failed(t)
+ case _ =>
+ Future.successful(())
+ }
+ } else { // download the code
+ get(entityStore, entityName.toDocId, DocRevision.empty, true)
+ .flatMap { whiskAction =>
+ val currentPermissions = whiskAction.annotations
+ .get(WhiskAction.permissionsFieldName)
+ .map(value => value.convertTo[String])
+ .getOrElse(WhiskAction.defaultPermissions)
+
+ val errorInfo = s"have no permission to download this shared action"
+ val currentDownloadPermission = currentPermissions.charAt(3)
Review comment:
shouldn't this be `currentPermissions.charAt(0)`? since permission string is like `rwx---` and the first char indicate the "read" permission
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] codecov-commenter edited a comment on pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#issuecomment-850276876
# [Codecov](https://codecov.io/gh/apache/openwhisk/pull/4058?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#4058](https://codecov.io/gh/apache/openwhisk/pull/4058?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (92b2a38) into [master](https://codecov.io/gh/apache/openwhisk/commit/b818c3b3e8bd3fa9ac7742d1b8c051ec09b76ae2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b818c3b) will **decrease** coverage by `1.46%`.
> The diff coverage is `90.62%`.
[](https://codecov.io/gh/apache/openwhisk/pull/4058?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #4058 +/- ##
==========================================
- Coverage 77.26% 75.80% -1.47%
==========================================
Files 219 226 +7
Lines 11205 11578 +373
Branches 487 498 +11
==========================================
+ Hits 8658 8777 +119
- Misses 2547 2801 +254
```
| [Impacted Files](https://codecov.io/gh/apache/openwhisk/pull/4058?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...cala/org/apache/openwhisk/http/ErrorResponse.scala](https://codecov.io/gh/apache/openwhisk/pull/4058/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2h0dHAvRXJyb3JSZXNwb25zZS5zY2FsYQ==) | `89.79% <ø> (+2.04%)` | :arrow_up: |
| [...pache/openwhisk/core/entitlement/Entitlement.scala](https://codecov.io/gh/apache/openwhisk/pull/4058/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9jb250cm9sbGVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvZW50aXRsZW1lbnQvRW50aXRsZW1lbnQuc2NhbGE=) | `83.67% <86.66%> (+1.85%)` | :arrow_up: |
| [...org/apache/openwhisk/core/controller/Actions.scala](https://codecov.io/gh/apache/openwhisk/pull/4058/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9jb250cm9sbGVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvY29udHJvbGxlci9BY3Rpb25zLnNjYWxh) | `90.90% <94.11%> (+0.12%)` | :arrow_up: |
| [...org/apache/openwhisk/core/entity/WhiskAction.scala](https://codecov.io/gh/apache/openwhisk/pull/4058/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvZW50aXR5L1doaXNrQWN0aW9uLnNjYWxh) | `93.06% <100.00%> (+0.71%)` | :arrow_up: |
| [...core/database/cosmosdb/RxObservableImplicits.scala](https://codecov.io/gh/apache/openwhisk/pull/4058/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvZGF0YWJhc2UvY29zbW9zZGIvUnhPYnNlcnZhYmxlSW1wbGljaXRzLnNjYWxh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ore/database/cosmosdb/cache/CacheInvalidator.scala](https://codecov.io/gh/apache/openwhisk/pull/4058/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9jb3Ntb3NkYi9jYWNoZS1pbnZhbGlkYXRvci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL29wZW53aGlzay9jb3JlL2RhdGFiYXNlL2Nvc21vc2RiL2NhY2hlL0NhY2hlSW52YWxpZGF0b3Iuc2NhbGE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...e/database/cosmosdb/cache/ChangeFeedConsumer.scala](https://codecov.io/gh/apache/openwhisk/pull/4058/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9jb3Ntb3NkYi9jYWNoZS1pbnZhbGlkYXRvci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL29wZW53aGlzay9jb3JlL2RhdGFiYXNlL2Nvc21vc2RiL2NhY2hlL0NoYW5nZUZlZWRDb25zdW1lci5zY2FsYQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...core/database/cosmosdb/CosmosDBArtifactStore.scala](https://codecov.io/gh/apache/openwhisk/pull/4058/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvZGF0YWJhc2UvY29zbW9zZGIvQ29zbW9zREJBcnRpZmFjdFN0b3JlLnNjYWxh) | `0.00% <0.00%> (-95.85%)` | :arrow_down: |
| [...sk/core/database/cosmosdb/CosmosDBViewMapper.scala](https://codecov.io/gh/apache/openwhisk/pull/4058/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvZGF0YWJhc2UvY29zbW9zZGIvQ29zbW9zREJWaWV3TWFwcGVyLnNjYWxh) | `0.00% <0.00%> (-93.90%)` | :arrow_down: |
| [...tabase/cosmosdb/cache/CacheInvalidatorConfig.scala](https://codecov.io/gh/apache/openwhisk/pull/4058/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9jb3Ntb3NkYi9jYWNoZS1pbnZhbGlkYXRvci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL29wZW53aGlzay9jb3JlL2RhdGFiYXNlL2Nvc21vc2RiL2NhY2hlL0NhY2hlSW52YWxpZGF0b3JDb25maWcuc2NhbGE=) | `0.00% <0.00%> (-92.31%)` | :arrow_down: |
| ... and [49 more](https://codecov.io/gh/apache/openwhisk/pull/4058/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/openwhisk/pull/4058?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/openwhisk/pull/4058?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [b818c3b...92b2a38](https://codecov.io/gh/apache/openwhisk/pull/4058?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang edited a comment on pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang edited a comment on pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#issuecomment-626607066
Hi, guys, i push a commit here in this patch, just check my direction(permission unix sytle) is whether right. https://github.com/apache/openwhisk/pull/4058/commits/d77a80fd0348d05f3733f95dbc0e016429e96753
After check in my local, i found that, there has some differences between our own permission management between unix permission management.
### 1. user type is different
for one action, just have 2 type users.
* the action's owner
* the user (not the owner) who used the shared action directly(eg. get, invoke)
As we all already know, unix's permission has `3` user types, this is the different point.
### 2. our own permission value is restricted
* for one action, anyway, the user(not the owner) can't write the action on any situation, just the owner has the write permission.
* the owner's permission can affect the permission as well, e.g. if the owner is not given `read` permission, i think the user(not the owner) can't have `read` permission as well. if the owner is not give `execute` permission, the user(not the owner) can't have `execute` permission as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] jiangpengcheng commented on a change in pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
jiangpengcheng commented on a change in pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#discussion_r639375466
##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/entitlement/Entitlement.scala
##########
@@ -231,6 +234,157 @@ protected[core] abstract class EntitlementProvider(
.getOrElse(Future.successful(()))
}
+ /**
+ * Checks if action operation(get/write/execute) whether feasible
+ *
+ * @param operation the action operation, e.g. get/write/execute
+ * @param user the user who get/write/execute the action
+ * @param entityStore store to write the action to
+ * @param entityName entityName
+ * @param permissions the passed permission code
+ * @return a promise that completes with success iff action operation is feasible
+ */
+ protected[core] def checkActionPermissions(
+ operation: String,
+ user: Identity,
+ entityStore: ArtifactStore[WhiskEntity],
+ entityName: FullyQualifiedEntityName,
+ get: (ArtifactStore[WhiskEntity], DocId, DocRevision, Boolean) => Future[WhiskAction],
+ permissions: Option[String] = None)(implicit transid: TransactionId): Future[Unit] = {
+ if (operation == "create") {
+ permissions
+ .map { value =>
+ if (WhiskAction.permissionList.contains(value)) {
+ Future.successful(())
+ } else {
+ val errorInfo =
+ s"give error permission code: ${value}, available permission is in ${WhiskAction.permissionList}"
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ }
+ }
+ .getOrElse(Future.successful(()))
+ } else if (operation == "update") {
+ get(entityStore, entityName.toDocId, DocRevision.empty, true)
+ .flatMap { whiskAction =>
+ val currentPermissions = whiskAction.annotations
+ .get(WhiskAction.permissionsFieldName)
+ .map(value => value.convertTo[String])
+ .getOrElse(WhiskAction.defaultPermissions)
+
+ val errorInfo = s"have no permission to ${operation} this action"
+ permissions match {
+ case Some(value) =>
+ if (!WhiskAction.permissionList.contains(value)) {
+ val errorInfo =
+ s"give error permission code: ${value}, available permission is in ${WhiskAction.permissionList}"
+ Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+ } else {
+ val passedUpdatePermission = value.charAt(1)
+ if (passedUpdatePermission == 'w') { // make it to modifiable
+ Future.successful(())
Review comment:
so once passed a `w` of action permission in params, the update will always pass priviledge checking even old update permission is `-`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang commented on a change in pull request #4058: Add protect feature to avoid update or delete actions by mistake
Posted by GitBox <gi...@apache.org>.
ningyougang commented on a change in pull request #4058:
URL: https://github.com/apache/openwhisk/pull/4058#discussion_r436552166
##########
File path: common/scala/src/main/scala/org/apache/openwhisk/core/entity/WhiskAction.scala
##########
@@ -350,6 +360,31 @@ object WhiskAction extends DocumentFactory[WhiskAction] with WhiskEntityQueries[
val execFieldName = "exec"
val requireWhiskAuthHeader = "x-require-whisk-auth"
+ // annotation permission key name
+ val permissionsFieldName = "permissions"
+
+ val defaultPermissions = "rwxr-x"
+
+ // notes on users, just have 2 type users,
+ // 1. the action's owner
+ // 2. the user (not the owner) who used the shared action directly(e.g. get, invoke)
+ //
+ // Notes on permission control
+ // 1. the action's read permission should open forever, because under invoke action or update action and so on,
+ // need to use `fetch` api to get the action to judge it whether exist.
+ // 2. the user(not the owner) can't update/delete the action forever.
+ // 3. the owner's permission can affect other user's permission, e.g
+ // if the owner is not given execute permission, the user(not the owner) can't have execute permission as well.
+ //
+ // Notes on permission values, include below permission value
+ // 1. permission code:rwxr-x: owner:read(yes)/write(yes)/execute(yes)|the shared action's user:read(yes)/write(no)/execute(yes), this is default
+ // 2. permission code:rwxr--: owner:read(yes)/write(yes)/execute(yes)|the shared action's user:read(yes)/write(no)/execute(no)
+ // 3. permission code:r-xr-x: owner:read(yes)/write(no)/execute(yes)|the shared action's user:read(yes)/write(no)/execute(yes)
+ // 4. permission code:r-xr--: owner:read(yes)/write(no)/execute(yes)|the shared action's user:read(yes)/write(no)/execute(no)
+ // 5. permission code:r--r--: owner:read(yes)/write(no)/execute(no)|the shared action's user:read(yes)/write(no)/execute(no)
+ // 6. permission code:rw-r--: owner:read(yes)/write(yes)/execute(no)|the shared action's user:read(yes)/write(no)/execute(no)
+ val permissionList = List(defaultPermissions, "rwxr--", "r-xr-x", "r-xr--", "r--r--", "rw-r--")
Review comment:
for the shared user, the `r` means `download the code`
##########
File path: common/scala/src/main/scala/org/apache/openwhisk/core/entity/WhiskAction.scala
##########
@@ -350,6 +360,31 @@ object WhiskAction extends DocumentFactory[WhiskAction] with WhiskEntityQueries[
val execFieldName = "exec"
val requireWhiskAuthHeader = "x-require-whisk-auth"
+ // annotation permission key name
+ val permissionsFieldName = "permissions"
+
+ val defaultPermissions = "rwxr-x"
+
+ // notes on users, just have 2 type users,
+ // 1. the action's owner
+ // 2. the user (not the owner) who used the shared action directly(e.g. get, invoke)
+ //
+ // Notes on permission control
+ // 1. the action's read permission should open forever, because under invoke action or update action and so on,
+ // need to use `fetch` api to get the action to judge it whether exist.
+ // 2. the user(not the owner) can't update/delete the action forever.
+ // 3. the owner's permission can affect other user's permission, e.g
+ // if the owner is not given execute permission, the user(not the owner) can't have execute permission as well.
+ //
+ // Notes on permission values, include below permission value
+ // 1. permission code:rwxr-x: owner:read(yes)/write(yes)/execute(yes)|the shared action's user:read(yes)/write(no)/execute(yes), this is default
+ // 2. permission code:rwxr--: owner:read(yes)/write(yes)/execute(yes)|the shared action's user:read(yes)/write(no)/execute(no)
+ // 3. permission code:r-xr-x: owner:read(yes)/write(no)/execute(yes)|the shared action's user:read(yes)/write(no)/execute(yes)
+ // 4. permission code:r-xr--: owner:read(yes)/write(no)/execute(yes)|the shared action's user:read(yes)/write(no)/execute(no)
+ // 5. permission code:r--r--: owner:read(yes)/write(no)/execute(no)|the shared action's user:read(yes)/write(no)/execute(no)
+ // 6. permission code:rw-r--: owner:read(yes)/write(yes)/execute(no)|the shared action's user:read(yes)/write(no)/execute(no)
+ val permissionList = List(defaultPermissions, "rwxr--", "r-xr-x", "r-xr--", "r--r--", "rw-r--")
Review comment:
`r` means `readable`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org