Fix/highway middleware

[prnk28]

=== auto-pr-body ===

Summary
This Pull Request updates client query functions to use the newly added domainproxy, identityproxy, and serviceproxy packages. It adds functions to check alias availability, get a controller account with an address, get a DID document given an alias, and get a service record given an origin. Suggested refactorings are included.

[sonarcloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

[senior-dev-bot]

Feedback from Senior Dev Bot

[senior-dev-bot]

LGTM!

The code changes look good. The new functions CheckAliasAvailable and CheckAliasUnavailable make use of the existing GetEmailRecordCreator function to check if an alias is available or unavailable. The code follows the best practices of using context and handling errors properly.

[senior-dev-bot]

The code changes look good overall. Here are a few suggestions to improve the code:

1. Import Grouping: Group import statements together for better readability. It is a best practice to have two separate groups for standard library packages and third-party packages.

import (
	"context"
	"fmt"
	
	"google.golang.org/grpc"

	"github.com/spf13/viper"

	identitytypes "github.com/sonrhq/core/x/identity/types"
)

2. Error Handling: The code should check for errors when dialing the gRPC server and closing the gRPC connection. It's a good practice to handle errors explicitly.

...
// Create a connection to the gRPC server.
grpcConn, err := grpc.Dial(
	addr,
	grpc.WithInsecure(), // The Cosmos SDK doesn't support any transport security mechanism.
)
if err != nil {
	return nil, fmt.Errorf("unable to connect to gRPC server: %w", err)
}
defer func() {
	if closeErr := grpcConn.Close(); closeErr != nil {
		// Handle error while closing the gRPC connection, if required.
		fmt.Printf("error closing gRPC connection: %v\n", closeErr)
	}
}()
...

3. Configuration: The code is using viper to retrieve the gRPC server address and port. Make sure that the necessary configuration values (node.grpc.host and node.grpc.port) are set correctly before invoking these functions.

Overall, the code changes are good and follow best practices. However, it's always a good idea to add proper error handling and check the configuration values before using them.

[senior-dev-bot]

The code changes look good. They add import statements for the domainproxy, identityproxy, and serviceproxy packages. As long as these packages are being used correctly elsewhere in the code, these import statements are necessary for the code to compile and run correctly.

[senior-dev-bot]

The code changes look good overall. However, there are a couple of improvements that can be made:

1. Error handling: When handling errors, it is best to follow the principle of "fail early, fail loud". In the original code, the error is simply logged and a generic 500 error response is returned. It would be more helpful to provide specific error messages based on the type of error encountered. For example, if the error is due to a service not being available, you can return a more specific error message indicating that the service is unavailable. This would help in troubleshooting and debugging.

2. Context propagation: The code change introduces passing the request context to the GetServiceRecord function. This is a good practice as it allows for proper context propagation and cancellation. However, it is important to ensure that the context passed is appropriately canceled when the request is aborted. This can be achieved by using methods like WithCancel or WithTimeout on the context.

[senior-dev-bot]

The code changes look good overall. It seems that the GetServiceRecord function has been moved to a new package called serviceproxy and is now being called with an additional context parameter. This change ensures that the function can also work with a context, which is a good practice.

However, I have a few suggestions for improvement:

1. Error handling: The code currently returns a JSON response with the error message when GetServiceRecord fails. It would be better to use appropriate HTTP status codes for different error scenarios (e.g., 500 for server errors, 404 for not found, etc.) and provide a consistent error response format. You may consider using a centralized error handling mechanism or middleware.
2. Documentation: Add comments or documentation for functions and important code blocks to improve code readability and maintainability.
3. Input validation: It would be beneficial to validate the attestation and challenge query parameters before using them to avoid potential security vulnerabilities or issues. Consider adding input validation checks for proper data types and bounds.

Here's an updated version incorporating these suggestions:

attestation := c.Query("attestation")
challenge := c.Query("challenge")

// Validate input parameters
if attestation == "" || challenge == "" {
    c.JSON(400, gin.H{"error": "Invalid request"})
    return
}

// Get the service record from the origin
record, err := serviceproxy.GetServiceRecord(c.Request.Context(), origin)
if err != nil {
    c.JSON(500, gin.H{"error": "Failed to retrieve service record"})
    return
}

These updates improve error handling and input validation while maintaining the overall functionality of the code.

[senior-dev-bot]

Overall, the code changes seem to be fine. They include importing relevant packages and adding context to the imports. However, there are a few suggestions that can be made to improve the code:

1. Order the import statements: It is best practice to order the import statements in a consistent manner. This can be done alphabetically or by grouping similar imports together.

2. Remove unused import: There is an empty import statement for github.com/sonrhq/core/pkg/crypto that can be removed.

3. Use shorter import names: The imported packages github.com/sonrhq/core/x/domain/* and github.com/sonrhq/core/x/identity/* are imported using longer names (domainproxy, domaintypes, identityproxy, identitytypes). It would be better to use shorter names that are easier to read and type.

Here's the updated code with the suggested changes:

package middleware

import (
	"context"
	"fmt"

	"github.com/gin-gonic/gin"
	"github.com/go-webauthn/webauthn/protocol"

	"github.com/sonrhq/core/internal/highway/types"
	"github.com/sonrhq/core/x/domain/client/proxy"
	"github.com/sonrhq/core/x/domain/types"
	"github.com/sonrhq/core/x/identity/client/proxy"
	"github.com/sonrhq/core/x/identity/types"
	servicetypes "github.com/sonrhq/core/x/service/types"
)

// code... (existing code)

Let me know if I can help you with anything else.

[senior-dev-bot]

The changes in this code appear to introduce the use of a context and replace the function calls GetEmailRecordCreator and GetControllerAccount with functions from different packages (GetEmailRecordCreator with domainproxy.GetEmailRecordCreator and GetControllerAccount with identityproxy.GetControllerAccount). However, the context is not being used in the new function calls, which can lead to potential issues.

Here are a few suggestions to improve the code:

1. Make sure to pass the context to the new function calls domainproxy.GetEmailRecordCreator and identityproxy.GetControllerAccount.
2. Consider using more descriptive variable names for the context and the new function calls, to improve code readability.
3. Add error handling for the new function calls, as they may also return errors that need to be handled properly.

Updated code:

func IssueCredentialAssertionOptions(email string, record *servicetypes.ServiceRecord) (string, protocol.URLEncodedBase64, string, error) {
    ctx := context.Background()
    emailRecordCreator, err := domainproxy.GetEmailRecordCreator(ctx, email)
    if err != nil {
        return "", nil, "", fmt.Errorf("failed to get email record creator: %w", err)
    }
    controllerAcc, err := identityproxy.GetControllerAccount(ctx, emailRecordCreator)
    if err != nil {
        return "", nil, "", fmt.Errorf("failed to get controller account: %w", err)
    }
    // ... rest of the code
}

The updated code now correctly passes the ctx context to the new function calls and uses more meaningful variable names (emailRecordCreator) to improve code clarity. Additionally, error handling is included for the new function calls.

[senior-dev-bot]

The code changes look good. Here are a few suggestions to further improve the code:

1. It's a good practice to create a context with a timeout or deadline instead of using context.Background() directly. This can prevent potential resource leaks and help with controlling the execution time of operations.

ctx, cancel := context.WithTimeout(context.Background(), time.Second*5) // Add timeout of 5 seconds
defer cancel() // Cancel the context to release resources

2. Ensure that the context is passed down the call stack to all the relevant functions that might need it. This ensures that the context's timeout or deadline is honored throughout the process.

Here's the modified code incorporating the changes:

func UseControllerAccount(token string) (*controller.SonrController, error) {
    ctx, cancel := context.WithTimeout(context.Background(), time.Second*5) // Add timeout of 5 seconds
    defer cancel() // Cancel the context to release resources

    claims, err := types.VerifySessionJWTClaims(token)
    if err != nil {
        return nil, fmt.Errorf("failed to verify claims: %w", err)
    }
    acc, err := identityproxy.GetControllerAccount(ctx, claims.Address)
    if err != nil {
        return nil, fmt.Errorf("failed to get controller account: %w", err)
    }
    
    // rest of the code...
}

3. If the GetControllerAccount function can also be modified to accept a context, it would be ideal to pass the existing ctx to it instead of context.Background(). This allows for better control and cancellation of the entire operation.

Overall, the code changes are good, but incorporating the suggestions above can further enhance the code.

[senior-dev-bot]

The code changes look good overall. However, the CreateOrganizationRecord function is incomplete and does not have any logic. It currently returns nil without performing any operations. Please ensure that the function performs the necessary actions to create and publish an organization record to the blockchain.

Also, it would be a good practice to handle the error returned by the CreateOrganizationRecord function and provide meaningful error messages to the caller instead of returning nil.

[senior-dev-bot]

The code changes look good overall. However, there are a few suggestions for improvement:

1. Add a description: It's good practice to add a brief description to the workflow file to explain what it does and provide context. This can be done by adding a description field under the name field.

2. Specify the timezone: The cron expression is using UTC time, but it's important to specify the timezone explicitly to avoid confusion. You can add a timezone field under the schedule field and specify the timezone, for example: timezone: America/Los_Angeles.

3. Use double quotes: The run command is using single quotes for the yesterday variable and inside the gh release list command. It's generally recommended to use double quotes unless there is a specific reason to use single quotes. Double quotes allow for variable expansion and better readability. You can update the code to use double quotes instead.

4. Handle date formatting: The date command used to calculate yesterday's date might not work on all systems. It would be better to use a more portable and reliable approach. You can use the date command with the -v option, which is available on macOS and some Linux distributions. Alternatively, you can use a programming language like Python or Node.js to calculate the date.

5. Error handling: Currently, there is no error handling in the code. If there are any errors while executing the gh command, it will fail silently. It would be beneficial to add error handling and logging to ensure that any issues are identified and addressed.

Here's an updated version of the code that incorporates these improvements:

name: Publish Releases
description: Automatically publishes draft releases

on:
  schedule:
    - cron: "0 17 * * *"
  timezone: America/Los_Angeles

jobs:
  publish-drafts:
    runs-on: ubuntu-latest

    steps:
      - name: Publish Drafts
        run: |
          yesterday=$(date -u --date='-1 day' +"%Y-%m-%d")
          drafts=$(gh release list --json createdAt,draft -q "map(select(.draft==true and .createdAt >= \"$yesterday\")) | .[].name")
          for draft in $drafts; do
            gh release update $draft --draft false || echo "Failed to update release: $draft"
          done

Note: The exact method for handling dates may vary depending on the system and tools available. Please adjust the date calculation based on the platform you are using.