go-fuego / fuego

Golang Fuego - web framework generating OpenAPI 3 spec from source code
https://go-fuego.github.io/fuego/
MIT License
918 stars 46 forks source link

added securityScheme #226

Open simonemastella opened 6 days ago

simonemastella commented 6 days ago

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

Route-Level Security

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

Breaking Changes

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

Example

image

simonemastella commented 3 days ago

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!

dylanhitt commented 1 day ago

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.
simonemastella commented 1 day ago

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?

dylanhitt commented 1 day ago

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 commented 1 day ago

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 commented 23 hours ago

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