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

feat: add endpoint authenticaton #7

Merged
merged 1 commit into from
Nov 2, 2021

Conversation

ShwetaGhenand
Copy link
Owner

  1. Generate jwt token (based on user credentials and secret key)
  2. Validate jwt token

@ShwetaGhenand ShwetaGhenand force-pushed the feature/add-auth-middleware branch 3 times, most recently from cdaa22b to 4b3eae1 Compare October 27, 2021 13:35
return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
authorizationHeader := req.Header.Get("authorization")
secret := os.Getenv("SECRET")
if authorizationHeader != "" {
Copy link

@sebnyberg sebnyberg Oct 27, 2021

Choose a reason for hiding this comment

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

  • For authN (and often AuthZ too), do not return any messages. Fail in silence.
  • Configuration should be passed down from the root and declared/orchestrated in once place (the entrypoint). Avoid from fetching configuration outside the root.
  • Use if-statements primarily for early returns / errors, not for the happy path. Linking this again for reference: https://medium.com/@matryer/line-of-sight-in-code-186dd7cdea88
  • authorizationHeader != "" is redundant - strings.Split will not have two parts if the string is empty and performance gains are negligible.
  • Side-note / Not Applicable: when splitting on spaces, it's sometimes useful to use a more forgiving alternative: strings.Fields. See this example. However, theAuthorization HTTP header has a very strict definition (RFC6750) so in this scenario, strings.Split is the right choice.
  • The JWT library that you use here is not recommended. If you check the issue tracker, there's been an effort to move to a different repository. I found an alternative through jwt.io: https://github.com/lestrrat-go/jwx. Fun fact: the person who wrote that issue is one of the core contributors to Argo, a big and popular ecosystem of projects written in Go.

sr := s.router.PathPrefix("/users").Subrouter()
sr.HandleFunc("", s.createUser).Methods("POST")
sr.Use(authMiddleware())

Choose a reason for hiding this comment

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

Pass the secret value to the function here from the root.

@@ -1,10 +1,20 @@
package server

type userDto struct {
Copy link

@sebnyberg sebnyberg Oct 27, 2021

Choose a reason for hiding this comment

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

I haven't felt the need to make a JSON DTO / Service (Entity) model distinction in Go. Maybe due to how versatile tags are (for example a field can easily be excluded with json:"-"), and the natural separation of struct fields and their receivers (methods).

So I think this could just be User (even if it has JSON tags). To be strict, the current user struct (that is created by sqlc) feels more of a DTO - it is flavoured by our choice of persistence and SQL maybe-nil types. Maybe move the sqlc models into its own package, like userpg or dbgen? That would result in User and userpg.User.

Also try to refrain from using OOP-terms (like "entity" and "dto") in Go. While OOP principles are applicable and valuable, these principles and decisions can just as well be explained through well written comments. This makes the code more inclusive to those who do not come from an OOP background:

// somefile.go

// DBUser allows persistent storage of the User in the database.
// 
// User -> DBUser: user.AsDBUser().
// DBUser -> User: NewUserFromDBUser(dbUser)
type DBUser struct {
  // [...]
}
// someotherfile.go

// JSONUser contains a JSON view of the User model.
//
// This model is separate from User because x and y and z.
type JSONUser struct {
  // [...]
}

Copy link
Owner Author

Choose a reason for hiding this comment

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

Fixed by using JSONUser. I'll create a separate pr to move models, files to its own package:)

Address string `json:"address"`
}

type loginDto struct {
Copy link

@sebnyberg sebnyberg Oct 27, 2021

Choose a reason for hiding this comment

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

I don't think this loginDTO is valuable as a model in its own right. Instead I would put it right above where it's used, i.e. at the loginUser handler - see my comment on the handler.

writeError(w, err)
return
}
t, err := generateToken(c)
Copy link

@sebnyberg sebnyberg Oct 27, 2021

Choose a reason for hiding this comment

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

I think it would be cleaner to pass primitive values (a string) to this function rather than a struct. Especially since the struct is called "c".

queries.sql Outdated
) VALUES (
$1, $2, $3, $4, $5, $6
$1, $2, $3, $4, $5, $6, $7
)
RETURNING *;
Copy link

@sebnyberg sebnyberg Oct 27, 2021

Choose a reason for hiding this comment

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

Since there are no defaults in the table definition, there is no need to do RETURNING - it doesn't provide the caller with any information they didn't have already.

schema.sql Outdated
@@ -2,6 +2,7 @@
CREATE TABLE IF NOT EXISTS Users(
ID INT PRIMARY KEY NOT NULL,

Choose a reason for hiding this comment

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

While postgres doesn't have any official guidelines for naming conventions, it does casefold identifiers to lowercase unless an identifier is put within quotation marks. The idiosyncratic way of naming tables and columns is to use lower_snake_case.

Password string `json:"password"`
}

type tokenDto struct {
Copy link

@sebnyberg sebnyberg Oct 27, 2021

Choose a reason for hiding this comment

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

I don't feel like this struct adds any value at the moment.

Like, maybe if the generateToken function was instead made to be a constructor, then it could be worth having as a struct. But I still feel like it would just be easier to use the library directly instead and pass around the token a s a string / *jwt.Token.

In any case a Token would be something like this:

type JWTToken struct {
  t *jwt.Token
}

func NewJWTToken(name string) (*Token, error) {
  // put body of generateToken() here
}

func ParseJWTToken(tokenStr string) (*Token, error) {
  // put some portions of the middleware auth handler here
}

func (t *JWTToken) Valid() error {
  // put some portions of the middleware auth handler here
}

@@ -25,7 +25,7 @@ func checkError(err error) error {
ce.Err = errors.New("duplicate user id")
ce.Code = 400
default:
ce.Err = errors.New("database error")
ce.Err = fmt.Errorf("database error %v", err)

Choose a reason for hiding this comment

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

It doesn't matter here but when formatting errors, try to use the directive %w for "wrap".

This is a rabbit hole in its own right, but see this example on how errors can be wrapped:

https://play.golang.org/p/8_k_TspDVK1

@@ -37,6 +37,31 @@ func getHealth(w http.ResponseWriter, r *http.Request) {
}
}

Choose a reason for hiding this comment

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

Instead of having a login DTO, I would recommend creating a request struct here.

Like:

type loginUserRequest struct {
  Name `json:"name"`
  Password `json:"password"`
}

type loginUserResponse struct {
  Token `json:"token"`
}

This way the declaration is closer and more available for refactoring, and the request/response models are allowed to evolve without having to worry about what they represent.

@ShwetaGhenand ShwetaGhenand force-pushed the feature/add-auth-middleware branch 5 times, most recently from 81dfb9f to 31cd180 Compare November 2, 2021 09:39
@ShwetaGhenand ShwetaGhenand merged commit f3a3bc9 into main Nov 2, 2021
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.

2 participants