feat: Added enhancement in authentication server #4024
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Sarthak Jain <[email protected]>
Signed-off-by: Sarthak Jain <[email protected]>
Signed-off-by: Sarthak Jain <[email protected]>
Signed-off-by: Sarthak Jain <[email protected]>
Signed-off-by: Sarthak Jain <[email protected]>
SarthakJain26
requested review from
imrajdas,
Saranya-jena,
amityt,
Jonsy13 and
gdsoumya
Saranya-jena
approved these changes
Jun 29, 2023
Jonsy13
reviewed
Jun 29, 2023
|
|
||
| func (s *ServerGrpc) GetUserById(ctx context.Context, | ||
| inputRequest *protos.GetUserByIdRequest) (*protos.GetUserByIdResponse, error) { | ||
| fmt.Println("req", inputRequest.UserID) |
Jonsy13
reviewed
Jun 29, 2023
| log.Error(err) | ||
| return nil, err | ||
| } | ||
| fmt.Println("Resp", user) |
Jonsy13
reviewed
Jun 29, 2023
Comment on lines
111
to
113
| if err != nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
Yes @Jonsy13 thanks for pointing out, I will remove this
Jonsy13
reviewed
Jun 29, 2023
| projects, err := service.GetProjectsByUserID(outputUser.ID, false) | ||
| if err != nil { | ||
| log.Error(err) | ||
| c.JSON(utils.ErrorStatusCodes[utils.ErrServerError], presenter.CreateErrorResponse(utils.ErrServerError)) |
There was a problem hiding this comment.
Should we add error for "project not found" just like we have for "user not found" in line 37?
There was a problem hiding this comment.
@Jonsy13 the error here will not necessarily be project not found, it can be also some other error coming from the mongo side.
Jonsy13
reviewed
Jun 29, 2023
| } | ||
| authUsers, err := service.FindUsersByUID(uids) | ||
| if err != nil || authUsers == nil { | ||
| return |
There was a problem hiding this comment.
Shouldn't we return some error here?
There was a problem hiding this comment.
Sure thanks for pointing out
Jonsy13
reviewed
Jun 29, 2023
Comment on lines
308
to
329
| // Extracting user role | ||
| //role := c.MustGet("role").(string) | ||
| // | ||
| //var conn *grpc.ClientConn | ||
| //client, conn := utils.GetProjectGRPCSvcClient(conn) | ||
| //err = utils.ProjectInitializer(c, client, pID, role) | ||
| // | ||
| //defer func(conn *grpc.ClientConn) { | ||
| // err := conn.Close() | ||
| // if err != nil { | ||
| // log.Errorf("could not close gRPC client connection: %v", err) | ||
| // c.JSON(500, "could not close gRPC client connection") | ||
| // return | ||
| // } | ||
| //}(conn) | ||
| // | ||
| //if err != nil { | ||
| // log.Errorf("could not initialize project %v: %v", userRequest.ProjectName, err) | ||
| // c.JSON(500, "could not initialize project") | ||
| // return | ||
| //} | ||
|
|
There was a problem hiding this comment.
This is not required as of now
Jonsy13
reviewed
Jun 29, 2023
| return | ||
| } | ||
| c.JSON(200, gin.H{ | ||
| "message": "password has been reset", |
There was a problem hiding this comment.
Suggested change
| "message": "password has been reset", | |
| "message": "password has been updated successfully", |
We have different handler for reset below this function
Jonsy13
reviewed
Jun 29, 2023
Comment on lines
98
to
102
| for configName, configValue := range configs { | ||
| if configValue == "" { | ||
| log.Fatalf("Config %s has not been set", configName) | ||
| } | ||
| } |
There was a problem hiding this comment.
This shouldn't be required if we already have all these fields as required in envconfig in line 29. envConfig itself will give error if they are not provided being required.
There was a problem hiding this comment.
Yes good catch! looks like duplicate check.
Jonsy13
reviewed
Jun 29, 2023
| if err == utils.ErrUserExists { | ||
| log.Println("Admin already exists in the database, not creating a new admin") | ||
| } | ||
| if err != nil && err != utils.ErrUserExists { |
There was a problem hiding this comment.
Suggested change
| if err != nil && err != utils.ErrUserExists { | |
| else if err != nil { |
We already check equality in line 126. So again checking inequality is not required, it won't be equal for sure.
Jonsy13
reviewed
Jun 29, 2023
| log.Infof("Listening and serving HTTP on %s", utils.Port) | ||
| err := app.Run(utils.Port) | ||
| if err != nil { | ||
| log.Fatalf("Failure to start litmus-portal authentication server due to %s", err) |
There was a problem hiding this comment.
Suggested change
| log.Fatalf("Failure to start litmus-portal authentication server due to %s", err) | |
| log.Fatalf("Failure to start litmus-portal authentication REST server due to %s", err) |
Jonsy13
reviewed
Jun 29, 2023
| // Starting gRPC server | ||
| lis, err := net.Listen("tcp", utils.GrpcPort) | ||
| if err != nil { | ||
| log.Fatalf("Failure to start litmus-portal authentication server due"+ |
There was a problem hiding this comment.
Suggested change
| log.Fatalf("Failure to start litmus-portal authentication server due"+ | |
| log.Fatalf("Failure to start litmus-portal authentication GRPC server due"+ |
Jonsy13
reviewed
Jun 29, 2023
| log.Infof("Listening and serving gRPC on %s", utils.GrpcPort) | ||
| err = grpcServer.Serve(lis) | ||
| if err != nil { | ||
| log.Fatalf("Failure to start litmus-portal authentication server due"+ |
There was a problem hiding this comment.
Suggested change
| log.Fatalf("Failure to start litmus-portal authentication server due"+ | |
| log.Fatalf("Failure to start litmus-portal authentication GRPC server due"+ |
Jonsy13
reviewed
Jun 29, 2023
| } | ||
| } | ||
|
|
||
| log.Info(collectionName + "'s mongo collection created") |
There was a problem hiding this comment.
I saw in some places we used fmt.print as well. if we have log, then it's better to use the same
Jonsy13
reviewed
Jun 29, 2023
Comment on lines
+8
to
+34
| var ( | ||
| JwtSecret = os.Getenv("JWT_SECRET") | ||
| AdminName = os.Getenv("ADMIN_USERNAME") | ||
| AdminPassword = os.Getenv("ADMIN_PASSWORD") | ||
| DBUrl = os.Getenv("DB_SERVER") | ||
| DBUser = os.Getenv("DB_USER") | ||
| DBPassword = os.Getenv("DB_PASSWORD") | ||
| JWTExpiryDuration = getEnvAsInt("JWT_EXPIRY_MINS", 1440) | ||
| OAuthJWTExpDuration = getEnvAsInt("OAUTH_JWT_EXP_MINS", 5) | ||
| OAuthJwtSecret = os.Getenv("OAUTH_SECRET") | ||
| StrictPasswordPolicy = getEnvAsBool("STRICT_PASSWORD_POLICY", false) | ||
| DexEnabled = getEnvAsBool("DEX_ENABLED", false) | ||
| DexCallBackURL = os.Getenv("DEX_OAUTH_CALLBACK_URL") | ||
| DexClientID = os.Getenv("DEX_OAUTH_CLIENT_ID") | ||
| DexClientSecret = os.Getenv("DEX_OAUTH_CLIENT_SECRET") | ||
| DexOIDCIssuer = os.Getenv("OIDC_ISSUER") | ||
| DBName = "auth" | ||
| Port = ":3000" | ||
| GrpcPort = ":3030" | ||
| UserCollection = "users" | ||
| ProjectCollection = "project" | ||
| UsernameField = "username" | ||
| PasswordEncryptionCost = 15 | ||
| DefaultLitmusGqlGrpcEndpoint = "localhost" | ||
| DefaultLitmusGqlGrpcPort = ":8000" | ||
| ) | ||
|
|
There was a problem hiding this comment.
we can use envConfig for values we are taking as envs
There was a problem hiding this comment.
We can take it later as discussed.
Signed-off-by: Sarthak Jain <[email protected]>
…6/litmus into 3.0.0/auth-refactor
Jonsy13
approved these changes
Jun 29, 2023
SohamRatnaparkhi
pushed a commit
to SohamRatnaparkhi/litmus
that referenced
this pull request
Jun 30, 2023
* base commit to add dockerfile Signed-off-by: Sarthak Jain <[email protected]> * Added utility functions Signed-off-by: Sarthak Jain <[email protected]> * Added schemas Signed-off-by: Sarthak Jain <[email protected]> * Added presenters Signed-off-by: Sarthak Jain <[email protected]> * Added pkg, handlers and roles Signed-off-by: Sarthak Jain <[email protected]> * Code optimisation Signed-off-by: Sarthak Jain <[email protected]> --------- Signed-off-by: Sarthak Jain <[email protected]> Co-authored-by: Saranya Jena <[email protected]>
SohamRatnaparkhi
pushed a commit
to SohamRatnaparkhi/litmus
that referenced
this pull request
Jun 30, 2023
* base commit to add dockerfile Signed-off-by: Sarthak Jain <[email protected]> * Added utility functions Signed-off-by: Sarthak Jain <[email protected]> * Added schemas Signed-off-by: Sarthak Jain <[email protected]> * Added presenters Signed-off-by: Sarthak Jain <[email protected]> * Added pkg, handlers and roles Signed-off-by: Sarthak Jain <[email protected]> * Code optimisation Signed-off-by: Sarthak Jain <[email protected]> --------- Signed-off-by: Sarthak Jain <[email protected]> Co-authored-by: Saranya Jena <[email protected]> Signed-off-by: SohamRatnaparkhi <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes
Types of changes
What types of changes does your code introduce to Litmus? Put an
xin the boxes that applyChecklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Dependency
Special notes for your reviewer: