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

added securityScheme #226

Merged
merged 12 commits into from
Dec 6, 2024
Merged

added securityScheme #226

merged 12 commits into from
Dec 6, 2024

Conversation

simonemastella
Copy link
Contributor

@simonemastella simonemastella commented Nov 16, 2024

Add WithSecurity Server Option and Route Security Documentation

Issue #221

Description

Adds a new WithSecurity server option to configure security schemes in the OpenAPI specification and documents the route-level .Security() method. This change improves the API by providing a complete security configuration solution at both the server and route levels.

Features

Server-Level Security Configuration

  • New WithSecurity function that accepts a map of security schemes
  • Supports all OpenAPI security scheme types (HTTP, API Key, OAuth2, etc.)
  • Safe initialization of SecuritySchemes map
  • Allows multiple security schemes to be configured simultaneously

Route-Level Security

  • Enhanced documentation for .Security(requirementName string) method
  • Allows per-route security requirements specification
  • Links routes to security schemes defined at the server level

Example Usage

Server Security Configuration

s := NewServer(
    WithSecurity(openapi3.SecuritySchemes{
        "bearerAuth": &openapi3.SecuritySchemeRef{
            Value: openapi3.NewSecurityScheme().
                WithType("http").
                WithScheme("bearer").
                WithBearerFormat("JWT").
                WithDescription("Enter your JWT token in the format: Bearer <token>"),
        },
    }),
)

Route Security Configuration

goCopyGet(s, "/protected", handler).
    Security("bearerAuth").  // Requires JWT authentication
    Summary("Protected endpoint")

Benefits

  • Complete security solution: Configure security at both server and route levels
  • More maintainable: Security schemes can be defined and updated in one place
  • More flexible: Easy to add or modify security requirements
  • Better API design: Follows the pattern of other server options
  • Improved developer experience: Makes security configuration more explicit and easier to understand
  • Fine-grained control: Apply different security requirements to different routes

Breaking Changes

None. This is a new feature that maintains backward compatibility.

Example

image

Copy link
Collaborator

@dylanhitt dylanhitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution. Would you mind making the requested changes?

Also a reminder to please add tests! Thank you

openapi_operations.go Outdated Show resolved Hide resolved
@simonemastella
Copy link
Contributor Author

Thanks for the contribution. Would you mind making the requested changes?

Also a reminder to please add tests! Thank you

Yeah I'll work on it but this is a busy week and it might require a couple of days since I would like to add the possibility to combine multiple requirements: (requirement1 OR requirement2) AND requirement3

It might make sense to add the scope of a requirement as well https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.0.3.md#security-requirement-object

I will also add some test to cover the feature!

Copy link
Collaborator

@dylanhitt dylanhitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will 100% need a docs update for this (it doesn't have to exist in this PR if you don't have the time. I thank you for the contribution enough).

That being said, and looking at this pretty late. This looks fantastic. To play devils advocate. What are the thoughts on making the OR logic be a new OptionSecurity entry on the route. That might possibly simplify the interface/make it more clear.

Again I didn't look closely and won't have the time til Friday. But that's my initial feedback. I'll more closely friday unless another team member beats me to it.

@dylanhitt
Copy link
Collaborator

Was able to get to this today.

Disregard my comment as I like this. I realized OR logic with separate calls to OptionSecurity is implemented with this.

Final thoughts:

  1. Should we add some validation in OptionSecurity to ensure the schema being inputted is valid schema. If not we panic/exit?
  2. Could you add an example of using this into the examples/petstore? This will update our golden file open api spec. It helps us spot unwanted changes to spec outputs, kinda an integration test of sorts.

option_test.go Outdated Show resolved Hide resolved
option_test.go Show resolved Hide resolved
option_test.go Show resolved Hide resolved
option_test.go Outdated Show resolved Hide resolved
option_test.go Outdated Show resolved Hide resolved
@simonemastella
Copy link
Contributor Author

Was able to get to this today.

Disregard my comment as I like this. I realized OR logic with separate calls to OptionSecurity is implemented with this.

Final thoughts:

  1. Should we add some validation in OptionSecurity to ensure the schema being inputted is valid schema. If not we panic/exit?
  2. Could you add an example of using this into the examples/petstore? This will update our golden file open api spec. It helps us spot unwanted changes to spec outputs, kinda an integration test of sorts.

On point 2, yes of course I will work on it!
On point 1: Some validation could be done on the component declaration but if I remember correctly each type should have different properties (apikey / bearer / oauth2 / openIdConnect) and I would have to delve into which fields vary.
Also for when you “hook” a securityRequirement to a route in addition to checking whether that component has been declared some stricter checks could be added to avoid the same requirement being entered multiple times or with empty fields...

How strict do you think the validation rules should be?

Co-authored-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com>
@dylanhitt
Copy link
Collaborator

dylanhitt commented Nov 20, 2024

Oh sorry for not being clear. I just meant “ensure the security schema being asked on the route exists in the security schemas on the server.” Just check the map if not panic so we can tell the user early.

