Skip to content
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

actions with require-whisk-auth annotation as boolean false are rejected by controller #4985

Closed
rabbah opened this issue Sep 25, 2020 · 4 comments

Comments

@rabbah
Copy link
Member

@rabbah rabbah commented Sep 25, 2020

The annotation value require-whisk-auth: false on a web action leads the controller to reject the web action invocation.

  1. The openwhisk documentation does not say what should happen when the value is false. I don't know why this case is omitted case https://github.com/apache/openwhisk/blob/master/docs/webactions.md#securing-web-actions
  2. The implementation treats this case as "not allowed, something is not right" see line 790 in this snippet
    private def confirmAuthenticated(annotations: Parameters,
    reqHeaders: Seq[HttpHeader],
    authenticatedUser: Option[Identity]): Option[Boolean] = {
    def checkAuthHeader(expected: String): Boolean = {
    reqHeaders.find(_.is(WhiskAction.requireWhiskAuthHeader)).map(_.value == expected).getOrElse(false)
    }
    annotations
    .get(Annotations.RequireWhiskAuthAnnotation)
    .map {
    case JsString(auth) => checkAuthHeader(auth) // allowed if auth matches header
    case JsNumber(auth) => checkAuthHeader(auth.toString) // allowed if auth matches header
    case JsTrue | JsBoolean(true) => authenticatedUser.isDefined // allowed if user already authenticated
    case _ => false // not allowed, something is not right

The controller should handle the case of the annotation being set to boolean false as not requiring authentication rather than reject the call.

@style95
Copy link
Member

@style95 style95 commented Sep 28, 2020

So this is the case where require-whisk-auth annotation is specified but the value is false.
I think it has not been noticed because if the annotation does not exist, it passes the authentication.
So anyone who does not require any authentication, they just skip configuring the annotation rather than set it explicitly.

But it looks clearly a bug.

@nitikagarw
Copy link
Contributor

@nitikagarw nitikagarw commented Oct 3, 2020

@rabbah @style95 I think this task just requires one more case to be added

case JsFalse | JsBoolean(false) => true

If so, Can I please pick this issue?

@rabbah
Copy link
Member Author

@rabbah rabbah commented Oct 3, 2020

@nitikagarw looks right - also will need a test.
Thank you for the contribution.

@rabbah
Copy link
Member Author

@rabbah rabbah commented Oct 12, 2020

Thanks @nitikagarw for the contribution!

@rabbah rabbah closed this Oct 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.