[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #3505: Oauth integration

[GitBox <gi...@apache.org>]

ocket8888 commented on a change in pull request #3505: Oauth integration
URL: https://github.com/apache/trafficcontrol/pull/3505#discussion_r291628933
 
 

 ##########
 File path: traffic_ops/traffic_ops_golang/login/login.go
 ##########
 @@ -104,3 +108,119 @@ func LoginHandler(db *sqlx.DB, cfg config.Config) http.HandlerFunc {
 		fmt.Fprintf(w, "%s", respBts)
 	}
 }
+
+func OauthLoginHandler(db *sqlx.DB, cfg config.Config) http.HandlerFunc {
+	return func(w http.ResponseWriter, r *http.Request) {
+		handleErrs := tc.GetHandleErrorsFunc(w, r)
+		defer r.Body.Close()
+		authenticated := false
+		resp := struct {
+			tc.Alerts
+		}{}
+
+		form := auth.PasswordForm{}
+		tokenForm := struct {
+			Token string `json:"t"`
+		}{}
+
+		if err := json.NewDecoder(r.Body).Decode(&tokenForm); err != nil {
+			handleErrs(http.StatusBadRequest, err)
+			return
+		}
+
+		encodedToken := tokenForm.Token
+
+		if encodedToken == "" {
+			log.Errorf("Token not found in request but is required")
+			handleErrs(http.StatusBadRequest, errors.New("Token not found in request but is required"))
+			return
+		}
+
+		decodedToken, err := jwt.Parse(encodedToken, func(unverifiedToken *jwt.Token) (interface{}, error) {
+			publicKeyUrl := unverifiedToken.Header["jku"].(string)
+			publicKeyId := unverifiedToken.Header["kid"].(string)
+
+			if !VerifyUrlOnWhiteList(publicKeyUrl, cfg.ConfigTrafficOpsGolang.WhitelistedOAuthUrls) {
+				return nil, errors.New("Key URL from token is not included in the whitelisted urls. Received: " + publicKeyUrl)
+			}
+
+			keys, err := jwk.FetchHTTP(publicKeyUrl)
+			if err != nil {
+				return nil, err
+			}
+
+			keyById := keys.LookupKeyID(publicKeyId)
+			selectedKey, err := keyById[0].Materialize()
+
+			if err != nil {
+				return nil, err
+			}
+
+			return selectedKey, nil
+		})
+		if err != nil {
+			handleErrs(http.StatusInternalServerError, errors.New("Error decoding token with message: "+err.Error()))
+			log.Errorf("Error decoding token: %s\n", err.Error())
+			return
+		}
+
+		authenticated = decodedToken.Valid
+
+		userId := decodedToken.Claims.(jwt.MapClaims)["sub"].(string)
+		form.Username = userId
+
+		userAllowed, err, blockingErr := auth.CheckLocalUserIsAllowed(form, db, time.Duration(cfg.DBQueryTimeoutSeconds)*time.Second)
+		if blockingErr != nil {
+			api.HandleErr(w, r, nil, http.StatusServiceUnavailable, nil, fmt.Errorf("error checking local user password: %s\n", blockingErr.Error()))
+			return
+		}
+		if err != nil {
+			log.Errorf("checking local user: %s\n", err.Error())
+			return
 
 Review comment:
   So I've just confirmed - if you login with an OAuth SSO user that doesn't exist in the database, this will return a 200 response. In Traffic Portal, this results in it trying to load the dashboard (or redirect page, I think) but then failing to load resources with 401 errors and the user is redirected to the login page and asked to log in again.
   
   The proper behavior here is to return a 403 with some appropriate message, and having looked at the normal user login code I don't see it bailing without setting an error response in this case. It _does_ do the error log the same way as this, but does not immediately return afterward, which is the problem here.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services