😅 sorry saying validate something in terms of OpenAPI can be super ambiguous.

@simonemastella
Copy link
Contributor Author

Oh sorry for not being clear. I just meant “ensure the security schema being asked on the route exists in the security schemas on the server.” Just check the map if not panic so we can tell the user early.

😅 sorry saying validate something in terms of OpenAPI can be super ambiguous.

ahahah perfect, do you think that something like this could be enough?
p.s. by the end of the week I should be able to modify the examples/petstore

                // Validate input is not empty
		if len(securityRequirements) == 0 {
			panic("at least one security requirement must be provided")
		}

		// Validate the security scheme exists in components
		for _, req := range securityRequirements {
			for schemeName := range req {
				if r.OpenAPI != nil && r.OpenAPI.Components != nil {
					if _, exists := r.OpenAPI.Components.SecuritySchemes[schemeName]; !exists {
						panic(fmt.Sprintf("security scheme '%s' not defined in components", schemeName))
					}
				}
			}
		}

@dylanhitt
Copy link
Collaborator

Yes, you can remove the nil checks as well as everything is new'd up when the server is created.

One thing that is interesting. If we moved away from the varadic input to just a single input we wouldn't need the extra for loop

option.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@ccoVeille ccoVeille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍, but minor remarks anyway

@@ -76,6 +76,38 @@ var Summary = fuego.OptionSummary
// Description adds a description to the route.
var Description = fuego.OptionDescription

// OptionSecurity configures security requirements to the route.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// OptionSecurity configures security requirements to the route.
// Security configures security requirements to the route.

You renamed it

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update this. And this should be good to go. 😄

Comment on lines +85 to +108
// OptionSecurity(openapi3.SecurityRequirement{
// "basic": [], // Requires basic auth
// "oauth2": ["read"] // AND requires oauth with read scope
// })
//
// Multiple Schemes (OR Logic):
//
// Add multiple security requirements.
// At least one requirement must be satisfied:
// OptionSecurity(
// openapi3.SecurityRequirement{"basic": []}, // First option
// openapi3.SecurityRequirement{"oauth2": ["read"]} // Alternative option
// })
//
// Mixing Approaches:
//
// Combine AND logic within requirements and OR logic between requirements:
// OptionSecurity(
// openapi3.SecurityRequirement{
// "basic": [], // Requires basic auth
// "oauth2": ["read:user"] // AND oauth with read:user scope
// },
// openapi3.SecurityRequirement{"apiKey": []} // OR alternative with API key
// })
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

Suggested change
// OptionSecurity(openapi3.SecurityRequirement{
// "basic": [], // Requires basic auth
// "oauth2": ["read"] // AND requires oauth with read scope
// })
//
// Multiple Schemes (OR Logic):
//
// Add multiple security requirements.
// At least one requirement must be satisfied:
// OptionSecurity(
// openapi3.SecurityRequirement{"basic": []}, // First option
// openapi3.SecurityRequirement{"oauth2": ["read"]} // Alternative option
// })
//
// Mixing Approaches:
//
// Combine AND logic within requirements and OR logic between requirements:
// OptionSecurity(
// openapi3.SecurityRequirement{
// "basic": [], // Requires basic auth
// "oauth2": ["read:user"] // AND oauth with read:user scope
// },
// openapi3.SecurityRequirement{"apiKey": []} // OR alternative with API key
// })
// Security(openapi3.SecurityRequirement{
// "basic": [], // Requires basic auth
// "oauth2": ["read"] // AND requires oauth with read scope
// })
//
// Multiple Schemes (OR Logic):
//
// Add multiple security requirements.
// At least one requirement must be satisfied:
// Security(
// openapi3.SecurityRequirement{"basic": []}, // First option
// openapi3.SecurityRequirement{"oauth2": ["read"]} // Alternative option
// })
//
// Mixing Approaches:
//
// Combine AND logic within requirements and OR logic between requirements:
// Security(
// openapi3.SecurityRequirement{
// "basic": [], // Requires basic auth
// "oauth2": ["read:user"] // AND oauth with read:user scope
// },
// openapi3.SecurityRequirement{"apiKey": []} // OR alternative with API key
// })

@@ -252,3 +253,27 @@ func OptionHide() func(*BaseRoute) {
r.Hidden = true
}
}

func OptionSecurity(securityRequirements ...openapi3.SecurityRequirement) func(*BaseRoute) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the comment could be added here

Copy link
Collaborator

@dylanhitt dylanhitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a fan of @ccoVeille requested changes. Once updated I'll approve

@EwenQuim EwenQuim merged commit 4cf4668 into go-fuego:main Dec 6, 2024
@EwenQuim
Copy link
Member

EwenQuim commented Dec 6, 2024

I've made the asked modifications myself on main.

Thank you a lot @simonemastella for this feature, and @dylanhitt & @ccoVeille for proofreading this! 🙏🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants