Make Validator and ContextKey required input

[DavZim]

This PR makes the Validator function as well as the ContextKey (API Key) a required input for keyauth.

It also adds some basic tests to keyauth

[DavZim]

This addresses @jozsefsallai point in #86

[ReneWerner87]

check the review comments

[DavZim]

Maybe I have a misunderstanding: ContextKey is the name of the (locals) variable in which the actual key is saved and not the key itself, right?
At the moment I use contextKey as the actual key/password.

[ReneWerner87]

Maybe I have a misunderstanding: ContextKey is the name of the (locals) variable in which the actual key is saved and not the key itself, right? At the moment I use contextKey as the actual key/password.

Correct, contextKey is the key under which the data is stored in the local variables

[jozsefsallai]

I agree w/ the above, I don't think we should change anything in the context locals without the user's knowledge. There's no real reason for doing so anyway (that I can think of).

[DavZim]

Hi all,
I am revisiting this PR and trying to finish/close it.
IIRC the todos are: write the password to locals using the name given by "ContextKey" and update the test cases accordingly instead of using "ContextKey" to store the key.

But then where do we specify the actual key, does that mean Config should get a new argument or would the user have to write the key to the fiber context separetely? (at the moment we specify the password as contextKey and use that in the validateAPIKey function.)

Or in other words, is this what you had in mind?:

Add another element to Config eg ApiKey which holds the actual key (~password), save this key to c.Locals(cfg.ContextKey, cfg.ApiKey) and then in the Validator function use key == c.Locals(c.Locals("ContextKey")) (keeping in mind that ContextKey is a variable set by the user, with a default value of "token")

[ReneWerner87]

The context key already exists in the config and is specified per configuration by the user
I meant we should work in the locals also with this specified string and not with the fixed word "contextKey".

[ReneWerner87]

Will try tomorrow to provide an example or a tutorial or change (depending on how much time I have tomorrow)

[DavZim]

At the moment, I would envision the functionality like so:

// setup the fiber endpoint
app := fiber.New()

// use keyauth.New and keyauth.Config outside of testing
app.Use(New(Config{
  KeyLookup: "header:api-key",
  Validator:  validateAPIKey,         // function to compare a given api-key found in header to the ApiKey below
  ContextKey: "token1",                  // save the ApiKey in the Middleware as "token1" in c.Locals; a second middleware might use token2
  ApiKey: "MySecretPassword",    // the key, if given correctly, which grants access to the API
}))

Then inside keyauth, the string "MySecretPassword" is saved to c.Locals("token1", ApiKey) and used by the validateAPIKey function.

One Problem that I see, is that when we have multiple instances of the middleware, we need to store multiple ApiKeys and need to have multiple ContextKeys, and because the ContextKeys are different, we also need to have multiple Validator functions.

Is it possible to make it easier, so that only the ApiKey must be provided even when multiple middlewares are used?

[DavZim]

Note that the last two commits would implement that idea

[ReneWerner87]

okay now I am completely through

don't think we need an apiKey in the configuration

important is only that the validation function is made required
the rest of the process with the locals etc I would not change anything

if we would specify an apiKey in the configuration, we would limit the whole middelware by saying that only one apiKey is possible

currently it is up to the creator of the validation function to validate the passed key which comes from a resource(header, queryParameter, bodyParameter, cookie), where in the background also several different apiKeys can be considered valid
e.g. you could query a database or other data resource in the validation method

the key which was then determined as valid, goes into the locals, whereby one can recognize also other users possibly at different api key's and a usermanagent ontop can suspend

actually the readme already contains the perfect example, but maybe we should extend it to make it even clearer

as a change in the code i would only accept the panic for a missing validation method

[DavZim]

I have updated the code (by removing most of the changes, except the panic in case no validator is provided, as well as I kept the tests).
The latest commits also add tests for the different auth sources (header, cookie, query, and form), there is a bug (?) in the param authentication but I will open a separate issue.

As far as I understood your intentions @ReneWerner87, your points should be taken care of.

[DavZim]

Note that the test cases for param needed some small changes in the key extraction code (see b759180). Notably, it uses twice (!) the url.PathUnescape() function to unescape the key. This is required as otherwise special characters would break the authentication, The test key has also special characters, so all auth types support and are tested with special characters in the key.

[ReneWerner87]

please use subtests in the loops
https://go.dev/blog/subtests

and explain the url escape case more
(cannot find the url with the bad character)

[DavZim]

I am working on the subtest cases and will push later.

Regarding the url.PathUnescapes. I looked into it and it was only an issue in the tests. Using app := fiber.New(fiber.Config{UnescapePath: true}) fixes the issue in the tests and I have removed the the second path unescapes with the latest commit.
The issue only arose when using an escaped character in the key (eg APIKey = ": \"", note the \"), which we have in the test cases.

An example would be the following app:

package main

import (
	"github.com/gofiber/fiber/v2"
	"github.com/gofiber/keyauth/v2"
)

var (
	APIKey = "specials: !$%,.#\"!?~`<>@$^*(){}[]|/\\123"
)

func validateAPIKey(c *fiber.Ctx, key string) (bool, error) {
	fmt.Printf("Comparing keys:\nGOT:  '%v'\nWANT: '%v'\n===> %v\n", key, APIKey, key == APIKey)
	if key == APIKey {
		return true, nil
	}
	return false, ErrMissingOrMalformedAPIKey
}

func main() {
	app := fiber.New()
  
	app.Use("/key/:access_token", keyauth.New(keyauth.Config{
		KeyLookup:  "param:access_token",
		Validator:  validateAPIKey,
	}))

	app.Get("/key/:access_token", func(c *fiber.Ctx) error {
		return c.SendString("Successfully authenticated!")
	})

	app.Listen(":3000")
}

which should authenticate with

curl http://localhost:3000/key/specials%3A%20%21%24%25%2C.%23%22%21%3F~%60%3C%3E%40%24%5E%2A%28%29%7B%7D%5B%5D%7C%2F%5C123
#> Comparing keys:
#> GOT:  'specials: !$%,.#"!?~`<>@$^*(){}[]|/\123'
#> WANT: 'specials: !$%,.#"!?~`<>@$^*(){}[]|/\123'
#> ===> true

specials%3A%20%21%24%25%2C.%23%22%21%3F~%60%3C%3E%40%24%5E%2A%28%29%7B%7D%5B%5D%7C%2F%5C123 is converted to

specials: !$%,.#"!?~`<>@$^*(){}[]|/\123

[ReneWerner87]

thx, after the change for the subtests i will merge it

[DavZim]

Tests are restructured to use subtests. The output looks a lot cleaner now.
Thank you for the hints and comments along this PR!

=== RUN   TestKeyAuth
--- PASS: TestKeyAuth (0.00s)
=== RUN   TestAuthSources
=== RUN   TestAuthSources/header
=== RUN   TestAuthSources/cookie
=== RUN   TestAuthSources/query
=== RUN   TestAuthSources/param
=== RUN   TestAuthSources/form
--- PASS: TestAuthSources (0.00s)
    --- PASS: TestAuthSources/header (0.00s)
    --- PASS: TestAuthSources/cookie (0.00s)
    --- PASS: TestAuthSources/query (0.00s)
    --- PASS: TestAuthSources/param (0.00s)
    --- PASS: TestAuthSources/form (0.00s)
=== RUN   TestMultipleKeyAuth
--- PASS: TestMultipleKeyAuth (0.00s)
PASS
ok      github.com/gofiber/keyauth/v2   0.005s

[ReneWerner87]

@DavZim can you do the same for TestKeyAuth and TestMultipleKeyAuth in the loop

[DavZim]

Revisiting the TestKeyAuth; all test cases are covered by the TestAuthSources scenario. I would remove the former, then we don't need to reorganize the TestMultipleKeyAuth test scenario into subtests. Is that OK with you as well?

[ReneWerner87]

yes is ok for me

[DavZim]

All code is pushed. From my side we are good to merge!