-
Notifications
You must be signed in to change notification settings - Fork 53
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
Improve the Header Authentication Method #16
Conversation
d7a86e4
to
d499213
Compare
cmd/admin/auth.go
Outdated
|
||
if username == "" { | ||
log.Printf("A username is required to use this system.") | ||
w.WriteHeader(http.StatusBadRequest) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other authentication methods I have used a redirect instead of writing the header:
http.Redirect(w, r, forbiddenPath, http.StatusFound)
Do you think we should use WriteHeader
instead?
cmd/admin/auth.go
Outdated
|
||
// This user didn't present a group that has permission to use the service | ||
if _, ok := s["level"]; !ok { | ||
w.WriteHeader(http.StatusForbidden) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
cmd/admin/auth.go
Outdated
return | ||
} | ||
|
||
err := adminUsers.UpdateMetadata(r.RemoteAddr, r.Header.Get("User-Agent"), username) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this authentication method does not require to pre-create users, better follow these steps:
1 - Check if user already exists
2 - If not, create user
3 - Update metadata
I would replace this block (135-138) with the following code:
if !adminUsers.Exists(username) {
log.Printf("user not found, creating new user: %s", username)
email := r.Header.Get(headersConfig.TrustedPrefix + headersConfig.Email)
fullname := r.Header.Get(headersConfig.TrustedPrefix + headersConfig.DisplayName)
newUser, err := adminUsers.New(username, "", email, fullname, (s["level"] == adminLevel))
if err != nil {
log.Printf("error creating user %s: %v", username, err)
http.Redirect(w, r, forbiddenPath, http.StatusFound)
return
}
} else {
if err := adminUsers.UpdateMetadata(r.RemoteAddr, r.Header.Get("User-Agent"), username); err != nil {
log.Printf("error updating metadata for user %s: %v", username, err)
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some comments, great job!
This adds more of the code needed to properly authenticate a user based on headers present added by a reverse proxy.
d499213
to
227036d
Compare
cmd/admin/auth.go
Outdated
|
||
if !adminUsers.Exists(username) { | ||
log.Printf("User not found, creating: %s", username) | ||
_, err := adminUsers.New(username, "", fullname, (s["level"] == adminLevel)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is one string
parameter missing, also my bad since I forgot that .New()
just creates the AdminUser
structure, and in order to complete the creation of the user, it needs a subsequent call to .Create()
, for example:
email := r.Header.Get(headersConfig.TrustedPrefix + headersConfig.Email)
newUser, err := adminUsers.New(username, "", email, fullname, (s["level"] == adminLevel))
if err != nil {
log.Printf("Error with new user %s: %v", username, err)
http.Redirect(w, r, forbiddenPath, http.StatusFound)
return
}
if err := adminUsers.Create(newUser); err != nil {
log.Printf("Error creating user %s: %v", username, err)
http.Redirect(w, r, forbiddenPath, http.StatusFound)
return
}
Troubleshooting this build error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This adds more of the code needed to properly authenticate a user based on headers present added by a reverse proxy.
I'm wary of the CSRF token line that's commented out but I must just be missing something there. Also updating users always fails and I assume that's because the login flow never runs with header authentication.
The way this is designed to work is to be put behind a proxy that handles the authentication for you that then passes the request along with newly added trusted headers from the proxy.
Admin vs User level permissions are handled by checking for the presence of specified groups passed along from the proxy.