-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add protect feature to avoid update or delete actions by mistake #4058
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
thanks a lot for your contribution. This PR definitely makes sense.
You are adding the protection
-parameter as URL parameter. What's the reason for doing it like this?
Today: Everything that is a property of the action is within WhiskActionPut
. The overwrite
parameter only says, that this request should overwrite the action if it already exists.
IMHO the protection
parameter is a property of the action itself and not of that request.
And a second thought that comes into my mind: If we protect the action, should it only be protected against deletion? Or would it make sense to protect it against updates as well? (The trick here would be to allow the update to unlock the action again).
I don't know what your thoughts on these two issues are. If your thoughts are completely different mybe it makes sense to ask the dev-list, if someone has a strong opinion on these two questions on the dev-list.
core/controller/src/main/scala/whisk/core/controller/Actions.scala
Outdated
Show resolved
Hide resolved
core/controller/src/main/scala/whisk/core/controller/Actions.scala
Outdated
Show resolved
Hide resolved
This is a good start but we should discuss and settle the design first. For example my preference is for Unix style permissions on the action (or asset more generally). |
@cbickel @rabbah ,thanks for your quick reply. Actually, i opened a pr first here: apache/openwhisk-cli#372 I did below changes
Or use wsk like this
After executed above method, this action enabled
|
3b7a7ed
to
d95dfce
Compare
Codecov Report
@@ Coverage Diff @@
## master #4058 +/- ##
=========================================
- Coverage 86.21% 78.61% -7.6%
=========================================
Files 152 153 +1
Lines 7275 7895 +620
Branches 477 518 +41
=========================================
- Hits 6272 6207 -65
- Misses 1003 1688 +685
Continue to review full report at Codecov.
|
d95dfce
to
5356f5a
Compare
5356f5a
to
63a2aaf
Compare
core/controller/src/main/scala/org/apache/openwhisk/core/controller/Actions.scala
Outdated
Show resolved
Hide resolved
546f431
to
69602d9
Compare
Sorry for taking so long to get through this --- taking a look now it's a good first step but i think it points to a more general permission set on an action: this is an write permission for the namespace (group in unix terminology), and one can image an execute permission as well next (disables an action from executing for a namespace). So one of my thoughts is whether to factor the permission more generally to allow for generalization later... or leave it as is and change it later. Thoughts? |
@rabbah At the same time, we can wait other upsteam guys to give more suggestions, |
@ningyougang |
@mhenke1 Are the alternatives you've implemented something that is planned for open source contribution? |
docs/actions.md
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will suggest some updates to this text once the API change and implementation are finalized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated accordingly
69602d9
to
d77a80f
Compare
Hi, guys, i push a temp commit here in this patch, just check my direction(permission unix sytle) is whether right. d77a80f 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 differentfor one action, just have 2 type users.
As we all already know, unix's permission has 2. our own permission value is restricted
How do you guys think? |
cf35509
to
1c37112
Compare
// 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--") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does r
means get codes here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the shared user, the r
means download the code
1c37112
to
4af70b8
Compare
4af70b8
to
db02599
Compare
case t: RejectRequest => | ||
Future.failed(t) | ||
case _ => | ||
Future.successful(()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
, returnFuture.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 toget(entityStore, entityName.toDocId, DocRevision.empty, true)
's second param, actually, this method needsaction
, so will throw NoDocument exception, in order to make the test case passed, need to make it returnFuture.successful(())
Already rebased and solved relative test cases error |
|
||
val defaultPermissions = "rwxr-x" | ||
|
||
// notes on users, just have 2 type users, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather keep this information in some markdown file and just leave the link here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated accordingly.
"rwxr--", | ||
"r-xr-x", | ||
"r-xr--", | ||
"r--r--", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So even if an owner doesn't have any permission, he can still update the permission annotations?
// 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( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd apply a regex match to confirm the permission value rather than just checking the existence of the value in a list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree
@@ -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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not quite sure it is a good idea to use annotations for permission management.
I feel like we need a new protocol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, i also feel it is not a good idea to use annotation for permission management.
Currently, have no better idea for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think such permission information is applied to actions instead of Identity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
4367a2c
to
a7a51d9
Compare
get(entityStore, entityName.toDocId, DocRevision.empty, true) | ||
.flatMap { whiskAction => | ||
val currentPermissions = whiskAction.annotations | ||
.get(WhiskAction.permissionsFieldName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can depend on permission data in DB rather than relying on action fields.
} else { | ||
val passedUpdatePermission = value.charAt(1) | ||
if (passedUpdatePermission == 'w') { // make it to modifiable | ||
Future.successful(()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so once passed a w
of action permission in params, the update will always pass priviledge checking even old update permission is -
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
entityName: FullyQualifiedEntityName, | ||
get: (ArtifactStore[WhiskEntity], DocId, DocRevision, Boolean) => Future[WhiskAction], | ||
permissions: Option[String] = None)(implicit transid: TransactionId): Future[Unit] = { | ||
if (operation == "create") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
operation match {
case "create" =>
...
should be better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated accordingly.
.getOrElse(WhiskAction.defaultPermissions) | ||
|
||
val errorInfo = s"have no permission to download this shared action" | ||
val currentDownloadPermission = currentPermissions.charAt(3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be currentPermissions.charAt(0)
? since permission string is like rwx---
and the first char indicate the "read" permission
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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(())
}
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 |
- 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 updating or deletion by mistake. - Apply unix style permission management - Make the action's downloadable for the shared user
a7a51d9
to
92b2a38
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ 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 ☔ View full report in Codecov by Sentry. |
According to review comments It seems there has another option to do 1. There is no need to add permissions controls for action's "owner" since he/she can anyway update/remove/invoke his/her action2. 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:
|
@style95 , Current system permission already existed design solution:
New design solution: #4058 (comment) The difference between the new design solution and
hm..actually, for now, i don't know what's the direction of this pr. |
@ningyougang Sorry for the late response. |
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)
The lock field will be added in couchdb's annotation, like below
So this action can't be deleted/updated until the protection is updated to
false
Description
Related issue and scope
My changes affect the following components
Types of changes
Checklist: