Adds implementation for Auth Server Using Goth #315

PuneetPunamiya · 2021-08-30T10:36:04Z

Changes

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Run API Unit Tests, Lint Checks, API Design, Golden Files with make api-check
Run UI Unit Tests, Lint Checks with make ui-check
Commit messages follow commit message best practices

See the contribution guide for more details.

PuneetPunamiya · 2021-08-30T10:37:49Z

api/cmd/auth/auth.go

+	key := ""            // Replace with your SESSION_SECRET or similar
+	maxAge := 86400 * 30 // 30 days
+	isProd := false      // Set to true when serving over https
+
+	store := sessions.NewCookieStore([]byte(key))
+	store.MaxAge(maxAge)
+	store.Options.Path = "/"
+	store.Options.HttpOnly = true // HttpOnly should always be enabled
+	store.Options.Secure = isProd
+
+	gothic.Store = store


For now we are adding this to avoid the comment from the goth library while running the auth server

sm43 · 2021-09-01T10:50:45Z

api/cmd/auth/auth.go

+
+	logger.Info(fmt.Sprintf("listening on %s", AUTH_BASE_URL))
+
+	logger.Fatal(http.ListenAndServe(":4200",


having this as separate dir, does it means it will have a separate image?

if it is possible we should try to run this on different port but with the existing api server

running this server as a thread

maintaining different images not a good idea if we can package in one

Yes it will have a separate image

I think the ultimate aim of having a separate auth server is it's reusability and transparency from api server, running it in existing api server with different port might break this. Also if someone just wants the auth server, but building the image would also have the api in it, which would be of no use for the user

Also do you mean adding it in existing server means designing the api endpoints for auth using GOA,
If yes then for auth we have used Gorilla mux, the reason behind using gorilla was Goth library which we are using for authentication requires http.ResponseWriter and http.Request, which were not able to access using GOA

sm43 · 2021-09-01T10:56:28Z

api/pkg/auth/auth.go

+
+	var UI_URL = os.Getenv("UI_URL")
+	params := req.URL.Query()
+	if err = r.insertData(ghUser, params.Get("code")); err != nil {


is code available with all provider?

what is the primary info across all platform? mail id could be

is code available with all provider?

yes

what is the primary info across all platform? mail id could be

Yes we can have mail as primary for all the platforms but saying that we need to keep in mind for the users who are already logged in before we add that

api/cmd/api/main.go

api/pkg/auth/auth.go

api/.env.dev

api/pkg/db/migration/202109201733_create_user_table.go

api/pkg/oauth/auth/base.go

api/pkg/oauth/auth/service/auth.go

vinamra28

a couple of comments else looks fine from my side.

api/.env.dev

vinamra28 · 2021-11-15T15:16:02Z

api/cmd/api/main.go

+	go func() {
+		// start the web server on port and accept requests
+		logger.Infof("AUTH server listening on port %q", authPort)
+		logger.Fatal(http.ListenAndServe(":"+authPort,


yes if auth server returns error at some point then the api server would still be in running state.

2021-11-15T20:44:36.247+0530 INFO log/logger.go:33 HTTP Request {"service": "http", "id": "aq_UyPLp", "status": 200, "bytes": 611, "time": "1.071069ms"} 2021-11-15T20:44:36.247+0530 ERROR service/auth.go:143 hello world error {"service": "auth"} github.com/tektoncd/hub/api/pkg/auth/service.(*service).HubAuthenticate /home/vj/go/src/github.com/tektoncd/hub/api/pkg/auth/service/auth.go:143 net/http.HandlerFunc.ServeHTTP /usr/local/go/src/net/http/server.go:2049 github.com/gorilla/mux.(*Router).ServeHTTP /home/vj/go/pkg/mod/github.com/gorilla/mux@v1.8.0/mux.go:210 github.com/gorilla/handlers.(*cors).ServeHTTP /home/vj/go/pkg/mod/github.com/gorilla/handlers@v1.5.1/cors.go:138 net/http.serverHandler.ServeHTTP /usr/local/go/src/net/http/server.go:2867 net/http.(*conn).serve /usr/local/go/src/net/http/server.go:1932 2021-11-15T20:44:36.248+0530 INFO log/logger.go:33 HTTP Request {"service": "http", "id": "FCXdNplQ", "req": "GET /v1/catalogs", "from": "::1"} 2021-11-15T20:44:36.248+0530 INFO log/logger.go:33 HTTP Request {"service": "http", "id": "kYJJ0u5Q", "req": "GET /resources", "from": "::1"} 2021-11-15T20:44:36.249+0530 INFO app/gorm_logger.go:85 /home/vj/go/src/github.com/tektoncd/hub/api/v1/service/catalog/catalog.go:46 [1.664ms] [rows:1] SELECT * FROM "catalogs" WHERE "catalogs"."deleted_at" IS NULL ORDER BY id {"service": "catalog", "id": "FCXdNplQ"}

If you see the above case I manually logged the error randomly in one of the auth functions, the API server still worked

I think that should be the case only right, I mean if ui is showing the error for auth then api server should be still running right ??

vinamra28 · 2021-11-15T15:24:06Z

api/pkg/auth/base.go

+
+	key := ""            // Replace with your SESSION_SECRET or similar
+	maxAge := 86400 * 30 // 30 days
+	isProd := false      // Set to true when serving over https


should configure this on the basis of env variable ENVIRONMENT ?

How about when we read providers from env ?

isProd := false we will be serving over https right?

Are we adding an env in config? How we are changing it?

vinamra28 · 2021-11-15T15:42:51Z

api/pkg/auth/service/auth.go

+// Provides a list of git provider present in auth server
+func List(res http.ResponseWriter, req *http.Request) {
+	providers := authApp.ProviderList{
+		Data: []authApp.Provider{
+			{


can we add a TODO comment over here to configure these values dynamically?

when we want to add a new provide let say google? will there be code change?

I mean the logic for adding the provider for auth

vinamra28 · 2021-11-15T15:44:14Z

/cc @piyush-garg @sm43

sm43 · 2021-11-16T05:08:38Z

api/pkg/service/admin/admin.go

@@ -24,13 +24,13 @@ import (
 	"github.com/tektoncd/hub/api/pkg/app"
 	"github.com/tektoncd/hub/api/pkg/db/initializer"
 	"github.com/tektoncd/hub/api/pkg/db/model"
-	"github.com/tektoncd/hub/api/pkg/service/auth"
+	"github.com/tektoncd/hub/api/pkg/service/utils"


service named utils 😅 ?

sm43 · 2021-11-16T05:13:59Z

api/pkg/auth/base.go

+	authSvc := auth.New(api)
+
+	// Return name and status of the services
+	r.HandleFunc("/", auth.Status)


yes we have !

- Removes the design for auth - Updates the test for admin, catalog, rating, user Signed-off-by: Puneet Punamiya <ppunamiy@redhat.com>

vinamra28

/lgtm

vinamra28

/lgtm

api/pkg/auth/service/auth_test.go

Signed-off-by: Puneet Punamiya <ppunamiy@redhat.com>

pratap0007 · 2021-11-17T12:05:08Z

/lgtm

sm43 · 2021-11-18T02:59:22Z

/approve
/meow

tekton-robot · 2021-11-18T02:59:24Z

@sm43:

In response to this:

/approve
/meow

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tekton-robot · 2021-11-18T02:59:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sm43

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [sm43]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tekton-robot requested review from sm43 and vdemeester August 30, 2021 10:36

tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 30, 2021

PuneetPunamiya commented Aug 30, 2021

View reviewed changes

PuneetPunamiya changed the title ~~Adds implementation for Auth Server Using Goth~~ [WIP] Adds implementation for Auth Server Using Goth Aug 30, 2021

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 30, 2021

PuneetPunamiya force-pushed the auth-server branch 2 times, most recently from ce9cbf3 to 468ae1e Compare August 30, 2021 11:10

sm43 reviewed Sep 1, 2021

View reviewed changes

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 2, 2021

PuneetPunamiya force-pushed the auth-server branch from 468ae1e to 0c9b913 Compare September 6, 2021 06:19

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 6, 2021

PuneetPunamiya force-pushed the auth-server branch 2 times, most recently from 5b32506 to 715b24b Compare September 6, 2021 07:09

pratap0007 reviewed Sep 6, 2021

View reviewed changes

api/cmd/api/main.go Show resolved Hide resolved

pratap0007 reviewed Sep 9, 2021

View reviewed changes

api/pkg/auth/auth.go Outdated Show resolved Hide resolved

vinamra28 reviewed Sep 13, 2021

View reviewed changes

api/pkg/auth/auth.go Outdated Show resolved Hide resolved

api/pkg/auth/auth.go Outdated Show resolved Hide resolved

PuneetPunamiya force-pushed the auth-server branch 3 times, most recently from bda882e to a9491cd Compare September 14, 2021 14:12

PuneetPunamiya force-pushed the auth-server branch 3 times, most recently from 1467005 to b03b080 Compare September 20, 2021 13:42

PuneetPunamiya changed the title ~~[WIP] Adds implementation for Auth Server Using Goth~~ Adds implementation for Auth Server Using Goth Sep 20, 2021

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 20, 2021

pratap0007 reviewed Sep 21, 2021

View reviewed changes

api/.env.dev Outdated Show resolved Hide resolved

pratap0007 reviewed Sep 22, 2021

View reviewed changes

api/pkg/db/migration/202109201733_create_user_table.go Outdated Show resolved Hide resolved

pratap0007 reviewed Sep 22, 2021

View reviewed changes

api/pkg/oauth/auth/base.go Outdated Show resolved Hide resolved

pratap0007 reviewed Sep 22, 2021

View reviewed changes

api/pkg/oauth/auth/service/auth.go Outdated Show resolved Hide resolved

PuneetPunamiya force-pushed the auth-server branch from f581371 to 6b82ad9 Compare November 15, 2021 04:58

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 15, 2021

vinamra28 reviewed Nov 15, 2021

View reviewed changes

tekton-robot requested review from piyush-garg and sm43 November 15, 2021 15:44

sm43 reviewed Nov 16, 2021

View reviewed changes

api/pkg/auth/base.go

authSvc := auth.New(api)

// Return name and status of the services

r.HandleFunc("/", auth.Status)

Copy link

Member

sm43 Nov 16, 2021

Choose a reason for hiding this comment

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

yes we have !

Removes design for auth from the Hub API server

7e962b4

- Removes the design for auth - Updates the test for admin, catalog, rating, user Signed-off-by: Puneet Punamiya <ppunamiy@redhat.com>

PuneetPunamiya force-pushed the auth-server branch 2 times, most recently from 4ff850d to 324c971 Compare November 16, 2021 06:26

vinamra28 reviewed Nov 16, 2021

View reviewed changes

tekton-robot assigned vinamra28 Nov 16, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 16, 2021

PuneetPunamiya force-pushed the auth-server branch from 324c971 to 596c143 Compare November 16, 2021 13:27

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 16, 2021

vinamra28 reviewed Nov 17, 2021

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 17, 2021

pratap0007 reviewed Nov 17, 2021

View reviewed changes

api/pkg/auth/service/auth_test.go Outdated Show resolved Hide resolved

Adds design implementation for Auth Server using Goth

5ec7359

Signed-off-by: Puneet Punamiya <ppunamiy@redhat.com>

PuneetPunamiya force-pushed the auth-server branch from 596c143 to 5ec7359 Compare November 17, 2021 11:31

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 17, 2021

tekton-robot assigned pratap0007 Nov 17, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 17, 2021

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 18, 2021

tekton-robot merged commit 013101e into tektoncd:main Nov 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds implementation for Auth Server Using Goth #315

Adds implementation for Auth Server Using Goth #315

PuneetPunamiya commented Aug 30, 2021 •

edited

Loading

PuneetPunamiya Aug 30, 2021

sm43 Sep 1, 2021

sm43 Sep 1, 2021

sm43 Sep 1, 2021

sm43 Sep 1, 2021

PuneetPunamiya Sep 2, 2021

sm43 Sep 1, 2021

sm43 Sep 1, 2021

PuneetPunamiya Sep 1, 2021

vinamra28 left a comment

vinamra28 Nov 15, 2021 •

edited by PuneetPunamiya

Loading

vinamra28 Nov 15, 2021

PuneetPunamiya Nov 16, 2021

sm43 Nov 16, 2021

piyush-garg Nov 16, 2021

vinamra28 Nov 15, 2021

sm43 Nov 16, 2021

vinamra28 Nov 16, 2021

PuneetPunamiya Nov 16, 2021

PuneetPunamiya Nov 16, 2021

vinamra28 commented Nov 15, 2021

sm43 Nov 16, 2021

sm43 Nov 16, 2021

vinamra28 left a comment

vinamra28 left a comment

pratap0007 commented Nov 17, 2021

sm43 commented Nov 18, 2021

tekton-robot commented Nov 18, 2021

tekton-robot commented Nov 18, 2021


		logger.Info(fmt.Sprintf("listening on %s", AUTH_BASE_URL))

		logger.Fatal(http.ListenAndServe(":4200",

Adds implementation for Auth Server Using Goth #315

Adds implementation for Auth Server Using Goth #315

Conversation

PuneetPunamiya commented Aug 30, 2021 • edited Loading

Changes

Submitter Checklist

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinamra28 left a comment

Choose a reason for hiding this comment

vinamra28 Nov 15, 2021 • edited by PuneetPunamiya 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

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

vinamra28 commented Nov 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinamra28 left a comment

Choose a reason for hiding this comment

vinamra28 left a comment

Choose a reason for hiding this comment

pratap0007 commented Nov 17, 2021

sm43 commented Nov 18, 2021

tekton-robot commented Nov 18, 2021

tekton-robot commented Nov 18, 2021

PuneetPunamiya commented Aug 30, 2021 •

edited

Loading

vinamra28 Nov 15, 2021 •

edited by PuneetPunamiya

Loading