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

Refactor IWebSocketAuthenticationService #1177

Open
Shane32 opened this issue Nov 29, 2024 · 6 comments
Open

Refactor IWebSocketAuthenticationService #1177

Shane32 opened this issue Nov 29, 2024 · 6 comments
Milestone

Comments

@Shane32
Copy link
Member

Shane32 commented Nov 29, 2024

Suggest changing method AuthenticateAsync to have an AuthenticationRequest class that includes AuthenticationSchemes, as follows:

/// <summary>
/// Authenticates an incoming GraphQL over WebSockets request with the
/// connection initialization message.  A typical implementation will
/// set the <see cref="HttpContext.User"/> property after reading the
/// authorization token.  This service must be registered as a singleton
/// in the dependency injection framework.
/// </summary>
public interface IWebSocketAuthenticationService
{
    /// <summary>
    /// Authenticates an incoming GraphQL over WebSockets request with the connection initialization message.  The implementation should
    /// set the <paramref name="authenticationRequest"/>.<see cref="AuthenticationRequest.Connection">Connection</see>.<see cref="IWebSocketConnection.HttpContext">HttpContext</see>.<see cref="HttpContext.User">User</see>
    /// property after validating the provided credentials.
    /// <br/><br/>
    /// After calling this method to authenticate the request, the infrastructure will authorize the incoming request via the
    /// <see cref="GraphQLHttpMiddlewareOptions.AuthorizationRequired"/>, <see cref="GraphQLHttpMiddlewareOptions.AuthorizedRoles"/> and
    /// <see cref="GraphQLHttpMiddlewareOptions.AuthorizedPolicy"/> properties.
    /// </summary>
    Task AuthenticateAsync(AuthenticationRequest authenticationRequest);
}

/// <summary>
/// Represents an authentication request within the GraphQL ASP.NET Core WebSocket context.
/// </summary>
public class AuthenticationRequest
{
    /// <summary>
    /// Gets the WebSocket connection associated with the authentication request.
    /// </summary>
    /// <value>
    /// An instance of <see cref="IWebSocketConnection"/> representing the active WebSocket connection.
    /// </value>
    public IWebSocketConnection Connection { get; }

    /// <summary>
    /// Gets the subprotocol used for the WebSocket communication.
    /// </summary>
    /// <value>
    /// A <see cref="string"/> specifying the subprotocol negotiated for the WebSocket connection.
    /// </value>
    public string SubProtocol { get; }

    /// <summary>
    /// Gets the operation message containing details of the authentication operation.
    /// </summary>
    /// <value>
    /// An instance of <see cref="OperationMessage"/> that encapsulates the specifics of the authentication request.
    /// </value>
    public OperationMessage OperationMessage { get; }

    /// <summary>
    /// Gets a list of the authentication schemes the authentication requirements are evaluated against.
    /// When no schemes are specified, the default authentication scheme is used.
    /// </summary>
    public IEnumerable<string> AuthenticationSchemes { get; }

    /// <summary>
    /// Initializes a new instance of the <see cref="AuthenticationRequest"/> class.
    /// </summary>
    public AuthenticationRequest(IWebSocketConnection connection, string subProtocol, OperationMessage operationMessage, IEnumerable<string> authenticationSchemes)
    {
        Connection = connection;
        SubProtocol = subProtocol;
        OperationMessage = operationMessage;
        AuthenticationSchemes = authenticationSchemes;
    }
}

This allows for the authentication handler to attempt each scheme specified by the middleware options.

@Shane32 Shane32 added this to the 9.0.0 milestone Nov 29, 2024
@Shane32
Copy link
Member Author

Shane32 commented Dec 1, 2024

@gao-artur lmk if you have any comments about this proposal. For reference, see the current interface definition below. (I'm going to implement this for my own code now, but since it's a breaking change, won't be merging it in here until v9.)

https://github.com/graphql-dotnet/server/blob/8.1.0/src/Transports.AspNetCore/WebSockets/IWebSocketAuthenticationService.cs

/// <summary>
/// Authenticates an incoming GraphQL over WebSockets request with the
/// connection initialization message.  A typical implementation will
/// set the <see cref="HttpContext.User"/> property after reading the
/// authorization token.  This service must be registered as a singleton
/// in the dependency injection framework.
/// </summary>
public interface IWebSocketAuthenticationService
{
    /// <summary>
    /// Authenticates an incoming GraphQL over WebSockets request with the connection initialization message.  The implementation should
    /// set the <paramref name="connection"/>.<see cref="IWebSocketConnection.HttpContext">HttpContext</see>.<see cref="HttpContext.User">User</see>
    /// property after validating the provided credentials.
    /// <br/><br/>
    /// After calling this method to authenticate the request, the infrastructure will authorize the incoming request via the
    /// <see cref="GraphQLHttpMiddlewareOptions.AuthorizationRequired"/>, <see cref="GraphQLHttpMiddlewareOptions.AuthorizedRoles"/> and
    /// <see cref="GraphQLHttpMiddlewareOptions.AuthorizedPolicy"/> properties.
    /// </summary>
    Task AuthenticateAsync(IWebSocketConnection connection, string subProtocol, OperationMessage operationMessage);
}

@gao-artur
Copy link
Contributor

LGTM. You can use the default interface implementation feature to forward the new overload calls to the old one. I'm not a big fan of default interfaces because they are harder to discover, but they can be used during v8 and changed to the regular interface methods in v9.

@Shane32
Copy link
Member Author

Shane32 commented Dec 2, 2024

I would except they (default interface implementations) are not supported by .NET Framework so still a breaking change for what is officially a supported framework. (And people do use it for certain needs; I have a GraphQL.NET app built on it.)

@Shane32
Copy link
Member Author

Shane32 commented Dec 2, 2024

I will port this PR over for v9 at the appropriate time:

Feel free to comment on it if you like. I don't think there's any rush, per se, but just on the v9 todo list.

@Shane32
Copy link
Member Author

Shane32 commented Dec 2, 2024

In short it will add another package for a standardized way of handling JWT bearer tokens in the gql init call, fully linking into ASP.Net Core's JWT bearer handling, including the way you can have it completely auto-configure itself by only setting the authority and audience. (It uses OIDC to pull the JWKs to verify tokens with.) I might consider adding a full sample of using MS authentication, except it needs a client ID and a bunch of setup instructions within the Azure Portal for a operable sample.

@Shane32
Copy link
Member Author

Shane32 commented Dec 2, 2024

We can of course review that additional package, and decide whether it is appropriate to release or not -- maybe we just update the sample instead.

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

No branches or pull requests

2 participants