diff --git a/sdk/messaging/azwebpubsub/CHANGELOG.md b/sdk/messaging/azwebpubsub/CHANGELOG.md index 5767c67470e3..1a18470c38d6 100644 --- a/sdk/messaging/azwebpubsub/CHANGELOG.md +++ b/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://client/hubs/`) 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. diff --git a/sdk/messaging/azwebpubsub/client_custom.go b/sdk/messaging/azwebpubsub/client_custom.go index b38a846954ff..86a14525b5c8 100644 --- a/sdk/messaging/azwebpubsub/client_custom.go +++ b/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 } diff --git a/sdk/messaging/azwebpubsub/client_custom_test.go b/sdk/messaging/azwebpubsub/client_custom_test.go new file mode 100644 index 000000000000..38be6581873f --- /dev/null +++ b/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"]) +}