Feature/implement backend authentication #68

Zheng-Zhi-Qiang · 2024-02-18T09:46:49Z

Description

In this PR, I created the authentication and authorization middleware to authenticate requests in the backend.

*Assumptions:

All request will have to be authenticated first. Will not store any cookies for now.
To differentiate admins from users, will look for 'approve:pipeline_step' permission in user permissions

joshtyf

Let me do more research before approving this

joshtyf · 2024-02-21T12:13:09Z

backend/src/middleware/auth_middleware.go

+		accessToken := r.Header.Get("Authorization")
+		session, _ := store.Get(r, accessToken)


typically, the value in this header will be Bearer <token>. For example, using auth0, typically the frontend will get the token of type Bearer. They will then form the Bearer <token> string and pass it to the backend. You can see it from the React example provided directly from Auth0 https://auth0.com/docs/quickstart/spa/react/02-calling-an-api

So you will probably need to do some string parsing to get the token from the Authorization header.

joshtyf · 2024-02-21T12:18:20Z

backend/src/middleware/auth_middleware.go

+			logger.Error("[Authorization] Unable to verify token", map[string]interface{}{"err": err})
+			authError := &api.HandlerError{Error: errors.New("unable to verify token"), Code: http.StatusUnauthorized}
+			w.WriteHeader(authError.Code)
+			json.NewEncoder(w).Encode(authError)


encoding the error like this will not work. see my PR #65 description (or see this stack overflow answer https://stackoverflow.com/questions/44989924/golang-error-types-are-empty-when-encoded-to-json)

joshtyf · 2024-02-21T12:48:49Z

backend/src/authenticator/auth.go

+		ClientSecret: os.Getenv("AUTH0_CLIENT_SECRET"),
+		RedirectURL:  os.Getenv("AUTH0_CALLBACK_URL"),
+		Endpoint:     provider.Endpoint(),
+		Scopes:       []string{oidc.ScopeOpenID, "profile"},


should we be setting the scope here? I'm not too familiar

This was based off their quick start guide so we just stick with this first until changes are required bah since it doesn't seem to create any immediate issues in my mind

… and updated http err handling per PR#73

joshtyf

Key changes required:

I believe we should use access tokens instead of ID tokens
We should also verify the scope of the request, issuer domain as well as the audience of the token.

You can cross reference with the official auth0 Golang guide here

joshtyf · 2024-02-26T13:42:41Z

backend/src/authenticator/auth.go

+}
+
+func (a *Authenticator) VerifyIDToken(ctx context.Context, token *oauth2.Token) (*oidc.IDToken, error) {
+	rawIDToken, ok := token.Extra("id_token").(string)


what is this Extra("id_token") about actually? Can you provide a reference/documentation for me to take a look at the id_token field that is present in oauth2 JWT?

Also, I just read up more on the different token types for JWT. There is an ID token and Access token type. ID tokens should be used for authentication (i.e. a user is a valid user) and access tokens should be used for authorisation (i.e. a user is valid and is authorised to perform a certain action).

from https://auth0.com/docs/secure/tokens/id-tokens: ID Tokens should never be used to obtain direct access to APIs or to make authorization decisions.

So you'll need to consider which APIs require an ID token, and which ones require an Access token and use the correct middleware accordingly. In our scenario, I believe we should be using Access tokens instead.

The quick start guide I was using is post login when I created the application. The guide only showed how to integrate authentication and not authorisation, hence the discrepancy here I think. Ill just refer to the guide you mentioned.

joshtyf · 2024-02-26T13:46:57Z

backend/src/server/middleware.go

+func IsAuthenticated(next http.Handler) http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Add("Content-Type", "application/json")
+		accessToken := strings.TrimSpace(strings.Replace(r.Header.Get("Authorization"), "Bearer", "", 1))


I'm not too sure about the security implications about this method of doing things.

I think it's better to reject if the header doesn't contain the prefix Bearer, even if the token is correct. A better way is to do a strings.Split and verify 1) there is a Bearer prefix 2) there should only be the Bearer string and the token string after the split

joshtyf · 2024-02-26T13:55:03Z

backend/src/server/server.go

@@ -35,6 +35,7 @@ func addRoutes(r *mux.Router) {
 func New() http.Handler {
 	router := mux.NewRouter()
 	addRoutes(router)
+	router.Use(IsAuthenticated)


We have 2 options:

Create a global middleware that authenticates every request; create a authorisation middleware that we chain on every handler that requires authorisation (such as creating new pipelines, approvals, etc.)

Don't use a global middleware for authentication. On every handler (that requires authentication), we will chain this authentication middleware.

Method 1 reduces the amount of code to write but it assumes that every handler requires authentication. It might just be better to opt for method 2, even though we have to repeat ourselves every time. Thoughts?

reference: https://auth0.com/blog/id-token-access-token-what-is-the-difference/

Thing is I can't think of a situation where authentication is not required. The only possibility would be a server side landing page, but since our backend is purely just API, there should not be any paths where authorisation middleware is not required. In that case, since the only differentiation is between authorisation for different roles, I think we just do two bah

joshtyf · 2024-02-26T14:01:09Z

compose.yaml

@@ -20,6 +20,7 @@ services:
      - be
    build:
      context: backend
+    env_file: .env


you shouldn't need this field as .env is the default env_file that will be sourced.

It doesn't source from the .env for me though. I needed to add it in to see the env variables

where are you running your docker command from? your .env should be located in the root of the project. run your docker commands from the root as well.

…ation

…ncode cannot encode bytes properly

joshtyf

Code looks fine except the docker compose file and my comments. But you haven't used the middleware?

joshtyf · 2024-03-02T09:04:54Z

backend/src/server/helper.go

+// HasScope checks whether our claims have a specific scope.
+func (c CustomClaims) HasScope(expectedScope string) bool {
+	result := strings.Split(c.Scope, " ")
+	for i := range result {
+		if result[i] == expectedScope {
+			return true
+		}
+	}
+
+	return false
+}


why didn't you keep this method in the middleware.go file?

joshtyf · 2024-03-02T09:05:41Z

backend/src/server/middleware.go

+			jwtValidator.ValidateToken,
+			jwtmiddleware.WithErrorHandler(errorHandler),
+		)
+		logger.Info("Validating", nil)


can we get a better log message here?

joshtyf · 2024-03-02T09:07:11Z

backend/src/server/middleware.go

+		logger.Info("Authorizing", nil)
+		token := r.Context().Value(jwtmiddleware.ContextKey{}).(*validator.ValidatedClaims)
+		claims := token.CustomClaims.(*CustomClaims)
+		if !claims.HasScope("test") {


should the scope still be test?

…env variables required

joshtyf

Left some minor changes, other than that should be good

joshtyf · 2024-03-07T15:00:03Z

backend/src/server/server.go

@@ -15,21 +15,21 @@ func addRoutes(r *mux.Router) {
 	}

 	// Health Check
-	r.Handle("/api/healthcheck", handleHealthCheck()).Methods("GET")
+	r.Handle("/api/healthcheck", isAuthenticated(isAuthorisedAdmin(handleHealthCheck()))).Methods("GET")


I don't think we need to authorise the health check.

joshtyf · 2024-03-07T15:01:11Z

backend/src/server/server.go

+	r.Handle("/api/pipeline", isAuthenticated(isAuthorisedUser(handleGetAllPipelines(mongoClient)))).Methods("GET")
+	r.Handle("/api/pipeline/{pipelineId}", isAuthenticated(isAuthorisedUser(handleGetPipeline(mongoClient)))).Methods("GET")


Actually, basic users shouldn't be able to see the pipeline. They should only be able to see the service request that uses the latest version of the pipeline

joshtyf · 2024-03-07T15:01:53Z

backend/src/server/middleware.go

+// Validate does nothing for this example, but we need
+// it to satisfy validator.CustomClaims interface.


can remove this comment? or make it more relevant to our use case

…e between admin and user

…heck resource

joshtyf

LGTM. Except for the HasPermission in helper.go

joshtyf · 2024-03-08T08:56:47Z

backend/src/server/helper.go

+// HasPermission checks whether our claims have a specific permission.
+// In our case, since we are using this to check if user is admin, will be checking for approve:pipeline_step permission
+func (c CustomClaims) HasPermission(expectedPermission string) bool {
+	result := strings.Split(c.Permissions, ",")
+	for i := range result {
+		if result[i] == expectedPermission {
+			return true
+		}
+	}
+
+	return false
+}


wait why you move this here? why didn't you keep it with middleware.go?

this function is just a helper function to check if a permission exists, to me not really a middleware exactly

Zheng-Zhi-Qiang added 2 commits February 18, 2024 12:08

feat: created authenticator

7618acd

feat: added middleware to authenticate user

e37a5e8

Zheng-Zhi-Qiang added backend Backend related feature A new feature labels Feb 18, 2024

Zheng-Zhi-Qiang added this to the M2: More features milestone Feb 18, 2024

Zheng-Zhi-Qiang requested review from joshtyf, Ziyang-98 and lylaaron98 February 18, 2024 09:46

Zheng-Zhi-Qiang self-assigned this Feb 18, 2024

Zheng-Zhi-Qiang linked an issue Feb 18, 2024 that may be closed by this pull request

Authenticate requests in backend #41

Closed

joshtyf reviewed Feb 21, 2024

View reviewed changes

Zheng-Zhi-Qiang added 7 commits February 25, 2024 18:23

Merge branch 'main' into feature/implement-backend-authentication

8431734

refactor: updated handler error according to PR#68

493fd96

refactor: removed session store implementation for authentication

ada7e13

Merge branch 'main' into feature/implement-backend-authentication

e0b956f

refactor: moved auth middleware to middleware.go under server package…

946605b

… and updated http err handling per PR#73

fix: added auth middleware to server instantiation

c5100ed

fix: added string parsing to get access token from bearer token

87d36b7

Zheng-Zhi-Qiang requested a review from joshtyf February 25, 2024 15:01

fix: added missing logging to auth middleware

9adb4ab

joshtyf requested changes Feb 26, 2024

View reviewed changes

Zheng-Zhi-Qiang added 2 commits March 1, 2024 19:09

refactor: changed to auth functions for backend api instead of applic…

7f087b3

…ation

fix: changed healthcheck message sent to string instead of bytes as e…

f31b28e

…ncode cannot encode bytes properly

Zheng-Zhi-Qiang requested a review from joshtyf March 1, 2024 11:12

joshtyf requested changes Mar 2, 2024

View reviewed changes

Zheng-Zhi-Qiang added 3 commits March 3, 2024 16:19

refactor: applied respective authentication function to routes

e490f66

refactor: removed env_file setting from docker compose and specified …

2afbff7

…env variables required

refactor: removed uneccessary logging from auth functions

a56bd7f

joshtyf requested changes Mar 7, 2024

View reviewed changes

refactor: reduced authorization functions to only one to differentiat…

59bd685

…e between admin and user

fix: restricted pipeline resources to admins and unrestricted healthc…

bd45275

…heck resource

Zheng-Zhi-Qiang requested a review from joshtyf March 7, 2024 15:51

Zheng-Zhi-Qiang added 2 commits March 7, 2024 23:54

fix: merge conflict in compose.yaml

cdac4e5

fix: resolved merged conflicts

4ec3b0f

joshtyf approved these changes Mar 8, 2024

View reviewed changes

refactor: moved permission check function from helper to middleware

c396393

Zheng-Zhi-Qiang merged commit 5a83b0a into main Mar 8, 2024
2 checks passed

Zheng-Zhi-Qiang deleted the feature/implement-backend-authentication branch March 8, 2024 10:00

Zheng-Zhi-Qiang linked an issue Mar 17, 2024 that may be closed by this pull request

Integrate auth0 + google authentication #57

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/implement backend authentication #68

Feature/implement backend authentication #68

Zheng-Zhi-Qiang commented Feb 18, 2024 •

edited

Loading

joshtyf left a comment

joshtyf Feb 21, 2024 •

edited

Loading

joshtyf Feb 21, 2024

joshtyf Feb 21, 2024

Zheng-Zhi-Qiang Feb 25, 2024

joshtyf left a comment

joshtyf Feb 26, 2024

joshtyf Feb 26, 2024

Zheng-Zhi-Qiang Feb 26, 2024

joshtyf Feb 26, 2024

joshtyf Feb 26, 2024

Zheng-Zhi-Qiang Feb 26, 2024 •

edited

Loading

joshtyf Feb 26, 2024

Zheng-Zhi-Qiang Feb 27, 2024

joshtyf Mar 2, 2024

joshtyf left a comment

joshtyf Mar 2, 2024

joshtyf Mar 2, 2024

joshtyf Mar 2, 2024

joshtyf left a comment

joshtyf Mar 7, 2024

joshtyf Mar 7, 2024

joshtyf Mar 7, 2024

joshtyf left a comment

joshtyf Mar 8, 2024

Zheng-Zhi-Qiang Mar 8, 2024

		accessToken := r.Header.Get("Authorization")
		session, _ := store.Get(r, accessToken)

		r.Handle("/api/pipeline", isAuthenticated(isAuthorisedUser(handleGetAllPipelines(mongoClient)))).Methods("GET")
		r.Handle("/api/pipeline/{pipelineId}", isAuthenticated(isAuthorisedUser(handleGetPipeline(mongoClient)))).Methods("GET")

		// Validate does nothing for this example, but we need
		// it to satisfy validator.CustomClaims interface.

Feature/implement backend authentication #68

Feature/implement backend authentication #68

Conversation

Zheng-Zhi-Qiang commented Feb 18, 2024 • edited Loading

joshtyf left a comment

Choose a reason for hiding this comment

joshtyf Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshtyf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zheng-Zhi-Qiang Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshtyf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshtyf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshtyf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zheng-Zhi-Qiang commented Feb 18, 2024 •

edited

Loading

joshtyf Feb 21, 2024 •

edited

Loading

Zheng-Zhi-Qiang Feb 26, 2024 •

edited

Loading