Add support for "securityDefinitions"

[jhthorsen]

I have no idea if this should be merged, so I thought you could vote/decide.

What do you think?

@kivilahtio
@chandwki

Sidenote to @chandwki: You can now look up the spec in under() 12baad9

Sidenote to @leejo: Would it be cool to make a helper/extension that would do OAuth2 using this logic?

[jhthorsen]

I was also considering doing:

# App wide, instead of plugin wide:
$c->openapi->security->dummy($config, $wrapper);

# Could use something like "openapi_security_dummy" by default, but then be overridden by "x-mojo-helper" or something.
$c->$helper_name($config, $wrapper);

The reason why I want to use helpers is because I feel a "callback hash" does not encourage re-use.

[kivilahtio]

I am sure helpers are slick, especially since you use them in their own pseudo-namespace "openapi_security_dummy".
However I find helpers to be very confusing and I remember struggling to understand from where these cryptic $c->$helper_name -things spawn from?

Since you have a specific use-case for the "Security Definitions Object"-implementations it would be more clear to the end-user to find those implementations in
$c->openapi->security->dummy($config, $wrapper);

You can access the App from most everywhere anyway.

[jhthorsen]

@kivilahtio: Both $c->openapi->security->dummy and $c->openapi_security_dummy are helpers.

[jhthorsen]

Missing inheritance, meaning you can't set "security" globally in the API spec.

[kivilahtio]

You have inheritance for path parameters, using the same logic you can implement inheritance for the global "Security Requirement Object" (http://swagger.io/specification/#securityRequirementObject) ?

Not sure how important this is for us, since we have a separate scope for each operation object anyway, which overloads the global "Security Requirement Object".
To me, global inheritance doesn't look very difficult to add, so I would recommend adding it for the sake of specification completeness.

[jhthorsen]

Yeah, I just mentioned it so I don't forget in case this should be merged :)

[jhthorsen]

I kind of like the idea of calling $next or needing to render a document.

Need to fix typo. Should be:

return $c->render(openapi => $config, status => 401);

[kivilahtio]

I agree.
You must make it explicit in the documentation that this is expected and have error checking if somebody improperly forgets to render on auth failure.
Using clear error descriptions please.

[jhthorsen]

"Using clear error descriptions please"..? What do you mean? You decide the document to render. You can do this if you like:

return $c->render(json => {sorry => "you don't have access"}, status => 401);

The way to mark the user as authenticated or authorised is by calling $c->$next

[kivilahtio]

Yes.
My point was to catch the programmer error where one doesn't render anything by mistake or by not understanding the documentation, if the auth fails, and the $c->$next is not called.

I just wanted to make sure that the programmer gets some other error than "nothing rendered" if he doesn't understand that you must render something on a failed authentication. "nothing rendered" is really really vague and tracking the source of this problem takes ages.

This can prolly be dealt with in _security_action() ?

[jhthorsen]

No, since if you don't call render() or $next, then mojo will just sit there and nothing gets rendered. Just like you expect from a standard non-blocking under route. I could possibly add delay(), but I don't want to because it adds complexity that doesn't add anything from the standard mojo behaviour.

[kivilahtio]

So what happens then if next is not called and nothing is rendered?
The client timeouts?

[jhthorsen]

Yes. And the server. "inactivity timeout".

[kivilahtio]

The fatal exception is too vague.
It should state something like:
"You haven't defined any 'Security Definitions Object'-implementations as a OpenAPI-plugin 'security'-parameter"

This way one immediately knows where to look for what is missing.
Otherwise one must dig through the whole codebase to figure out why this error is given.

[kivilahtio]

I presume:
-2nd parameter of security settings is the subroutine implementing a "Security Scheme Object"? (http://swagger.io/specification/#securitySchemeObject)
-1st parameter is the name of the "Security Scheme Object", in this test case it would be 'dummy'?

The naming of the variable $config is obscuring. It should explicitly state that it is a anonymous subroutine, even better, of the type "Security Scheme Object"-implementation.
Maybe something like
$securitySchemeObjectImplementation

[jhthorsen]

$config is not a callback. In this case, $name is "dummy" and $config is []. You don't get the "securityDefinitions" object. You get the data from "security": [{"dummy": []}], meaning it will always (I think) be an empty array, except if you work with oauth2.

[kivilahtio]

Oh yeah, the "Security Scheme Object"-implementation is referenced via $security_cb->{$name} and the $config is actually the "Security Requirement Object".
This discovery doesn't change the fact that the variable should be named to mean what it is, in this case:
$securityRequirementObject
Had it been so, we wouldn't have had this discussion :)

[kivilahtio]

Sidenote to @\leejo: Would it be cool to make a helper/extension that would do OAuth2 using this logic?

It would.
@mrenvoize is prolly interested in this as well.

[leejo]

looks fine with current discussions

[leejo]

Sidenote to @leejo: Would it be cool to make a helper/extension that would do OAuth2 using this logic?

In the security callback you should be able to use it with Mojolicious::Plugin::OAuth2::Server as is:

return $c->$next if $c->oauth( $optional_scope );

[mrenvoize]

Still struggling to find time to properly dig into this, but in a quick excursion around it, it's showing allot of promise :)

[jberger]

The per-op under can't replace the global route. That would cause the security definitions to build up as you generate more routes!

[jhthorsen]

👍

[jhthorsen]

Are you sure that $global shouldn't get merged with the spec security?

[jberger]

No, it is overridden entirely. This is confirmed by the fact that the documentation says explicitly that a set of global security policies can be ignored by setting the op security to an empty array

[jhthorsen]

Awesome! Thanks 👍

[jhthorsen]

I don't get this part. Is it pre-optimization?

[jberger]

Perhaps, but I wouldn't want to have checks run multiple times if they occur in multiple OR conditions.

[jhthorsen]

Can you write a test/spec where this happens? I think this whole block makes the code harder to read, so I don't want it unless it has any effect on "the real world".

[jberger]

The cache test does this already. It defines two different ORed checks each that require pass1. The first also requires fail1 which fails and therefore the second check is attempted. Once that one succeeds and renders, it checks the hash of number of times that it was called, which thus tests that pass1 was only called once.

[jhthorsen]

Why don't you use $c->delay?

[jberger]

Perhaps I should, though that gives less flexibility in how the 500 is rendered. Maybe the logic from that should be copied though (wrt keeping the transaction alive).

[jhthorsen]

How come? There's already a openapi 500 handler in place. I think that should be sufficient, and a lot better than no error handling.

[jberger]

Will calling $c->reply->exception as the delay helper does work correctly in the plugin? If so then sure, it should definitely do that.

[jhthorsen]

Works like a charm. That's what my tests are doing now, becuase of https://github.com/kraih/mojo/blob/master/lib/Mojolicious/Plugin/DefaultHelpers.pm#L84

$delay->catch(sub { $c->helpers->reply->exception(pop) and undef $tx })->wait;

[jhthorsen]

I'm not sure if it's a good idea to run the checks in parallell. Think it would be much easier to implement a serial checking mechanism.

Maybe I misunderstand something?

[jberger]

If they are going to be asynchronous anyway, I don't see why they can't be in parallel.

[jhthorsen]

I want $cb to be get $c as the first argument.

[jberger]

I can agree with $c as the first argument if you want it. That said it doesn't add any value just consistency.

[jhthorsen]

I want, hehe. Then we can kill the begin(0) above as well, which makes the code easier to read.

[jberger]

I will just become begin since I still would need to cache the result.

[jberger]

Heh, actually it doesn't actually remove the begin(0) after all, just changes the shift to a $_[1].

[jhthorsen]

I was thinking $c->render would be issue a 401, but that doensn't work, since there is an OR between the checks...

[jberger]

My original response was

How can you render from a failing condition. You might have another ORed check that would cause a success. I think there has to be a security_failed event (or so) to handle when the routing is complete.

A possible alternative would be an arrayref that contains the failed results. If the array is empty at the end of a check then it succeeds. Values could then be accumulated and rendered as a standard JSON error response.

[jhthorsen]

How about reversing the code, so we could do something like the code below? That way, we could use the first error, merge them or pass the errors on to some other handler at the end.

$c->$cb({error => "You shall not pass!"});

[jberger]

That is kinda what I was saying in my second paragraph above. In this case I guess we have them call $c->$cb() or $c->$cb(undef) to indicate no error (ie. pass). We would need to consider what they should return. I'd think probably an error string or else an array of error strings. Then if no success is given the result could be the standard error document where each failed condition is an error in the errors object (perhaps even with the path to the security check that failed).

[jhthorsen]

I really don't like subtests.

[jberger]

Why not? These tests are grouped logically. I need a block anyway for the local on the diagnostic hash. I won't fight you on it though.

[jhthorsen]

It's just a matter of taste. I argued with @marcusramberg about perltidy for two years, and now I can't live without it... Maybe the same will happen with subtest, but not tonight 👎