Add OpenAPI security (rebase) #59

gcasar · 2021-06-21T14:57:26Z

Fixes Issue #35, continues stale PR #50.

I tried to keep the comments from #50 relevant by resolving conflicts by back-merging and addressing them here. We can squash commits at your discretion.

adds generator methods
some edge case test coverage
marshalling empty security field in path section (see security parameter - an empty array is used to signal "no security" override)

The last point is the only one that is troublesome since it does not align with the way JSON gets serialised and would benefit from something like this request. The proposed way is backwards compatible but does repeat some code.

wI2L · 2021-06-22T02:24:58Z

@gcasar Thanks for picking this up. I'll review it asap this week.

Regarding the omitnil proposal you linked, you could check out my other package jettison. (which support it), but after a quick peek to your custom marshaller implem, I think it's better than introducing a new dependency just for that need (which would only be for JSON marshaling anyway, and not YAML).

wI2L · 2021-06-29T12:26:31Z

openapi/spec.go

+}
+
+// A workaround for missing omitnil functionality.
+// explicitly excludes Security which:


I would rephrase the comment to smthing like this, up to you tho:

It explicitely omit the Security field from marshaling when it is nil, but not when empty.

wI2L · 2021-06-29T12:41:25Z

openapi/generator.go

@@ -104,6 +104,12 @@ func (g *Generator) SetServers(servers []*Server) {
 	g.api.Servers = servers
 }

+// SetSecurity sets the security options for the
+// current specification.
+func (g *Generator) SetSecurity(security *SecurityRequirement) {


I would prefer to name the function SetSecurityRequirement, to be in-par with the OpenAPI spec object naming.

wI2L · 2021-06-29T12:43:36Z

fizz.go

+}
+
+// NoSecurity explicitly removes all security.
+func NoSecurity() func(*openapi.OperationInfo) {


Having this feels weird for the end-user. I'm wondering if it shouldn't be the default, applied directly by the package (i.e, if operation.Security == nil, initialize Security to empty slice, via MarshalYAML/JSON) ?

Hmm... given that I understand your comment correctly I believe that would actually change semantics:

you want to omit Security altogether (as per the spec) if you are not overriding the top level Security setting

any serialized Security value, even an empty array has meaning.

From Operation Object:

A declaration of which security mechanisms can be used for this operation. The list of values includes alternative security requirement objects that can be used. Only one of the security requirement objects need to be satisfied to authorize a request. To make security optional, an empty security requirement ({}) can be included in the array. This definition overrides any declared top-level security. To remove a top-level security declaration, an empty array can be used.

Okay, that was me, I didn't read the spec properly. NoSecurity() would be useful only if the top-level Security field is set:

To remove a top-level security declaration, an empty array can be used.

What would you think of WithoutSecurity instead as name ?

Speaking of that, the spec also specify the following:

To make security optional, an empty security requirement ({}) can be included in the array.

We could provide a "helper-func" (WithOptionalSecurity()) for that, that would append an empty openapi.SecurityRequirement object in the operation's Security field (slice). Thoughts ?

I like it 👍

wI2L · 2021-06-29T12:53:03Z

fizz.go

@@ -353,6 +353,20 @@ func InputModel(model interface{}) func(*openapi.OperationInfo) {
 	}
 }

+// Security adds a security definition to an operation.
+func Security(security *openapi.SecurityRequirement) func(*openapi.OperationInfo) {


Please add this to the README: https://github.com/wI2L/fizz#operation-informations

Also, if we're including security-related helpers functions such as WithoutSecurity() and OptionalSecurity(), they should also be documented/mentionned.

wI2L · 2021-06-29T13:28:44Z

@gcasar Sorry for the delay, just got back from holidays. Please see the minor fixes suggestions, otherwise looks good.
I'd rather squash indeed, for commit logs brevity.

wI2L · 2021-06-29T13:31:14Z

Closes #50.

wI2L

Thanks.
Few docs fixes proposed, but otherwise LGTM.
Please squash your commits, and I'll merge it ASAP (might take some time before to fix the CI/CD workflow to switch from Jenkins to GitHub Actions).

wI2L · 2021-06-29T14:56:04Z

README.md

@@ -96,6 +96,17 @@ fizz.Header(name, desc string, model interface{})
 // Override the binding model of the operation.
 fizz.InputModel(model interface{})

+// Add a security requirement to the list of security overrides for this operation.


Sounds redundant to me.

What do you think of:

Overrides top-level security requirement for this operation. Note that this function can be used more than once to add may requirements.

wI2L · 2021-06-29T15:02:32Z

README.md

+// that might have been specified in the root `security` property.
+fizz.Security(security *openapi.SecurityRequirement)
+
+// Make other security requirements optional.


Again, I think it is better to be explicit. We should indicate what the function does, not just its effect:

Add an empty security requirement to this operation to make other security requirements optional.

I was not sure how to break this one - let me know if you want me to reformat it:)

wI2L · 2021-06-29T15:03:13Z

README.md

+// Make other security requirements optional.
+fizz.WithOptionalSecurity()
+
+// Remove any security requirements for this operation.


Remove any top-level security requirements for this operation.

wI2L · 2021-06-29T15:03:48Z

fizz.go

+	}
+}
+
+// WithoutSecurity explicitly removes all security.


Use same comment as those in README. See proposal.

wI2L · 2021-06-29T15:03:50Z

fizz.go

+	}
+}
+
+// WithOptionalSecurity makes other security requirements optional.


Use same comment as those in README. See proposal.

gcasar · 2021-06-29T19:13:05Z

@wI2L almost there :) I don't like to squash by force pushing - is it ok with you if you squash it yourself when merging?

ipfans · 2021-08-08T08:53:46Z

Any updates @wI2L ?

wI2L · 2021-08-20T13:01:21Z

@ipfans I'll take care of migrating CI to Github actions and merge pending pull requests to make a proper release by september. Sorry for the delay.

nikicc · 2021-11-19T14:09:05Z

Any updates on this? I'd find it useful 😊

codecov · 2021-12-22T14:53:10Z

Codecov Report

Merging #59 (2994b14) into master (8a048be) will decrease coverage by 0.71%.
The diff coverage is 72.41%.

❗ Current head 2994b14 differs from pull request most recent head c029d2c. Consider uploading reports for the commit c029d2c to get more accurate results

@@            Coverage Diff             @@
##           master      #59      +/-   ##
==========================================
- Coverage   95.88%   95.17%   -0.72%     
==========================================
  Files           7        7              
  Lines         924      953      +29     
==========================================
+ Hits          886      907      +21     
- Misses         22       30       +8     
  Partials       16       16

Impacted Files	Coverage Δ
openapi/generator.go	`94.25% <50.00%> (-0.14%)`	⬇️
openapi/spec.go	`81.57% <65.00%> (-18.43%)`	⬇️
fizz.go	`98.66% <100.00%> (+0.06%)`	⬆️

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 8a048be...c029d2c. Read the comment docs.

Co-authored-by: Gregor Casar <gregorcasar@gmail.com>

wI2L · 2021-12-22T15:55:53Z

To everyone involved on this PR, and expecting the changes, I apologize for the unreasonable delay, haven't found some time to work on it, and I wanted to update the CI pipeline of the project before merhing anything new. This will be shipped in next release.

nikicc · 2021-12-22T15:57:34Z

Thanks for getting it in @wI2L!

wI2L requested changes Jun 29, 2021

View reviewed changes

wI2L reviewed Jun 29, 2021

View reviewed changes

wI2L mentioned this pull request Jun 29, 2021

Add OpenAPI security #50

Closed

gcasar requested a review from wI2L June 29, 2021 14:15

wI2L requested changes Jun 29, 2021

View reviewed changes

gcasar requested a review from wI2L June 29, 2021 19:13

wI2L force-pushed the openapi-security branch 5 times, most recently from 285ce19 to 7606e1d Compare December 22, 2021 15:49

wI2L previously approved these changes Dec 22, 2021

View reviewed changes

feat: support for OpenAPI security spec

c029d2c

Co-authored-by: Gregor Casar <gregorcasar@gmail.com>

wI2L dismissed their stale review via c029d2c December 22, 2021 15:51

wI2L force-pushed the openapi-security branch from 7606e1d to c029d2c Compare December 22, 2021 15:51

wI2L approved these changes Dec 22, 2021

View reviewed changes

wI2L merged commit 14ef325 into wI2L:master Dec 22, 2021

nikicc mentioned this pull request Jan 18, 2022

Readd x-tagGroups #70

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add OpenAPI security (rebase) #59

Add OpenAPI security (rebase) #59

gcasar commented Jun 21, 2021

wI2L commented Jun 22, 2021 •

edited

Loading

wI2L Jun 29, 2021 •

edited

Loading

wI2L Jun 29, 2021

wI2L Jun 29, 2021

gcasar Jun 29, 2021

wI2L Jun 29, 2021

gcasar Jun 29, 2021

wI2L Jun 29, 2021

wI2L Jun 29, 2021

wI2L commented Jun 29, 2021

wI2L commented Jun 29, 2021 •

edited

Loading

wI2L left a comment

wI2L Jun 29, 2021

wI2L Jun 29, 2021

gcasar Jun 29, 2021

wI2L Jun 29, 2021

wI2L Jun 29, 2021

wI2L Jun 29, 2021

gcasar commented Jun 29, 2021

ipfans commented Aug 8, 2021

wI2L commented Aug 20, 2021 •

edited

Loading

nikicc commented Nov 19, 2021

codecov bot commented Dec 22, 2021 •

edited

Loading

wI2L commented Dec 22, 2021

nikicc commented Dec 22, 2021

Add OpenAPI security (rebase) #59

Add OpenAPI security (rebase) #59

Conversation

gcasar commented Jun 21, 2021

wI2L commented Jun 22, 2021 • edited Loading

wI2L Jun 29, 2021 • 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

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

wI2L commented Jun 29, 2021

wI2L commented Jun 29, 2021 • edited Loading

wI2L 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

gcasar commented Jun 29, 2021

ipfans commented Aug 8, 2021

wI2L commented Aug 20, 2021 • edited Loading

nikicc commented Nov 19, 2021

codecov bot commented Dec 22, 2021 • edited Loading

Codecov Report

wI2L commented Dec 22, 2021

nikicc commented Dec 22, 2021

wI2L commented Jun 22, 2021 •

edited

Loading

wI2L Jun 29, 2021 •

edited

Loading

wI2L commented Jun 29, 2021 •

edited

Loading

wI2L commented Aug 20, 2021 •

edited

Loading

codecov bot commented Dec 22, 2021 •

edited

Loading