[azwebpubsub] Fix GenerateClientAccessURL on the TokenCredential path

sdk/messaging/azwebpubsub/CHANGELOG.md

@@ -7,6 +7,10 @@
 ### Breaking Changes
 
 ### Bugs Fixed
+* Fixed `GenerateClientAccessURL` panicking when `options` is `nil` and the client was created with `NewClient`.
+* Fixed `GenerateClientAccessURL` failing with `MinutesToExpire must be greater than 0` when `ExpirationTimeInMinutes` was not set and the client was created with `NewClient`. It now defaults to 60 minutes, matching `NewClientFromConnectionString`.
+* Fixed `GenerateClientAccessURL` producing a malformed audience and client URL (for example `wss://<host>client/hubs/<hub>`) when the endpoint passed to `NewClient` has no trailing slash.
+* `GenerateClientAccessURL` now validates a negative `ExpirationTimeInMinutes` on both credential types.
 
 ### Other Changes
 * Regenerated code with the latest emitter.

sdk/messaging/azwebpubsub/client_custom.go

@@ -109,10 +109,23 @@ type GenerateClientAccessURLResponse struct {
 //   - hub - The hub name.
 //   - options - GenerateClientAccessUrlOptions contains the optional parameters for the Client.GenerateClientAccessURL method.
 func (c *Client) GenerateClientAccessURL(ctx context.Context, hub string, options *GenerateClientAccessURLOptions) (*GenerateClientAccessURLResponse, error) {
-	endpoint := c.endpoint
 	if hub == "" {
 		return nil, errors.New("empty hub name is not allowed")
 	}
+	if options == nil {
+		options = &GenerateClientAccessURLOptions{}
+	}
+	if options.ExpirationTimeInMinutes < 0 {
+		return nil, errors.New("the value of ExpirationTimeInMinutes is out of range")
+	}
+
+	// The audience and the client URL are formed by appending to the endpoint, so it must
+	// end with a slash. NewClientFromConnectionString normalizes this, but an endpoint
+	// passed to NewClient may not have a trailing slash.
+	endpoint := c.endpoint
+	if !strings.HasSuffix(endpoint, "/") {
+		endpoint += "/"
+	}
 	hubPath := url.PathEscape(hub)
 	parsedURL, err := url.Parse(endpoint)
 	if err != nil {
@@ -131,14 +144,22 @@ func (c *Client) GenerateClientAccessURL(ctx context.Context, hub string, option
 			return nil, err
 		}
 	} else {
-		var userId *string
-		if options.UserID == "" {
-			userId = nil
-		} else {
-			userId = &options.UserID
+		var userID *string
+		if options.UserID != "" {
+			userID = &options.UserID
+		}
+		// The service rejects minutesToExpire=0, so fall back to the same default the
+		// key-based path uses instead of forwarding the zero value.
+		minutesToExpire := int32(defaultExpirationTime / time.Minute)
+		if options.ExpirationTimeInMinutes > 0 {
+			minutesToExpire = options.ExpirationTimeInMinutes
 		}
-		// Replace with your logic to generate the token using a webPubSub method
-		resp, err := c.generateClientToken(ctx, hub, &GenerateClientTokenOptions{UserID: userId, Role: options.Roles, Group: options.Groups, MinutesToExpire: &options.ExpirationTimeInMinutes})
+		resp, err := c.generateClientToken(ctx, hub, &GenerateClientTokenOptions{
+			UserID:          userID,
+			Role:            options.Roles,
+			Group:           options.Groups,
+			MinutesToExpire: &minutesToExpire,
+		})
 		if err != nil {
 			return nil, err
 		}

sdk/messaging/azwebpubsub/client_custom_test.go

@@ -0,0 +1,186 @@
+//go:build go1.18
+// +build go1.18
+
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License. See License.txt in the project root for license information.
+
+package azwebpubsub_test
+
+import (
+	"context"
+	"io"
+	"net/http"
+	"net/url"
+	"strings"
+	"testing"
+
+	"github.com/Azure/azure-sdk-for-go/sdk/azcore"
+	"github.com/Azure/azure-sdk-for-go/sdk/azcore/policy"
+	"github.com/Azure/azure-sdk-for-go/sdk/messaging/azwebpubsub"
+	"github.com/golang-jwt/jwt/v5"
+	"github.com/stretchr/testify/require"
+)
+
+type stubCredential struct{}
+
+func (stubCredential) GetToken(context.Context, policy.TokenRequestOptions) (azcore.AccessToken, error) {
+	return azcore.AccessToken{Token: "stub-aad-token"}, nil
+}
+
+// stubTransport records the last request and returns a canned generateToken response.
+type stubTransport struct{ lastRequest *http.Request }
+
+func (s *stubTransport) Do(req *http.Request) (*http.Response, error) {
+	s.lastRequest = req
+	return &http.Response{
+		StatusCode: http.StatusOK,
+		Header:     http.Header{"Content-Type": []string{"application/json"}},
+		Body:       io.NopCloser(strings.NewReader(`{"token":"server-issued-token"}`)),
+		Request:    req,
+	}, nil
+}
+
+func newTokenCredentialClient(t *testing.T, endpoint string) (*azwebpubsub.Client, *stubTransport) {
+	transport := &stubTransport{}
+	options := &azwebpubsub.ClientOptions{}
+	options.Transport = transport
+
+	client, err := azwebpubsub.NewClient(endpoint, stubCredential{}, options)
+	require.NoError(t, err)
+	return client, transport
+}
+
+// The endpoint passed to NewClient is not required to have a trailing slash, and the
+// audience and client URL must be well formed either way.
+func TestClient_GenerateClientAccessURLEndpointTrailingSlash(t *testing.T) {
+	hub := "chat"
+
+	for _, endpoint := range []string{
+		"https://host.webpubsub.azure.com",
+		"https://host.webpubsub.azure.com/",
+	} {
+		t.Run(endpoint, func(t *testing.T) {
+			client, _ := newTokenCredentialClient(t, endpoint)
+
+			resp, err := client.GenerateClientAccessURL(context.Background(), hub, nil)
+			require.NoError(t, err)
+
+			require.Equal(t, "wss://host.webpubsub.azure.com/client/hubs/chat", resp.BaseURL)
+			require.Equal(t, "wss://host.webpubsub.azure.com/client/hubs/chat?access_token=server-issued-token", resp.URL)
+		})
+	}
+}
+
+// The same normalization applies to the audience embedded in a key-signed token.
+func TestClient_GenerateClientAccessURLAudienceTrailingSlash(t *testing.T) {
+	client, err := azwebpubsub.NewClientFromConnectionString("Endpoint=https://host.webpubsub.azure.com;AccessKey=ABC;", nil)
+	require.NoError(t, err)
+
+	resp, err := client.GenerateClientAccessURL(context.Background(), "chat", nil)
+	require.NoError(t, err)
+
+	claims := jwt.MapClaims{}
+	_, _, err = jwt.NewParser().ParseUnverified(resp.Token, claims)
+	require.NoError(t, err)
+
+	// The service validates the host and path of the audience, so a missing separator
+	// between the endpoint and "client/hubs" makes the token unusable.
+	require.Equal(t, "https://host.webpubsub.azure.com/client/hubs/chat", claims["aud"])
+	require.Equal(t, "wss://host.webpubsub.azure.com/client/hubs/chat", resp.BaseURL)
+}
+
+// nil options must be treated the same as empty options on both credential paths.
+func TestClient_GenerateClientAccessURLNilOptions(t *testing.T) {
+	t.Run("TokenCredential", func(t *testing.T) {
+		client, transport := newTokenCredentialClient(t, "https://host.webpubsub.azure.com/")
+
+		require.NotPanics(t, func() {
+			resp, err := client.GenerateClientAccessURL(context.Background(), "chat", nil)
+			require.NoError(t, err)
+			require.Equal(t, "server-issued-token", resp.Token)
+		})
+
+		require.NotNil(t, transport.lastRequest)
+	})
+
+	t.Run("ConnectionString", func(t *testing.T) {
+		client, err := azwebpubsub.NewClientFromConnectionString("Endpoint=https://host.webpubsub.azure.com;AccessKey=ABC;", nil)
+		require.NoError(t, err)
+
+		require.NotPanics(t, func() {
+			resp, err := client.GenerateClientAccessURL(context.Background(), "chat", nil)
+			require.NoError(t, err)
+			require.NotEmpty(t, resp.Token)
+		})
+	})
+}
+
+// The service rejects minutesToExpire=0, so an unset expiration must fall back to the
+// default rather than being forwarded as a zero value.
+func TestClient_GenerateClientAccessURLMinutesToExpire(t *testing.T) {
+	minutesToExpire := func(t *testing.T, transport *stubTransport) string {
+		t.Helper()
+		require.NotNil(t, transport.lastRequest)
+		query, err := url.ParseQuery(transport.lastRequest.URL.RawQuery)
+		require.NoError(t, err)
+		return query.Get("minutesToExpire")
+	}
+
+	t.Run("unset uses the default", func(t *testing.T) {
+		client, transport := newTokenCredentialClient(t, "https://host.webpubsub.azure.com/")
+
+		_, err := client.GenerateClientAccessURL(context.Background(), "chat", &azwebpubsub.GenerateClientAccessURLOptions{})
+		require.NoError(t, err)
+		require.Equal(t, "60", minutesToExpire(t, transport))
+	})
+
+	t.Run("nil options uses the default", func(t *testing.T) {
+		client, transport := newTokenCredentialClient(t, "https://host.webpubsub.azure.com/")
+
+		_, err := client.GenerateClientAccessURL(context.Background(), "chat", nil)
+		require.NoError(t, err)
+		require.Equal(t, "60", minutesToExpire(t, transport))
+	})
+
+	t.Run("explicit value is honored", func(t *testing.T) {
+		client, transport := newTokenCredentialClient(t, "https://host.webpubsub.azure.com/")
+
+		_, err := client.GenerateClientAccessURL(context.Background(), "chat", &azwebpubsub.GenerateClientAccessURLOptions{
+			ExpirationTimeInMinutes: 5,
+		})
+		require.NoError(t, err)
+		require.Equal(t, "5", minutesToExpire(t, transport))
+	})
+}
+
+// A negative expiration is rejected on both credential paths.
+func TestClient_GenerateClientAccessURLNegativeExpiration(t *testing.T) {
+	options := &azwebpubsub.GenerateClientAccessURLOptions{ExpirationTimeInMinutes: -1}
+
+	client, _ := newTokenCredentialClient(t, "https://host.webpubsub.azure.com/")
+	_, err := client.GenerateClientAccessURL(context.Background(), "chat", options)
+	require.ErrorContains(t, err, "out of range")
+
+	keyClient, err := azwebpubsub.NewClientFromConnectionString("Endpoint=https://host.webpubsub.azure.com;AccessKey=ABC;", nil)
+	require.NoError(t, err)
+	_, err = keyClient.GenerateClientAccessURL(context.Background(), "chat", options)
+	require.ErrorContains(t, err, "out of range")
+}
+
+// Options are forwarded to the service on the TokenCredential path.
+func TestClient_GenerateClientAccessURLForwardsOptions(t *testing.T) {
+	client, transport := newTokenCredentialClient(t, "https://host.webpubsub.azure.com/")
+
+	_, err := client.GenerateClientAccessURL(context.Background(), "chat", &azwebpubsub.GenerateClientAccessURLOptions{
+		UserID: "user1",
+		Roles:  []string{"admin"},
+		Groups: []string{"group1"},
+	})
+	require.NoError(t, err)
+
+	query, err := url.ParseQuery(transport.lastRequest.URL.RawQuery)
+	require.NoError(t, err)
+	require.Equal(t, "user1", query.Get("userId"))
+	require.Equal(t, []string{"admin"}, query["role"])
+	require.Equal(t, []string{"group1"}, query["group"])
+}