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

Add diagnostic logs to the authenticator #147

Merged
merged 5 commits into from
Aug 16, 2023

Conversation

sahandilshan
Copy link
Contributor

@sahandilshan sahandilshan commented Aug 12, 2023

Proposed changes in this pull request

  • Add diagnostic logs to the outbound OIDC authenticator

Approach

image

Diagnostic logs will be covered the green-colored actions/validations of the authentication process.

  1. Authentication Request Validation
    Within each authenticator, the initiateAuthenticationRequest method houses the logic for this step. This triggers two diagnostic logs. The first log indicates the initialization of the authentication request, and the second log shows the authentication request has been successfully sent – whether it's a Success or marked Invalid.

  2. Validate Authentication Response
    The processAuthenticationResponse method handles authentication responses sent by the federated IDP. Once users are sent to the federated IDP page and complete the login, the authenticator receives an authentication response sent by the federated IDP, which this method manages. Just like the request validation, this step also generates two diagnostic logs. The first one marks the beginning of response validation, while the second one is only created if the authentication is successful. We've chosen not to include logs for authentication response failures for a couple of reasons:

Additionally, there will be another diagnostic log that will get published from the canHandle() method of the authenticator. The canHandle method gets executed each time before the initiateAuthenticationRequest and processAuthenticationResponse get executed. This log is published as an internal log for our internal developers. The purpose of this log is to verify whether the auth request/response was passed into the authenticator to handle it.

I've also introduced a new protected method named getComponentId. This addition serves a specific purpose. The OpenIDAuthenticator class extends into other Authenticators like GitHub and Google. These extended Authenticators utilize the initiateAuthenticationRequest and processAuthenticationResponse methods from the OpenIDAuthenticator class. However, this can lead to diagnostic logs being attributed to the OIDC authenticator even when they're handled by the relevant Authenticator (with no change in the component ID of the logs).
To address this, I've implemented the getComponentId method. This method returns the component ID. When each Authenticator (like Google or Github) overrides this method, the component ID in the logs gets adjusted accordingly. This ensures that the logs correctly reflect the handling Authenticator.

@jenkins-is-staging
Copy link

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/5841561428

@jenkins-is-staging
Copy link

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/5841561428
Status: failure

This method has been introduce since some other authenticators extends the OpenIDConnectAuthenticator class and use its methods. In that case we need to change the component id of those repos. That's why this method was introduced
@jenkins-is-staging
Copy link

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/5866716321

@jenkins-is-staging
Copy link

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/5866716321
Status: success

Copy link

@jenkins-is-staging jenkins-is-staging left a comment

Choose a reason for hiding this comment

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

Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/5866716321

chamathns
chamathns previously approved these changes Aug 16, 2023
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