From 6a316dce231f4cd07889059e8a1b86032d832df8 Mon Sep 17 00:00:00 2001 From: Haoming Meng Date: Fri, 29 Mar 2024 21:26:03 +0000 Subject: [PATCH] Add "login required" endpoints behind "admin" check * Also fixed a bug where admin auth handler didn't abort at check failure --- web_ui/authentication.go | 7 ++- web_ui/authentication_test.go | 61 ++++++++++++++++--- .../app/api/docs/pelican-swagger.yaml | 35 ++++++----- web_ui/ui.go | 4 +- 4 files changed, 76 insertions(+), 31 deletions(-) diff --git a/web_ui/authentication.go b/web_ui/authentication.go index c3a6481ed..364d9f4fb 100644 --- a/web_ui/authentication.go +++ b/web_ui/authentication.go @@ -212,14 +212,15 @@ func AdminAuthHandler(ctx *gin.Context) { user := ctx.GetString("User") // This should be done by a regular auth handler from the upstream, but we check here just in case if user == "" { - ctx.JSON(http.StatusUnauthorized, gin.H{"error": "Login required to view this page"}) + ctx.AbortWithStatusJSON(http.StatusUnauthorized, gin.H{"error": "Login required to view this page"}) + return } isAdmin, msg := CheckAdmin(user) if isAdmin { ctx.Next() return } else { - ctx.JSON(http.StatusForbidden, gin.H{"error": msg}) + ctx.AbortWithStatusJSON(http.StatusForbidden, gin.H{"error": msg}) } } @@ -370,7 +371,7 @@ func configureAuthEndpoints(ctx context.Context, router *gin.Engine, egrp *errgr group.POST("/login", mw, loginHandler) group.POST("/logout", AuthHandler, logoutHandler) group.POST("/initLogin", initLoginHandler) - group.POST("/resetLogin", AuthHandler, resetLoginHandler) + group.POST("/resetLogin", AuthHandler, AdminAuthHandler, resetLoginHandler) // Pass csrfhanlder only to the whoami route to generate CSRF token // while leaving other routes free of CSRF check (we might want to do it some time in the future) group.GET("/whoami", csrfHandler, whoamiHandler) diff --git a/web_ui/authentication_test.go b/web_ui/authentication_test.go index 17286c353..381a369d9 100644 --- a/web_ui/authentication_test.go +++ b/web_ui/authentication_test.go @@ -37,6 +37,8 @@ import ( "github.com/pelicanplatform/pelican/config" "github.com/pelicanplatform/pelican/param" "github.com/pelicanplatform/pelican/test_utils" + "github.com/pelicanplatform/pelican/token" + "github.com/pelicanplatform/pelican/token_scopes" "github.com/spf13/viper" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -174,10 +176,10 @@ func TestPasswordResetAPI(t *testing.T) { assert.NoError(t, err) //Create a user for testing - err = WritePasswordEntry("user", "password") + user := "admin" // With admin privilege + err = WritePasswordEntry(user, "password") assert.NoError(t, err, "error writing a user") password := "password" - user := "user" payload := fmt.Sprintf(`{"user": "%s", "password": "%s"}`, user, password) //Create a request @@ -220,7 +222,7 @@ func TestPasswordResetAPI(t *testing.T) { assert.JSONEq(t, `{"msg":"Success"}`, recorderReset.Body.String()) //After password reset, test authorization with newly generated password - loginWithNewPasswordPayload := `{"user": "user", "password": "newpassword"}` + loginWithNewPasswordPayload := `{"user": "admin", "password": "newpassword"}` reqLoginWithNewPassword, err := http.NewRequest("POST", "/api/v1.0/auth/login", strings.NewReader(loginWithNewPasswordPayload)) assert.NoError(t, err) @@ -237,6 +239,39 @@ func TestPasswordResetAPI(t *testing.T) { assert.JSONEq(t, `{"msg":"Success"}`, recorderLoginWithNewPassword.Body.String()) }) + //Invoking password reset without a cookie should result in failure + t.Run("Without admin privilege", func(t *testing.T) { + resetPayload := `{"password": "newpassword"}` + reqReset, err := http.NewRequest("POST", "/api/v1.0/auth/resetLogin", strings.NewReader(resetPayload)) + assert.NoError(t, err) + + reqReset.Header.Set("Content-Type", "application/json") + + loginCookieTokenCfg := token.NewWLCGToken() + loginCookieTokenCfg.Lifetime = 30 * time.Minute + loginCookieTokenCfg.Issuer = param.Server_ExternalWebUrl.GetString() + loginCookieTokenCfg.AddAudiences(param.Server_ExternalWebUrl.GetString()) + loginCookieTokenCfg.Subject = "user" // general user + loginCookieTokenCfg.AddScopes(token_scopes.WebUi_Access, token_scopes.Monitoring_Query, token_scopes.Monitoring_Scrape) + + // CreateToken also handles validation for us + tok, err := loginCookieTokenCfg.CreateToken() + require.NoError(t, err) + + reqReset.AddCookie(&http.Cookie{ + Name: "login", + Value: tok, + }) + + recorderReset := httptest.NewRecorder() + router.ServeHTTP(recorderReset, reqReset) + + //Check ok http reponse + assert.Equal(t, 403, recorderReset.Code) + //Check that success message returned + assert.JSONEq(t, `{"error":"Registry.AdminUsers is not set, and user is not root user. Admin check returns false"}`, recorderReset.Body.String()) + }) + //Invoking password reset without a cookie should result in failure t.Run("Without valid cookie", func(t *testing.T) { resetPayload := `{"password": "newpassword"}` @@ -464,9 +499,6 @@ func TestWhoamiAPI(t *testing.T) { } func TestAdminAuthHandler(t *testing.T) { - // Initialize Gin and set it to test mode - gin.SetMode(gin.TestMode) - // Define test cases testCases := []struct { name string @@ -527,13 +559,22 @@ func TestAdminAuthHandler(t *testing.T) { }, } + // Initialize Gin and set it to test mode + gin.SetMode(gin.TestMode) + for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { w := httptest.NewRecorder() - ctx, _ := gin.CreateTestContext(w) - tc.setupUserFunc(ctx) - - AdminAuthHandler(ctx) + router := gin.Default() + // If admin middleware didn't abort, the response will have status code == 200 + router.GET("/test", + func(ctx *gin.Context) { tc.setupUserFunc(ctx) }, + AdminAuthHandler, + func(ctx *gin.Context) { ctx.AbortWithStatus(http.StatusOK) }, + ) + req, err := http.NewRequest("GET", "/test", nil) + require.NoError(t, err) + router.ServeHTTP(w, req) assert.Equal(t, tc.expectedCode, w.Code) if tc.expectedError != "" { diff --git a/web_ui/frontend/app/api/docs/pelican-swagger.yaml b/web_ui/frontend/app/api/docs/pelican-swagger.yaml index cc369a934..ba29ea468 100644 --- a/web_ui/frontend/app/api/docs/pelican-swagger.yaml +++ b/web_ui/frontend/app/api/docs/pelican-swagger.yaml @@ -14,6 +14,8 @@ info: The cookie is issued after a successful call to `/api/v1.0/auth/login`. However, OpenAPI 2.0 does not support specifying cookie-based security check. Therefore, as an alternative, we will add `Authentication Required` to the API description where needed. + For endpoint that is only accesible by users with admin privilege, we mark the endpoint as `Admin privilege Required`. + For how to set up Pelican servers, please refer to the documentation at [docs.pelicanplatform.org](https://docs.pelicanplatform.org/)" license: @@ -398,7 +400,8 @@ paths: tags: - common summary: Return the configuration values of the server and their type - description: "`Authentication Required`" + description: >- + `Authentication Required` `Admin privilege Required` produces: - application/json responses: @@ -446,7 +449,7 @@ paths: - "common" summary: Update the server configuration with user-provided changes description: - "`Authentication Required` `Admin Previlege Required` + "`Authentication Required` `Admin privilege Required` The server will restart promptly after a successful request. You may query against @@ -531,7 +534,7 @@ paths: tags: - metrics summary: Returns the health status of server components - description: "`Authentication Required`" + description: "`Authentication Required` `Admin Privilege Required" produces: - application/json responses: @@ -638,7 +641,7 @@ paths: tags: - auth summary: Reset the password for the user - description: "`Authentication Required`" + description: "`Authentication Required` `Admin Privilege Required`" consumes: - application/json produces: @@ -912,7 +915,7 @@ paths: description: "`Authentication Required` - For user with admin previlege, it returns for all valid namespace request. + For user with admin privilege, it returns for all valid namespace request. For general users, it only returns namespace belonging to the user, or it returns 404 @@ -943,7 +946,7 @@ paths: type: object $ref: "#/definitions/ErrorModel" "404": - description: Namespace not found, either does not exists or the user doesn't have previlege to get it + description: Namespace not found, either does not exists or the user doesn't have privilege to get it schema: type: object $ref: "#/definitions/ErrorModel" @@ -959,7 +962,7 @@ paths: description: "`Authentication Required` - For user with admin previlege, they can update any valid namespace. + For user with admin privilege, they can update any valid namespace. Non-admin users can update only namespaces they own and only if the approval status is `admin_metadata.status == approved`, otherwise the endpoint returns 404. @@ -1003,7 +1006,7 @@ paths: type: object $ref: "#/definitions/ErrorModel" "403": - description: The user does not have previlege to update the namespace + description: The user does not have privilege to update the namespace schema: type: object $ref: "#/definitions/ErrorModel" @@ -1019,7 +1022,7 @@ paths: $ref: "#/definitions/ErrorModel" delete: summary: Delete a namespace registration by ID - description: "`Authentication Required` `Admin` + description: "`Authentication Required` `Admin privilege Required` Only admin users have privilege to this action. " @@ -1045,7 +1048,7 @@ paths: type: object $ref: "#/definitions/ErrorModel" "403": - description: The user does not have previlege to update the namespace + description: The user does not have privilege to update the namespace schema: type: object $ref: "#/definitions/ErrorModel" @@ -1081,7 +1084,7 @@ paths: type: object $ref: "#/definitions/ErrorModel" "404": - description: Namespace not found, either does not exist or the user doesn't have previlege to get it + description: Namespace not found, either does not exist or the user doesn't have privilege to get it schema: type: object $ref: "#/definitions/ErrorModel" @@ -1101,7 +1104,7 @@ paths: Update namespace status to `approved` by namespace `id`. - This action requires admin previlege to perform. + This action requires admin privilege to perform. " parameters: - name: id @@ -1133,7 +1136,7 @@ paths: type: object $ref: "#/definitions/ErrorModel" "403": - description: The user does not have previlege to update the namespace status + description: The user does not have privilege to update the namespace status schema: type: object $ref: "#/definitions/ErrorModel" @@ -1158,7 +1161,7 @@ paths: Update namespace status to `denied` by namespace `id`. - This action requires admin previlege to perform. + This action requires admin privilege to perform. " parameters: - name: id @@ -1190,7 +1193,7 @@ paths: type: object $ref: "#/definitions/ErrorModel" "403": - description: The user does not have previlege to update the namespace status + description: The user does not have privilege to update the namespace status schema: type: object $ref: "#/definitions/ErrorModel" @@ -1261,7 +1264,7 @@ paths: patch: summary: Filter a server from director redirecting description: | - `Authentication Required` `Admin` + `Authentication Required` `Admin privilege Required` **The filter rules registered by this endpoint are in-memory and will be reset at the server restart.** diff --git a/web_ui/ui.go b/web_ui/ui.go index 656466690..63525b3bd 100644 --- a/web_ui/ui.go +++ b/web_ui/ui.go @@ -240,7 +240,7 @@ func configureWebResource(engine *gin.Engine) error { // Configure common endpoint available to all server web UI which are located at /api/v1.0/* func configureCommonEndpoints(engine *gin.Engine) error { - engine.GET("/api/v1.0/config", AuthHandler, getConfigValues) + engine.GET("/api/v1.0/config", AuthHandler, AdminAuthHandler, getConfigValues) engine.PATCH("/api/v1.0/config", AuthHandler, AdminAuthHandler, updateConfigValues) engine.GET("/api/v1.0/servers", getEnabledServers) // Health check endpoint for web engine @@ -258,7 +258,7 @@ func configureMetrics(engine *gin.Engine) error { prometheusMonitor := ginprometheus.NewPrometheus("gin") prometheusMonitor.Use(engine) - engine.GET("/api/v1.0/metrics/health", AuthHandler, func(ctx *gin.Context) { + engine.GET("/api/v1.0/metrics/health", AuthHandler, AdminAuthHandler, func(ctx *gin.Context) { healthStatus := metrics.GetHealthStatus() ctx.JSON(http.StatusOK, healthStatus) })