Add API implementation for auth service #3178

guy-har · 2022-04-07T14:13:59Z

No description provided.

nopcoder

some comments I had while going over the code - will continue sunday morning

nopcoder · 2022-04-09T14:08:02Z

pkg/auth/service.go

+}
+
+// getNonRequiredString returns empty string in case of nil
+func getNonRequiredString(s *string) string {


This one can be found in swag and api packages - we can move in a different pr all the api.StringValue functions into apiutil package

don't think we need this one - already have couple in our code and in packages we bring.

nopcoder · 2022-04-09T14:09:26Z

pkg/auth/service.go

+	return *s
+}
+
+func toPagination(paginator Pagination) *model.Paginator {


same as the above function - looks like a set of functions we require to build api client or api user

nopcoder · 2022-04-09T14:10:58Z

pkg/auth/service.go

+	return expectedStatusCode(resp, 201)
+}
+
+var ErrUnexpectedStatusCode = errors.New("unexpected status code")


nopcoder · 2022-04-09T14:14:13Z

pkg/auth/service.go

+	if err != nil {
+		return InvalidUserID, err
+	}
+	if err := expectedStatusCode(resp, 201); err != nil {


As I see it we can have one function that handles all the remote api return values (errors) and map them to the local error. Alternative will be to have a switch case per status code to handle the local error mapping.

nopcoder · 2022-04-09T14:15:14Z

pkg/auth/service.go

@@ -1,15 +1,20 @@
 package auth

+//go:generate oapi-codegen -package auth -generate "types,client,spec"  -o client.gen.go api/swagger.yml


I think it should in placed on the /api directory.

nopcoder · 2022-04-09T14:17:16Z

pkg/auth/service.go

+	cache       Cache
+}
+
+func (a *APIAuthService) SecretStore() crypt.SecretStore {


I think the only thing the service needs to expose is the shared secret. Can be handled in a different PR.

nopcoder · 2022-04-09T14:18:03Z

pkg/auth/service.go

+}
+
+func (a *APIAuthService) DeleteUser(ctx context.Context, username string) error {
+	u, err := a.GetUser(ctx, username)


why do we need to get the user first - this is not the remote implementation's work?

delete user is the only one that works with ID - the service can delete user by name

nopcoder · 2022-04-09T14:23:11Z

pkg/auth/service.go

+	if err != nil {
+		return nil, err
+	}
+	logging.Default().Info("initialized Auth service")


consider accepting logger - or initialize one here with field "service" set to remote authorization service

nopcoder · 2022-04-09T14:23:51Z

pkg/auth/service.go

+		// avoid redirect automatically
+		CheckRedirect: func(req *http.Request, via []*http.Request) error {
+			return http.ErrUseLastResponse
+		},


not sure this one is required for this client

needs a way to handle timeout or at least have a default set

nopcoder · 2022-04-09T14:25:41Z

pkg/auth/service.go

+			policies, _, err := a.listUserPolicies(ctx, username, params, true)
+			return policies, err


use a different names inside the scope so the reader will not get mixed. or just return the 3 of them and ignore the middle one outside

nopcoder

🥇 added some comments, the main one is error handling and the second one is the authorized which looks like a client-side implementation and not server side implemented. which make you think it should be extracted out of the service that provides the authorization information.

nopcoder · 2022-04-11T15:12:42Z

api/authorization.yml

+          format: byte
+      required:
+        - username
+    SetupState:


can remove this one?

nopcoder · 2022-04-11T16:32:40Z

api/authorization.yml

+        default:
+          $ref: "#/components/responses/ServerError"
+
+  /auth/userbyname:


/auth/users?name={name}

nopcoder · 2022-04-11T16:33:11Z

api/authorization.yml

+        default:
+          $ref: "#/components/responses/ServerError"
+
+  /auth/userbyemail:


/auth/users?email={email}

nopcoder · 2022-04-11T16:41:21Z

pkg/auth/service.go

+
+func NewAPIAuthService(apiEndpoint, token string, secretStore crypt.SecretStore, cacheConf params.ServiceCache) (*APIAuthService, error) {
+	// TODO(Guys): currently token is used, maybe we should consider using APIKey
+	basicAuthProvider, err := securityprovider.NewSecurityProviderBearerToken(token)


This is a bearer token - why assign it to basic auth? name

At some point we should consider reload as we do for the logging level - so it will be easy to roll and update

Done

It will be done soon in a different PR

nopcoder · 2022-04-11T16:42:12Z

pkg/auth/service.go

+	}
+	httpClient := &http.Client{
+		// avoid redirect automatically
+		CheckRedirect: func(req *http.Request, via []*http.Request) error {


Can remove this one - prefer to support the default redirect if needed.

We should enable configurable timeout

nopcoder · 2022-04-11T17:07:42Z

pkg/auth/service.go

+}
+func (a *APIAuthService) ListUsers(ctx context.Context, params *model.PaginationParams) ([]*model.User, *model.Paginator, error) {


Suggested change

}

func (a *APIAuthService) ListUsers(ctx context.Context, params *model.PaginationParams) ([]*model.User, *model.Paginator, error) {

}

func (a *APIAuthService) ListUsers(ctx context.Context, params *model.PaginationParams) ([]*model.User, *model.Paginator, error) {

nopcoder · 2022-04-11T17:11:58Z

pkg/auth/service.go

+func paginationPrefix(prefix string) *PaginationPrefix {
+	p := PaginationPrefix(prefix)
+	return &p
+}
+func paginationAfter(after string) *PaginationAfter {
+	p := PaginationAfter(after)
+	return &p
+}
+func paginationAmount(amount int) *PaginationAmount {
+	p := PaginationAmount(amount)
+	return &p
+}


Suggested change

func paginationPrefix(prefix string) *PaginationPrefix {

p := PaginationPrefix(prefix)

return &p

}

func paginationAfter(after string) *PaginationAfter {

p := PaginationAfter(after)

return &p

}

func paginationAmount(amount int) *PaginationAmount {

p := PaginationAmount(amount)

return &p

}

func paginationPrefix(prefix string) *PaginationPrefix {

p := PaginationPrefix(prefix)

return &p

}

func paginationAfter(after string) *PaginationAfter {

p := PaginationAfter(after)

return &p

}

func paginationAmount(amount int) *PaginationAmount {

p := PaginationAmount(amount)

return &p

}

nopcoder · 2022-04-11T17:56:06Z

pkg/auth/service.go

+}
+
+func (a *APIAuthService) AddCredentials(ctx context.Context, username, accessKeyID, secretAccessKey string) (*model.Credential, error) {
+	// TODO(Guys): decide if we want to support this - We do! for ldap, we could not support in globalIAM


support/unsupport should be on the server side - return an unsupported error, but implement this one.

pkg/auth/service.go

nopcoder

lgtm! optional, service can delete user by name

nopcoder · 2022-04-12T14:30:08Z

pkg/auth/service.go

+}
+
+func (a *APIAuthService) DeleteUser(ctx context.Context, username string) error {
+	u, err := a.GetUser(ctx, username)


delete user is the only one that works with ID - the service can delete user by name

guy-har added 3 commits April 7, 2022 07:49

Add API implementation for auth service

b4ff811

Add Generate and Delete to swagger client on Makefile

67de6b9

Fix lint

abe0e2c

guy-har requested a review from nopcoder April 7, 2022 15:19

Remove unused

38f0d6e

guy-har force-pushed the feature/auth-service-api branch from ce7e6b4 to 38f0d6e Compare April 7, 2022 16:05

nopcoder reviewed Apr 9, 2022

View reviewed changes

guy-har added 4 commits April 10, 2022 07:38

Remove Setup enforcement for api authorization and bug fixes

3c632a5

Move swagger file and handle API response errors

a46ef8d

Chage UserID to type int64

4bc539f

Change variable name

b7cf4cb

guy-har requested a review from nopcoder April 11, 2022 15:11

nopcoder suggested changes Apr 11, 2022

View reviewed changes

Code review changes

f295990

guy-har requested a review from nopcoder April 12, 2022 10:07

Remove duplicate function

096a8e3

guy-har marked this pull request as ready for review April 12, 2022 10:31

Bugfix

d990a9f

nopcoder approved these changes Apr 12, 2022

View reviewed changes

guy-har added 2 commits April 12, 2022 19:27

Get/Delete user by username

ec7b0ba

Merge branch 'master' into feature/auth-service-api

f0250e1

guy-har requested a review from nopcoder April 12, 2022 17:22

nopcoder approved these changes Apr 13, 2022

View reviewed changes

Change password to required in swagger

b6658ee

guy-har added the include-changelog PR description should be included in next release changelog label Apr 13, 2022

Move error to errors file

94ea7e0

guy-har merged commit c1c6c0e into master Apr 13, 2022

guy-har deleted the feature/auth-service-api branch April 13, 2022 09:08

guy-har linked an issue Apr 13, 2022 that may be closed by this pull request

Add support for API Auth service #3168

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add API implementation for auth service #3178

Add API implementation for auth service #3178

guy-har commented Apr 7, 2022

nopcoder left a comment

nopcoder Apr 9, 2022

nopcoder Apr 11, 2022

nopcoder Apr 9, 2022

nopcoder Apr 9, 2022

nopcoder Apr 9, 2022

nopcoder Apr 9, 2022

nopcoder Apr 9, 2022

nopcoder Apr 9, 2022

nopcoder Apr 12, 2022

nopcoder Apr 9, 2022

nopcoder Apr 9, 2022

nopcoder Apr 9, 2022

nopcoder left a comment

nopcoder Apr 11, 2022

nopcoder Apr 11, 2022

nopcoder Apr 11, 2022

nopcoder Apr 11, 2022

guy-har Apr 12, 2022

nopcoder Apr 11, 2022

guy-har Apr 12, 2022

nopcoder Apr 11, 2022

nopcoder Apr 11, 2022

nopcoder Apr 11, 2022

nopcoder left a comment

nopcoder Apr 12, 2022

		@@ -1,15 +1,20 @@
		package auth

		//go:generate oapi-codegen -package auth -generate "types,client,spec" -o client.gen.go api/swagger.yml

		policies, _, err := a.listUserPolicies(ctx, username, params, true)
		return policies, err

		}
		func (a APIAuthService) ListUsers(ctx context.Context, params model.PaginationParams) ([]model.User, model.Paginator, error) {

Add API implementation for auth service #3178

Add API implementation for auth service #3178

Conversation

guy-har commented Apr 7, 2022

nopcoder 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

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

nopcoder 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

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

nopcoder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment