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.

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

Adding handlers for auth #132

Open
wants to merge 21 commits into
base: refactor
Choose a base branch
from

Conversation

shreelakshmijoshi
Copy link
Collaborator

@shreelakshmijoshi shreelakshmijoshi commented Dec 19, 2024

  • Please refer to the code of conduct : link

  • Please check if the PR fulfills these requirements 📋

  • The commit message follows our guidelines : link
  • Docs have been added/updated (for bug fixes/features)
  • A new branch is created for the change(s) and the changes are not committed on the main or master branch
  • No credentials or secrets are committed
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, refactor, chore, test, config) 💁 📜
    Refactor, Feature
  • Refactored authentication and authorization related classes
  • Added Bearer header related authentication in AuthenticationServiceImpl
  • Updated integration tests to have Authorization Bearer Header token
  • Users are inserted in database for now as the other tables have foreign keys referring to user table

  • Checklist for the current PR 📃
  • I have made corresponding changes to the documentation
  • I have commented on my code, particularly in hard-to-understand areas
  • I have added reviewers to check the proposed changes

Packaged classes according to handler, model, util for authentication and services for AuthClient, CatalogueClient related classes
Added validation class for bearer JWT token sent in Authorization header
Added Authorization header in the list of allowed header keys
Add user access handler to allow users to access APIs based on their roles
Added UserInfo class to map the POJO from the json object param to class variables
Added a handler to fetch user information DX Auth everytime an API is called to reduce the dependency over user table and to avoid reading and writing in the user table in the database to have DX Auth as single point of information and to avoid maintainability issues
Added auth handler for Verify policy API. Updated ApiServerVerticle, ConsumerApis, ProviderApis with handlers to be called according to the API
Refactored and updated AuthHandler
Added Bearer Authorization header value in the routing context if it is in the request header
Added Bearer Auth header in AuthenticationServiceImpl, refactored AuthenticationService, disabled unit tests after refactoring it to rewrite them after refactoring is completed. Refactored Auth classes to have Future returns
Added Bearer Authorization token as optional header in the open api specs
@jenkins-datakaveri
Copy link

Build finished.

Increased the failure threshold for pmd, checkstyle in Jenkinsfile
@jenkins-datakaveri
Copy link

Build finished.

Updated integration tests to have authorization bearer JWT token
@jenkins-datakaveri
Copy link

Build finished.

Copy link
Collaborator

@gopal-mahajan gopal-mahajan left a comment

Choose a reason for hiding this comment

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

Please check the comments

Copy link
Collaborator

Choose a reason for hiding this comment

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

try to validate the token in open API specs instead of having a separate class

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

DMP APD currently doesn't have validation done through Open API specs for any API

String token = authInfo.getString(TOKEN);

Promise<Void> promise = Promise.promise();
Future<JwtData> jwtDecodeFuture = decodeJwt(token);
jwtDecodeFuture
Copy link
Collaborator

Choose a reason for hiding this comment

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

try spearating the check in a different method eg:

jwtDecodeFuture
    .onSuccess(jwtData -> {
        String errorMessage = validateJwtData(jwtData, issuer, apdUrl);
        if (errorMessage != null) {
            LOGGER.error(errorMessage);
            promise.fail(errorMessage);
        } else {
            LOGGER.info("Auth token verified");
            promise.complete();
        }
    })
    .onFailure(failureHandler -> {
        String errorMsg = String.format("Failed to decode the token: %s", failureHandler.getMessage());
        LOGGER.error(errorMsg);
        promise.fail(errorMsg);
    });

return promise.future();

// Helper method to validate JWT data
private String validateJwtData(JwtData jwtData, String expectedIssuer, String expectedAudience) {
    if (jwtData.getSub() == null) {
        return "No sub value in JWT";
    }
    if (jwtData.getIss() == null || !expectedIssuer.equalsIgnoreCase(jwtData.getIss())) {
        return "Incorrect issuer value in JWT";
    }
    if (jwtData.getAud().isEmpty()) {
        return "No audience value in JWT";
    }
    if (!expectedAudience.equalsIgnoreCase(jwtData.getAud())) {
        return "Incorrect audience value in JWT";
    }
    return null;
} 

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated it here : link

String userId = jsonObject.getString(USERID);
String iudxRole = jsonObject.getString(ROLE).toLowerCase();
String resourceServer = jsonObject.getString("aud");
String userId = userInfo.getUserId().toString();
Copy link
Collaborator

Choose a reason for hiding this comment

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

try to make the String in the respective get methods of the userId

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks for reviewing the PR, updated it here : link

@jenkins-datakaveri
Copy link

Build finished.

Updated flyway postgres dependency to the latest due to no database found error. Reference : flyway/flyway#3722
@jenkins-datakaveri
Copy link

Build finished.

@jenkins-datakaveri
Copy link

Build finished.

Refactored insert into user_table query
 to update the name and email if the user_id is already present in the table. This is done according to the information from DX Auth server to maintain user info. This is updated only when the user requests for consumer, provider or delegate specific API
@jenkins-datakaveri
Copy link

Build finished.

Copy link
Collaborator

@gopal-mahajan gopal-mahajan left a comment

Choose a reason for hiding this comment

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

Please check the comments

accessHandler = new AccessHandler();
userInfo = new UserInfo();
userInfoFromAuthHandler = new UserInfoFromAuthHandler(authClient, userInfo, postgresService);
authHandler = new AuthHandler(authenticationService);
Copy link
Collaborator

Choose a reason for hiding this comment

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

userRolesForEndpoint for all the apis can be set here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated it here

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;

public class BearerTokenTypeValidator implements Validator{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we actually need this class when we are already validating the same thing in the pom file as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's no validation being done currently from openapi specs

@shreelakshmijoshi shreelakshmijoshi self-assigned this Jan 15, 2025
Refactored accessHandler variable declaration is ConsumerApis, ProviderApis, ApiServerVerticle. Updated RoutingContextHelper to refactor VerifyAuthHandler
@jenkins-datakaveri
Copy link

Build finished.

@jenkins-datakaveri
Copy link

Build finished.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants