Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

jwt implementation changes #53

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

jwt implementation changes #53

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bc62684
jwt implementation changes
Dec 10, 2024
43d52e9
fixed issues
Dec 14, 2024
ae83d01
fixed issue
Dec 15, 2024
c629a18
user fetch from redis concept
Dec 19, 2024
20c42e7
user fetch concept modify
Dec 24, 2024
2edae7e
Merge branch 'develop' into jwt-api
indraniBan Dec 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 29 additions & 11 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,10 @@
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter</artifactId>
</dependency>
<dependency>
<groupId>co.elastic.logging</groupId>
<artifactId>logback-ecs-encoder</artifactId>
<version>1.3.2</version>
<dependency>
<groupId>co.elastic.logging</groupId>
<artifactId>logback-ecs-encoder</artifactId>
<version>1.3.2</version>
</dependency>

<dependency>
Expand Down Expand Up @@ -189,17 +189,15 @@
<artifactId>spring-boot-starter-mail</artifactId>
</dependency>

<!--
https://mvnrepository.com/artifact/ca.uhn.hapi.fhir/hapi-fhir-structures-r4 -->
<!-- https://mvnrepository.com/artifact/ca.uhn.hapi.fhir/hapi-fhir-structures-r4 -->
<dependency>
<groupId>ca.uhn.hapi.fhir</groupId>
<artifactId>hapi-fhir-structures-r4</artifactId>
<version>7.0.2</version>
</dependency>


<!--
https://mvnrepository.com/artifact/ca.uhn.hapi.fhir/org.hl7.fhir.utilities -->
<!-- https://mvnrepository.com/artifact/ca.uhn.hapi.fhir/org.hl7.fhir.utilities -->
<dependency>
<groupId>ca.uhn.hapi.fhir</groupId>
<artifactId>org.hl7.fhir.utilities</artifactId>
Expand Down Expand Up @@ -235,6 +233,27 @@
<artifactId>json-path</artifactId>
<version>2.9.0</version>
</dependency>
<!-- https://mvnrepository.com/artifact/io.jsonwebtoken/jjwt-api -->
<dependency>
<groupId>io.jsonwebtoken</groupId>
<artifactId>jjwt-api</artifactId>
<version>0.12.6</version>
</dependency>
<!-- https://mvnrepository.com/artifact/io.jsonwebtoken/jjwt-impl -->
<dependency>
<groupId>io.jsonwebtoken</groupId>
<artifactId>jjwt-impl</artifactId>
<version>0.12.6</version>
<scope>runtime</scope>
</dependency>
<!-- https://mvnrepository.com/artifact/io.jsonwebtoken/jjwt-jackson -->
<dependency>
<groupId>io.jsonwebtoken</groupId>
<artifactId>jjwt-jackson</artifactId>
<version>0.12.6</version>
<scope>runtime</scope>
</dependency>


</dependencies>

Expand All @@ -250,7 +269,7 @@
<format>HTML</format>
<nvdApiServerId>nvd</nvdApiServerId>
</configuration>

</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
Expand Down Expand Up @@ -329,8 +348,7 @@
${target-properties} and
${source-properties}
</echo>
<concat destfile="${target-properties}"
append="yes"
<concat destfile="${target-properties}" append="yes"
force="yes">
<fileset file="${source-properties}">
</fileset>
Expand Down
3 changes: 2 additions & 1 deletion src/main/environment/common_ci.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,4 +95,5 @@ logging.level.com.iemr=DEBUG
logging.level.org.springframework=INFO

