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

Implement skipper middleware for basic-auth #509

Closed
3 of 5 tasks
yottahmd opened this issue Dec 20, 2023 · 3 comments · Fixed by #511
Closed
3 of 5 tasks

Implement skipper middleware for basic-auth #509

yottahmd opened this issue Dec 20, 2023 · 3 comments · Fixed by #511
Labels
good first issue Good for newcomers

Comments

@yottahmd
Copy link
Collaborator

yottahmd commented Dec 20, 2023

Current authentication method: Basic Authentication.
Proposed enhancement: Token-Based Authentication.
Motivation: To make it easier to use the API, especially for JavaScript applications running in a browser from a different domain. Refer to the discussion here for more context: GitHub Discussion #501.

Token auth implementation is already implemented in #508 .

The rest of work is to implement a skipper middleware for basic auth middleware to be skipped when the auth token is set in http request so that WebUI continues to be usable which does not send auth token.

To-do:

  • Implement the configuration option for token-based authentication (AuthToken field).
  • Create an environment variable (DAGU_AUTH_TOKEN) for token management.
  • Implement middleware for token-based authentication.
  • Add a high-order middleware WithSkipper to skip basic auth middleware when auth_token is set in HTTP Authorization header.
  • Test that the Web UI remains unaffected by these changes when auth_token is enabled.

Pseudo code:

func WithSkipper(middlewareFn, skipperFn func(r *http.Request) bool) func(next http.Handler) http.Handler {
    return func(next http.Handler) http.Handler {
        return func(w http.ResponseWriter, r *http.Request) {
          if skipperFn(r) {
            return next.ServeHTTP(r, w)
          }
          return middlewareFn.ServeHTTP(r, w)
        }
    }
}
@rafiramadhana
Copy link
Contributor

rafiramadhana commented Dec 31, 2023

@yohamta may I know why we need a skipperFn for this?

would user have the control to set a skipperFn fn?

if they wouldn't, i think we can have a AuthBasicFn fn instead (to follow the naming convention of AuthToken)

import (
        ...
	"github.com/go-chi/chi/v5/middleware"
)

func AuthBasicFn(realm string, creds map[string]string) func(next http.Handler) http.Handler {
	return func(next http.Handler) http.Handler {
		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
			// If auth token is set, skip basic auth middleware
			authHeader := strings.Split(r.Header.Get("Authorization"), " ")
			if len(authHeader) >= 2 && authHeader[0] == "Bearer" {
				next.ServeHTTP(w, r)
                                return
			}

			// Else, don't skip basic auth middleware
			middleware.BasicAuth(
				"restricted", map[string]string{basicAuth.Username: basicAuth.Password},
			)(next)
		})
	}
}

ref: https://github.com/dagu-dev/dagu/blob/main/service/frontend/middleware/tokenAuth.go#L11C2-L35C2

and update the SetupGlobalMiddleware

if authToken != nil {
	next = TokenAuth("restricted", authToken.Token)(next)
}

if basicAuth != nil {
	next = BasicAuthFn(
		"restricted", map[string]string{basicAuth.Username: basicAuth.Password},
	)(next)
}

ref: https://github.com/dagu-dev/dagu/blob/main/service/frontend/middleware/global.go#L16C6-L24C3

wdyt?

@yottahmd
Copy link
Collaborator Author

@rafiramadhana Thanks for the great suggestion! I agree, your approach seems more streamlined and straightforward. Just a quick reminder, we should ensure that Config.IsAuthToken is set to true when opting to skip the basic authentication middleware.

@rafiramadhana
Copy link
Contributor

Just a quick reminder, we should ensure that Config.IsAuthToken is set to true when opting to skip the basic authentication middleware.

got it, i will submit a PR

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

Successfully merging a pull request may close this issue.

2 participants