Modify that web action in the bound package can be accessed. #3880

upgle · 2018-07-16T10:30:38Z

Description

Recently, many users want to share their web actions with the package.
but, the web action in the bound package can't be accessed in the browser.

Because when it is invoked in the browser, it doesn't resolve an action to find original fully qualified action name. so, I added the action name resolver.

HOW TO TEST.

bind a package that contains web actions and access web action which is in the bound package using your browser, you may see errors like the image below.

Fixed

Now, the bound web action can be accessed in the browser.

Related issue and scope

I opened an issue to propose and discuss this change (#????)

My changes affect the following components

Types of changes

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:

I signed an Apache CLA.
I reviewed the style guides and followed the recommendations (Travis CI will check :).
I added tests to cover my changes.
My changes require further changes to the documentation.
I updated the documentation where necessary.

codecov-io · 2018-07-16T11:07:29Z

Codecov Report

Merging #3880 into master will decrease coverage by 4.86%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3880      +/-   ##
==========================================
- Coverage   86.09%   81.22%   -4.87%     
==========================================
  Files         148      148              
  Lines        7249     7240       -9     
  Branches      442      440       -2     
==========================================
- Hits         6241     5881     -360     
- Misses       1008     1359     +351

Impacted Files	Coverage Δ
.../main/scala/whisk/core/controller/WebActions.scala	`92.43% <100%> (+1.27%)`	⬆️
...core/database/cosmosdb/RxObservableImplicits.scala	`0% <0%> (-100%)`	⬇️
...core/database/cosmosdb/CosmosDBArtifactStore.scala	`0% <0%> (-95.54%)`	⬇️
...sk/core/database/cosmosdb/CosmosDBViewMapper.scala	`0% <0%> (-92.6%)`	⬇️
...whisk/core/database/cosmosdb/CosmosDBSupport.scala	`0% <0%> (-83.34%)`	⬇️
...abase/cosmosdb/CosmosDBArtifactStoreProvider.scala	`0% <0%> (-58.83%)`	⬇️
...la/whisk/core/database/cosmosdb/CosmosDBUtil.scala	`92% <0%> (-4%)`	⬇️
...ain/scala/whisk/core/entitlement/Entitlement.scala	`88.42% <0%> (-2.48%)`	⬇️
...cala/src/main/scala/whisk/http/ErrorResponse.scala	`93.25% <0%> (+1.12%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 708274c...eb9f4d8. Read the comment docs.

rabbah · 2018-07-16T11:42:45Z

Thanks for trying this. There are some issues to be aware of for packaged actions as web actions via bindings. The lack of support isn’t a bug (it was intentional at the time). This has come up before so it’s worthwhile to try and enable this.

If an action isn’t already a web action in the package, it can’t be accessed as a web action through the binding. That is a subject that binds the action can’t make it a web action (unless they already own it). So the creator/sharer of the package decides which of the actions to make available as web actions. Inherently this could create a dependence the subject can't control.
The entitlement check for a web action today is straightforward: namespace must own both and so implicitly entitled to them. With a binding you need to check if the binding continues to have rights to the bound package.
And of course you’ll need to add tests for the change/new feature.

upgle · 2018-07-16T12:45:30Z

@rabbah
Thank you for your feedback and sorry that I thought it was a bug.
I will add some test codes including the rights check and suggest it again.

upgle · 2018-07-18T04:47:30Z

@rabbah

I modified it to check the entitlement and added tests.
Now, It returns the authentication error if a web action has no entitlement.

{
  "error": "The supplied authentication is not authorized to access 'whisk.system/share'.",
  "code": "NmSIY4dJanMT7YS3MPwFoOPL4Np72g5B"
}

Could you please remove WIP label and review again?

upgle · 2018-07-25T01:06:15Z

I fixed a conflict. please review it.

rabbah

I don't think this is quite right - take a look at my comment. I suggest therefore also adding additional tests to cover the issues I pointed out.

rabbah · 2018-07-29T12:57:24Z

core/controller/src/main/scala/whisk/core/controller/WebActions.scala

-      case _: ArtifactStoreException | DeserializationException(_, _, _) =>
-        Future.failed(RejectRequest(NotFound))
+    resolveAction(actionName) flatMap { resolveAction =>
+      getAction(resolveAction) recoverWith {


i dont believe this is right - the resolveAction method returns just the name of the action in the base package, and so getting that action means you'll miss the bound parameters. I think you want to use resolveActionAndMergeParameters instead.

Oh... I understand I'll modify it soon.

rabbah · 2018-07-29T12:59:25Z

core/controller/src/main/scala/whisk/core/controller/WebActions.scala

+    implicit transid: TransactionId): Future[Unit] = {
+    val resource =
+      Resource(action.namespace.root.toPath, Collection(Collection.PACKAGES), Some(action.namespace.last.toString))
+    entitlementProvider.check(identity, Privilege.READ, resource)


this check also checks the throttles - which means there's now a redundant check for throttles.
the code (before this pr), did not check entitlement through the provider because it can implicitly infer the owner (there's no indirection through a binding) so it only checked throttles.

Thank you for your kind review. I've removed existing entitlementProvider.checkThrottles(), then modified to use only entitlementProvider.check ().

build passed :)

upgle · 2018-07-31T14:18:46Z

@rabbah Maybe it's done. could you please review it again? 😅

rabbah · 2018-08-06T15:15:50Z

core/controller/src/main/scala/whisk/core/controller/WebActions.scala

@@ -552,8 +552,11 @@ trait WhiskWebActionsApi extends Directives with ValidateRequestSize with PostAc
        if (a.namespace.defaultPackage) {
          Future.successful(a)


i think here you've missed the throttle check that was removed above.

rabbah · 2018-08-06T15:21:39Z

core/controller/src/main/scala/whisk/core/controller/WebActions.scala

+          checkEntitlement(actionOwnerIdentity, a) flatMap { _ =>
+            pkgLookup(a.namespace.toFullyQualifiedEntityName) map { pkg =>
+              (a.inherit(pkg.parameters))
+            }


i think this is complicating logic unnecessary - now you're dealing with 3 cases:

action in default package

action in a proper package

action in a bound package (new)

You might as well normalize all three code paths - it's not clear to me from looking at this small change here that the changes are complete (and as noted above, I think you missed on throttle check).

@rabbah
yes, I agree that it looks complicated. so I've changed some codes. (removed pkgLookup method which is not necessary anymore)

action in a default package no need to resolve, just check entitlement and if it is exported.

action in a bound package(new) is handled by actionLookup() and resolveActionAndMergeParameters(), it would resolve action and merge parameters.

check whether action is in a proper package -> It check whether an action is exported by confirmExportedAction() method

and all types of action is checked for entitlement and throttle by checkEntitlement() method

please check changed code here

private def verifyWebAction(actionName: FullyQualifiedEntityName, authenticated: Boolean)( implicit transid: TransactionId) = { // lookup the identity for the action namespace identityLookup(actionName.path.root) flatMap { actionOwnerIdentity => confirmExportedAction(actionLookup(actionName), authenticated) flatMap { a => checkEntitlement(actionOwnerIdentity, a) map { _ => (actionOwnerIdentity, a)} } } }

https://github.com/apache/incubator-openwhisk/pull/3880/files#diff-a356e6b010b8aeb5ea7b6f7fec395038R535

junoyoon · 2018-08-09T15:20:39Z

@upgle is it done?

rabbah · 2018-08-09T15:24:32Z

The last commit looks a lot better now - I'll review more thoroughly.

upgle · 2018-08-10T05:16:16Z

@junoyoon no, i'm waiting for review.

@rabbah ok, tell me if I missed something.

rabbah

I apologize that I couldn't review this sooner. I think the changes are correct now but please see the comments - there's a couple more minor things we should address for this change.

rabbah · 2018-09-12T04:30:14Z

core/controller/src/main/scala/whisk/core/controller/WebActions.scala

   */
-  protected def getAction(actionName: FullyQualifiedEntityName)(
+  protected def resolveActionAndMergeParameters(actionName: FullyQualifiedEntityName)(


We should add a comment that while parameters are merged, annotations are not. This is important because the web annotation is on an action not an entire package. We should separately add a test for this invariant.

need to update the docs here: https://github.com/apache/incubator-openwhisk/blob/master/docs/webactions.md#additional-features the precedence order would now also include package binding parameters.

ok I'll update the docs soon.

thank you - it might be worth adding a section in the docs about web actions for packages and bindings (taking in some of my comments below). what do you think?

rabbah · 2018-09-12T04:34:57Z

core/controller/src/main/scala/whisk/core/controller/WebActions.scala

-            (a.inherit(pkg.parameters))
-          }
+    // lookup the identity for the action namespace
+    identityLookup(actionName.path.root) flatMap { actionOwnerIdentity =>


here the identity is tied to how the action is invoked: if the path is relative to the package, it's the owner of the package. But if it's via the binding, the identity is the owner binding. The two identities are not the same. It might also not make sense to invoke an action in a public package since it might require a binding to work correctly.

another aspect to document: a web action in a public package cannot be disabled so any (private) binding of the package will have the web action exposed.

rabbah · 2018-09-12T04:36:31Z

core/controller/src/main/scala/whisk/core/controller/WebActions.scala

-        // fail the request with BadRequest so as not to leak information about the existence
-        // of packages that are otherwise private
-        logging.debug(this, s"package which does not exist")
-        Future.failed(RejectRequest(NotFound))


we should add some tests for resolveActionAndMergeParameters to preserve the behavior represented by these request rejections (missing package or malformed package).

resolveActionAndMergeParameters method is for mock testing, and the code which is removed here is covered in this line. https://github.com/apache/incubator-openwhisk/blob/91ec9bca7d5f6d0c8c95018a2c9fede131b02b6b/core/controller/src/main/scala/whisk/core/controller/WebActions.scala#L677

also, test for missing package exists already here.
https://github.com/apache/incubator-openwhisk/blob/91ec9bca7d5f6d0c8c95018a2c9fede131b02b6b/tests/src/test/scala/whisk/core/controller/test/WebActionsApiTests.scala#L362

rabbah · 2018-09-13T12:11:18Z

core/controller/src/main/scala/whisk/core/controller/WebActions.scala

-            (a.inherit(pkg.parameters))
-          }
+    // lookup the identity for the action namespace
+    identityLookup(actionName.path.root) flatMap { actionOwnerIdentity =>


another aspect to document: a web action in a public package cannot be disabled so any (private) binding of the package will have the web action exposed.

rabbah · 2018-09-13T12:12:21Z

core/controller/src/main/scala/whisk/core/controller/WebActions.scala

   */
-  protected def getAction(actionName: FullyQualifiedEntityName)(
+  protected def resolveActionAndMergeParameters(actionName: FullyQualifiedEntityName)(


need to update the docs here: https://github.com/apache/incubator-openwhisk/blob/master/docs/webactions.md#additional-features the precedence order would now also include package binding parameters.

rabbah · 2018-09-13T12:15:18Z

tests/src/test/scala/whisk/core/controller/test/WebActionsApiTests.scala

@@ -233,7 +238,7 @@ trait WebActionsApiBaseTests extends ControllerTestCommon with BeforeAndAfterEac
      if (actionName.path.defaultPackage) {
        Future.successful(theAction)
      } else {
-        getPackage(actionName.path.toFullyQualifiedEntityName) map (_ => theAction)
+        getPackage(actionName.path.toFullyQualifiedEntityName) map (pkg => theAction.inherit(pkg.parameters))


there's no test for actually using a package binding here (if there is, it would fail since the parameter inheritance does not include the binding).

It is no necessary anymore because we use 'WhiskActionMetaData.resolveActionAndMergeParameters()' for resolving and merging parameters.

…s in one test to binding from package.

rabbah · 2018-11-06T21:05:01Z

I fixed the test fixture cleanup and also tweaked one test.

upgle · 2018-11-07T03:18:42Z

@rabbah test is passed. thank you for your support 👍

rabbah · 2018-11-07T06:40:30Z

Thank you @upgle for your contribution.

A web action in a shared (i.e., public) package is accessible as a web action either directly via the package's fully qualified name, or via a package binding. It is important to note that a web action in a public package will be accessible for all bindings of the package even if the binding is private. This is because the web action annotation is carried on the action and cannot be overridden. If you do not wish to expose a web action through your package bindings, then you should clone-and-own the package instead.

upgle changed the title ~~Fix a bug that web action in the bound package cannot be accessed.~~ [WIP] Modify that web action in the bound package can be accessed. Jul 16, 2018

rabbah added enhancement controller wip labels Jul 16, 2018

rabbah changed the title ~~[WIP] Modify that web action in the bound package can be accessed.~~ Modify that web action in the bound package can be accessed. Jul 16, 2018

markusthoemmes requested a review from rabbah July 23, 2018 10:33

markusthoemmes assigned rabbah Jul 23, 2018

rabbah added review Review for this PR has been requested and yet needs to be done. and removed wip labels Jul 27, 2018

rabbah requested changes Jul 29, 2018

View reviewed changes

rabbah added the awaits-contributor The contributor needs to respond to comments from reviewer. label Jul 29, 2018

upgle force-pushed the fix-shared-web-action branch 2 times, most recently from 31b3b21 to c3879db Compare July 30, 2018 13:28

rabbah reviewed Aug 6, 2018

View reviewed changes

upgle force-pushed the fix-shared-web-action branch from 5ed3a34 to 91ec9bc Compare August 7, 2018 05:30

rabbah reviewed Sep 12, 2018

View reviewed changes

rabbah reviewed Sep 13, 2018

View reviewed changes

upgle and others added 20 commits November 6, 2018 16:04

Fix to share web action package.

53c8c0a

Modify to check entitlement

27436b1

Reformat with scalafmt

8586d24

Reformat code

21c8d6a

Fix test code

bd244e7

Modify an annotation.

08dd4d2

Modify duplicate throttle checks and 'resolveAction' method.

b6f6673

Reformat with scalafmt

1300767

Remove getPackage() method and modify some tests

3d7879d

It is no necessary anymore because we use 'WhiskActionMetaData.resolveActionAndMergeParameters()' for resolving and merging parameters.

Update test code for resolveActionAndMergeParameters

b774a35

Add tests for binding package

eee83b6

Fix typo

34a7025

Add test which checks precedence order for merging parameters

6cc054e

Reformat with scalafmt

6cebb5e

Reformat with scalafmt

11540dc

Update webactions.md and add more test case

9577753

Remove unnecessary method.

82708f0

Tweak the docs.

9c87d83

Add more test case

7fff834

Clean up text fixtures after all tests have run. Also moved parameter…

eb9f4d8

…s in one test to binding from package.

rabbah force-pushed the fix-shared-web-action branch from e970d3a to eb9f4d8 Compare November 6, 2018 21:04

rabbah approved these changes Nov 6, 2018

View reviewed changes

rabbah added ready and removed awaits-contributor The contributor needs to respond to comments from reviewer. labels Nov 6, 2018

rabbah merged commit 7f52af4 into apache:master Nov 7, 2018

rabbah mentioned this pull request Jan 9, 2019

Update web action and annotations docs to reflect that bound actions can't be web actions #2204

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify that web action in the bound package can be accessed. #3880

Modify that web action in the bound package can be accessed. #3880

upgle commented Jul 16, 2018 •

edited

Loading

codecov-io commented Jul 16, 2018 •

edited

Loading

rabbah commented Jul 16, 2018 •

edited

Loading

upgle commented Jul 16, 2018

upgle commented Jul 18, 2018

upgle commented Jul 25, 2018

rabbah left a comment

rabbah Jul 29, 2018

upgle Jul 30, 2018

rabbah Jul 29, 2018

upgle Jul 30, 2018 •

edited

Loading

upgle Jul 30, 2018

upgle commented Jul 31, 2018

rabbah Aug 6, 2018

rabbah Aug 6, 2018

upgle Aug 7, 2018 •

edited

Loading

junoyoon commented Aug 9, 2018

rabbah commented Aug 9, 2018

upgle commented Aug 10, 2018

rabbah left a comment

rabbah Sep 12, 2018

rabbah Sep 13, 2018

upgle Sep 13, 2018

rabbah Sep 13, 2018

rabbah Sep 12, 2018

rabbah Sep 13, 2018

rabbah Sep 12, 2018

upgle Sep 17, 2018 •

edited

Loading

rabbah Sep 13, 2018

rabbah Sep 13, 2018

rabbah Sep 13, 2018

rabbah commented Nov 6, 2018

upgle commented Nov 7, 2018

rabbah commented Nov 7, 2018

		@@ -552,8 +552,11 @@ trait WhiskWebActionsApi extends Directives with ValidateRequestSize with PostAc
		if (a.namespace.defaultPackage) {
		Future.successful(a)

Modify that web action in the bound package can be accessed. #3880

Modify that web action in the bound package can be accessed. #3880

Conversation

upgle commented Jul 16, 2018 • edited Loading

Description

HOW TO TEST.

Fixed

Related issue and scope

My changes affect the following components

Types of changes

Checklist:

codecov-io commented Jul 16, 2018 • edited Loading

Codecov Report

rabbah commented Jul 16, 2018 • edited Loading

upgle commented Jul 16, 2018

upgle commented Jul 18, 2018

upgle commented Jul 25, 2018

rabbah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

upgle Jul 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

upgle commented Jul 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

upgle Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

junoyoon commented Aug 9, 2018

rabbah commented Aug 9, 2018

upgle commented Aug 10, 2018

rabbah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

upgle Sep 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rabbah commented Nov 6, 2018

upgle commented Nov 7, 2018

rabbah commented Nov 7, 2018

upgle commented Jul 16, 2018 •

edited

Loading

codecov-io commented Jul 16, 2018 •

edited

Loading

rabbah commented Jul 16, 2018 •

edited

Loading

upgle Jul 30, 2018 •

edited

Loading

upgle Aug 7, 2018 •

edited

Loading

upgle Sep 17, 2018 •

edited

Loading