#ELK logging file name
[email protected]_API_LOGGING_FILE_NAME@
[email protected]_API_LOGGING_FILE_NAME@
jwt.secret=@JWT_SECRET_KEY@
1 change: 1 addition & 0 deletions src/main/environment/common_dev.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,3 +93,4 @@ logging.level.org.springframework.web=INFO
logging.level.org.hibernate=INFO
logging.level.com.iemr=DEBUG
logging.level.org.springframework=INFO
jwt.secret=@JWT_SECRET_KEY@
1 change: 1 addition & 0 deletions src/main/environment/common_example.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,3 +93,4 @@ logging.level.org.springframework.web=INFO
logging.level.org.hibernate=INFO
logging.level.com.iemr=DEBUG
logging.level.org.springframework=INFO
jwt.secret=@JWT_SECRET_KEY@
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve example property documentation and security

  1. Add an example format for the JWT secret (e.g., jwt.secret=<Enter a strong secret key with minimum 32 characters>)
  2. Consider replacing actual ABDM URLs with placeholders in the example file
  3. Add a comment explaining the JWT secret's purpose and requirements
-jwt.secret=@JWT_SECRET_KEY@
+# JWT secret key used for token signing and verification
+# Required: Minimum 32 characters, mix of letters, numbers, and special characters
+jwt.secret=<Enter your JWT secret key here>
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
jwt.secret=@JWT_SECRET_KEY@
# JWT secret key used for token signing and verification
# Required: Minimum 32 characters, mix of letters, numbers, and special characters
jwt.secret=<Enter your JWT secret key here>

1 change: 1 addition & 0 deletions src/main/environment/common_test.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,3 +93,4 @@ logging.level.org.springframework.web=INFO
logging.level.org.hibernate=INFO
logging.level.com.iemr=DEBUG
logging.level.org.springframework=INFO
jwt.secret=@JWT_SECRET_KEY@
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Review concerns about test environment JWT configuration are valid

Based on the verification results:

  1. All environments (test, dev, ci, example) use the same JWT secret placeholder @JWT_SECRET_KEY@, which is a security concern
  2. The JwtUtil class has validation for empty/null secrets but no environment-specific validation
  3. No dedicated test configuration or test-specific JWT handling was found

Recommendations:

  • Use a different placeholder for test JWT secret (e.g., @TEST_JWT_SECRET_KEY@)
  • Add environment validation in JwtUtil to prevent production secrets in test
  • Implement separate test-specific JWT configuration properties
🔗 Analysis chain

Enhance test environment security configuration

For the test environment:

  1. Ensure a test-specific JWT secret is used
  2. Consider implementing configuration validation to prevent production secrets in test
  3. Add appropriate test-specific logging for JWT operations

Let me gather more information about JWT configuration across different environments to verify the concern.

Let me check the content of these JWT-related files and properties to better understand the configuration and validation mechanisms.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for JWT configuration validation
echo "Checking for JWT configuration validation..."
rg -l "JwtConfig.*validate|validateJwtConfig" || echo "No JWT configuration validation found"

# Look for JWT-related test files
echo "Checking for JWT-related tests..."
fd -e java -e kt -e groovy --full-path ".*[Tt]est.*" | rg "JWT|[Jj]wt" || echo "No JWT-related test files found"

Length of output: 454

Script:

#!/bin/bash
# Check JWT configuration across different environments
echo "Checking JWT configuration across environments..."
rg "jwt\.secret" --type properties

# Look for JWT-related configuration classes and security setup
echo "Checking JWT-related configuration classes..."
rg -l "class.*[Jj]wt" 

# Check for any JWT token generation or validation logic
echo "Looking for JWT token handling..."
ast-grep --pattern 'class $_ {
  $$$
  $_(JWT|[Jj]wt)$_ {
    $$$
  }
  $$$
}'

Length of output: 639

Script:

#!/bin/bash
# Check JWT utility class implementations
echo "Checking JWT utility implementations..."
rg -A 10 "class JwtUtil" src/main/java/com/wipro/fhir/utils/JwtUtil.java

# Check JWT filter implementation
echo "Checking JWT filter implementation..."
rg -A 10 "class JwtUserIdValidationFilter" src/main/java/com/wipro/fhir/utils/JwtUserIdValidationFilter.java

# Check properties files for JWT configuration
echo "Checking properties files for JWT configuration..."
fd -e properties | xargs rg "jwt"

Length of output: 1637

74 changes: 74 additions & 0 deletions src/main/java/com/wipro/fhir/data/users/User.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
package com.wipro.fhir.data.users;

import java.sql.Timestamp;

import com.google.gson.annotations.Expose;

import jakarta.persistence.Column;
import jakarta.persistence.Entity;
import jakarta.persistence.GeneratedValue;
import jakarta.persistence.GenerationType;
import jakarta.persistence.Id;
import jakarta.persistence.Table;
import lombok.Data;

@Entity
@Table(name = "m_user")
@Data
public class User {
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Replace @DaTa with more specific Lombok annotations

Since this is a security-sensitive entity used for JWT authentication, using @Data might expose sensitive information through generated toString() method in logs. Consider using a combination of more specific annotations:

-@Data
+@Getter
+@Setter
+@EqualsAndHashCode(of = "userID")
+@ToString(exclude = {"userName"})
public class User {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@Data
public class User {
@Getter
@Setter
@EqualsAndHashCode(of = "userID")
@ToString(exclude = {"userName"})
public class User {

@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
@Expose
@Column(name = "UserID")
private Long userID;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Remove @expose annotation from sensitive fields

The @Expose annotation on userID and other sensitive fields might lead to unintended exposure in JSON responses.

Consider removing @Expose from sensitive fields or implementing a custom serialization strategy.


@Expose
@Column(name = "TitleID")
private Integer titleID;
@Expose
@Column(name = "FirstName")
private String firstName;
@Expose
@Column(name = "MiddleName")
private String middleName;
@Expose
@Column(name = "LastName")
private String lastName;

@Expose
@Column(name = "GenderID")
private Integer genderID;

@Expose
@Column(name = "MaritalStatusID")
private Integer maritalStatusID;

@Expose
@Column(name = "StatusID")
private Integer statusID;

@Expose
@Column(name = "DOB")
private Timestamp dOB;
@Expose
@Column(name = "DOJ")
private Timestamp dOJ;
@Expose
@Column(name = "QualificationID")
private Integer qualificationID;
@Expose
@Column(name = "userName")
private String userName;
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add validation constraints and establish relationships

The entity lacks validation constraints and proper relationships:

  1. Name fields should have length constraints
  2. Username should have uniqueness constraint
  3. ID fields suggest missing @manytoone relationships

Apply these improvements:

+    @Column(name = "userName", unique = true, length = 50)
+    @NotBlank(message = "Username is required")
     private String userName;

+    @ManyToOne
+    @JoinColumn(name = "TitleID")
+    private Title title;

+    @Size(max = 50)
+    @NotBlank(message = "First name is required")
     @Column(name = "FirstName")
     private String firstName;

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +24 to +25
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add validation and uniqueness constraints
Since this is user data that interfaces with authentication, consider adding constraints (e.g., @notblank, length limits) on userName. Also, a uniqueness constraint at the DB level can help avoid duplicate user names.

@Expose
@Column(name = "Deleted", insertable = false, updatable = true)
private Boolean deleted;
@Expose
@Column(name = "CreatedBy")
private String createdBy;
@Column(name = "CreatedDate", insertable = false, updatable = false)
private Timestamp createdDate;
@Column(name = "ModifiedBy")
private String modifiedBy;
@Column(name = "LastModDate", insertable = false, updatable = false)
private Timestamp lastModDate;
}
11 changes: 11 additions & 0 deletions src/main/java/com/wipro/fhir/repo/user/UserRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package com.wipro.fhir.repo.user;

import org.springframework.data.repository.CrudRepository;

import com.wipro.fhir.data.users.User;

public interface UserRepository extends CrudRepository<User, Long> {

User findByUserID(Long userID);
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use standard naming conventions for repository methods

Spring Data JPA uses naming conventions for query methods. The User entity may have a field named userId, but the method is named findByUserID. Ensure that the field name matches the method name in case sensitivity and camel case.

Apply this diff to correct the method name:

-    User findByUserID(Long userID);
+    User findByUserId(Long userId);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
User findByUserID(Long userID);
User findByUserId(Long userId);


}
Loading
Loading