-
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?
Changes from all commits
5824b5c
b9a8ca8
c00f5f6
51ca755
92b2a38
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -76,6 +76,16 @@ case class WhiskActionPut(exec: Option[Exec] = None, | |
case _ => this | ||
} getOrElse this | ||
} | ||
|
||
protected[core] def getPermissions(): Option[String] = { | ||
annotations match { | ||
case Some(value) => | ||
value | ||
.get(WhiskAction.permissionsFieldName) | ||
.map(value => value.convertTo[String]) | ||
case None => None | ||
} | ||
} | ||
} | ||
|
||
abstract class WhiskActionLike(override val name: EntityName) extends WhiskEntity(name, "action") { | ||
|
@@ -352,6 +362,27 @@ 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" | ||
|
||
// For more information about permission, please refer to the online manual at | ||
// https://github.com/apache/openwhisk/blob/master/docs/actions.md#action-permission-management | ||
val permissionList = List( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. agree |
||
defaultPermissions, | ||
"rwxr--", | ||
"r-xr-x", | ||
"r-xr--", | ||
"r--r--", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||
"rw-r--", | ||
"rwx--x", | ||
"rwx---", | ||
"r-x--x", | ||
"r-x---", | ||
"r-----", | ||
"rw----") | ||
|
||
override val collectionName = "actions" | ||
override val cacheEnabled = true | ||
|
||
|
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 leastwskadmin
) 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.