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

[Bug]: Authentication problem with 6.6.2 #2939

Closed
kenwonders opened this issue Jun 7, 2024 · 7 comments
Closed

[Bug]: Authentication problem with 6.6.2 #2939

kenwonders opened this issue Jun 7, 2024 · 7 comments
Assignees
Labels
bug external An issue in an external dependency upstream-swagger-ui
Milestone

Comments

@kenwonders
Copy link

Describe the bug

Upgrading from 6.50 to 6.6.2 caused our authentication system to stop working. We have a separate identity server using client credentials and with 6.6.2 we are always getting the error:

swashbuckle error

Reverting just Swashbuckle.AspNetCore back to 6.5.0 resolves the issue.

Our .AddSwaggerGen code:

                c.AddSecurityDefinition("Bearer", new OpenApiSecurityScheme
                {
                    Type = SecuritySchemeType.OAuth2,
                    Flows = new OpenApiOAuthFlows
                    {
                        ClientCredentials = new OpenApiOAuthFlow
                        {
                            TokenUrl = new Uri(Configuration["Identity:Authority"] + "/connect/token"),
                            Scopes = new Dictionary<string, string>
                            {
                                { "***.User", "" },
                                { "***.Admin", "" }
                            },
                            AuthorizationUrl = new Uri(Configuration["Identity:Authority"] + "/oauth2/authorize"),
                        }
                    }
                });
                c.AddSecurityRequirement(new OpenApiSecurityRequirement
                {
                    {
                        new OpenApiSecurityScheme
                        {
                            Reference = new OpenApiReference
                            {
                                Id = "Bearer",
                                Type = ReferenceType.SecurityScheme
                            }
                        },
                        new List<string>()
                    }
                });

            services.AddAuthentication(JwtBearerDefaults.AuthenticationScheme)
                .AddJwtBearer(o =>
                {                    
                    o.Authority = Configuration["Identity:Authority"];
                    o.RequireHttpsMetadata = bool.Parse(Configuration["Identity:RequireHttpsMetadata"]);
                    o.Audience = Configuration["Identity:Audience"];
                    o.TokenValidationParameters = new TokenValidationParameters { ValidateAudience = bool.Parse(Configuration["Identity:ValidateAudience"]) };                                        
                });

            services.AddAuthorization(options =>
            {
                options.AddPolicy("***", policy => policy.RequireClaim("client_Role", "***", "***"));
                options.AddPolicy("***", policy => policy.RequireClaim("client_Role", "***"));
            });

In our UseSwaggerUI:

                C.UseRequestInterceptor("" +
                    "(req) => { if (req.url.endsWith('connect/token') && req.body)" +
                    " req.body += '&client_id=' + client_id.value + '&client_secret=' + client_secret.value; return req; }");

Expected behavior

No response

Actual behavior

No response

Steps to reproduce

No response

Exception(s) (if any)

No response

Swashbuckle.AspNetCore version

6.6.2

.NET Version

NET8

Anything else?

If there's any other information you need please let me know.

@kenwonders kenwonders added the bug label Jun 7, 2024
@martincostello
Copy link
Collaborator

Is it possible for you to share what the rendered JavaScript in the HTML page looks like please?

@kenwonders
Copy link
Author

kenwonders commented Jun 7, 2024

Thank you hugely for helping. Here it is:

<script>
        /* Source: https://gist.github.com/lamberta/3768814
         * Parse a string function definition and return a function object. Does not use eval.
         * @param {string} str
         * @return {function}
         *
         * Example:
         *  var f = function (x, y) { return x * y; };
         *  var g = parseFunction(f.toString());
         *  g(33, 3); //=> 99
         */
        function parseFunction(str) {
            if (!str) return void (0);

            var fn_body_idx = str.indexOf('{'),
                fn_body = str.substring(fn_body_idx + 1, str.lastIndexOf('}')),
                fn_declare = str.substring(0, fn_body_idx),
                fn_params = fn_declare.substring(fn_declare.indexOf('(') + 1, fn_declare.lastIndexOf(')')),
                args = fn_params.split(',');

            args.push(fn_body);

            function Fn() {
                return Function.apply(this, args);
            }
            Fn.prototype = Function.prototype;

            return new Fn();
        }

        window.onload = function () {
            var configObject = JSON.parse('{"urls":[{"url":"/swagger/v1/swagger.json","name":"* v1"}],"deepLinking":false,"persistAuthorization":false,"displayOperationId":false,"defaultModelsExpandDepth":1,"defaultModelExpandDepth":1,"defaultModelRendering":"example","displayRequestDuration":false,"docExpansion":"list","showExtensions":false,"showCommonExtensions":false,"supportedSubmitMethods":["get","put","post","delete","options","head","patch","trace"],"tryItOutEnabled":false}');
            var oauthConfigObject = JSON.parse('{"scopeSeparator":" ","scopes":[],"useBasicAuthenticationWithAccessCodeGrant":false,"usePkceWithAuthorizationCodeGrant":false}');

            // Workaround for https://github.com/swagger-api/swagger-ui/issues/5945
            configObject.urls.forEach(function (item) {
                if (item.url.startsWith("http") || item.url.startsWith("/")) return;
                item.url = window.location.href.replace("index.html", item.url).split('#')[0];
            });

            // If validatorUrl is not explicitly provided, disable the feature by setting to null
            if (!configObject.hasOwnProperty("validatorUrl"))
                configObject.validatorUrl = null

            // If oauth2RedirectUrl isn't specified, use the built-in default
            if (!configObject.hasOwnProperty("oauth2RedirectUrl"))
                configObject.oauth2RedirectUrl = (new URL("oauth2-redirect.html", window.location.href)).href;

            // Apply mandatory parameters
            configObject.dom_id = "#swagger-ui";
            configObject.presets = [SwaggerUIBundle.presets.apis, SwaggerUIStandalonePreset];
            configObject.layout = "StandaloneLayout";

            // Parse and add interceptor functions
            var interceptors = JSON.parse('{"RequestInterceptorFunction":"(req) =\u003E { if (req.url.endsWith(\u0027connect/token\u0027) \u0026\u0026 req.body) req.body \u002B= \u0027\u0026client_id=\u0027 \u002B client_id.value \u002B \u0027\u0026client_secret=\u0027 \u002B client_secret.value; return req; }"}');
            if (interceptors.RequestInterceptorFunction)
                configObject.requestInterceptor = parseFunction(interceptors.RequestInterceptorFunction);
            if (interceptors.ResponseInterceptorFunction)
                configObject.responseInterceptor = parseFunction(interceptors.ResponseInterceptorFunction);

            // Begin Swagger UI call region

            const ui = SwaggerUIBundle(configObject);

            ui.initOAuth(oauthConfigObject);

            // End Swagger UI call region

            window.ui = ui
        }
    </script>

@martincostello
Copy link
Collaborator

Thanks - it looks like the changes I made to use System.Text.Json to support native AoT have broken the rendering here:

var interceptors = JSON.parse('{"RequestInterceptorFunction":"(req) =\u003E { if (req.url.endsWith(\u0027connect/token\u0027) \u0026\u0026 req.body) req.body \u002B= \u0027\u0026client_id=\u0027 \u002B client_id.value \u002B \u0027\u0026client_secret=\u0027 \u002B client_secret.value; return req; }"}');

@kenwonders
Copy link
Author

Really appreciate you for looking into it, than you. We'll stay on 6.5.0 as it's not problem to do so, I was just keeping our versions up to date.

@martincostello
Copy link
Collaborator

martincostello commented Jun 9, 2024

I notice we don't seem to have any test coverage for this area, so that's something we'll also need to sort out when this is fixed.

To help validate my hunch on what the issue is, if you explicitly set the following as the value of SwaggerUIOptions.JsonSerializerOptions does your code work as expected again?

Ignore, doesn't work.

app.UseSwaggerUI(c =>
{
    // Your configuration, then:
    c.JsonSerializerOptions = new()
    {
        Converters = { new JsonStringEnumConverter(JsonNamingPolicy.CamelCase, false) },
        Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping,
        DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull,
        PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
    };
});

Edit: nevermind, that isn't the fix or reason.

@martincostello martincostello self-assigned this Jun 9, 2024
@martincostello
Copy link
Collaborator

I've found the reason for this issue and it both is and isn't our fault 😄

In swagger-ui-dist@v4.15.5, which is included with v6.5.0, the name of the HTML element highlighted in red below has an client_id, which is in the scope of the closure were the interceptor function runs:

image

<input id="client_id" type="text" data-name="clientId">

In swagger-ui-dist@5.17.10, which is included with v6.6.2, the element has a different ID of client_id_authorizationCode:

<input id="client_id_authorizationCode" type="text" data-name="clientId">

client_secret was similarly renamed to client_secret_authorizationCode.

If I change your interceptor function like below to use client_id_authorizationCode, then things work as expected:

c.UseRequestInterceptor(
    "" +
    "(req) => {" +
    "  if (req.url.endsWith('connect/token') && req.body) {" +
    "    req.body += '&client_id=' + client_id_authorizationCode.value + '&client_secret=' + client_secret_authorizationCode.value;" +
    "    return req;" +
    "  }" +
    "}");

image

So this is our fault in one way because we updated swagger-ui-dist to v5, but it also has two other contributing factors.

The first is that swagger-ui did some UI refactoring and renamed the page element. I don't know if this is considered part of their public API surface area or not, but at face value it looks like an implementation detail to me that isn't intended to be something that's relied on. If it is part of their public surface area and covered by the v5 breaking changes, this feels quite out of scope of the sort of things we should be expected to find/detect/react-to within the library.

We did test that there was nothing obviously breaking with the update from v4 to v5 and didn't find anything, which is why it seemed safe to include in v6.6 (rather than needing it be part of a v7 as it was a major update).

If it isn't part of their public API surface area, then in that case the second issue is that you depended on an implementation detail of Swagger UI to get the client ID in your interceptor function.

From looking at req in the Chrome Developer Tools, the JSON equivalent of the value appears to be something like this:

{
  "url":"/auth-server/connect/token",
  "method":"post",
  "headers":{"Accept":"application/json, text/plain, */*","Content-Type":"application/x-www-form-urlencoded","X-Requested-With":"XMLHttpRequest"},
"body":"grant_type=authorization_code&code=Skg5ZRCelpGhVkzSao1QGsdZno78PONgZikGhSt0KbA&client_id=test-id&client_secret=test-secret&redirect_uri=https%3A%2F%2Flocalhost%3A5001%2Fresource-server%2Fswagger%2Foauth2-redirect.html&code_verifier=xPn7nrFvaxiTu2Z2K1JcMtHNkWY3JW5KrJtaO3Toz3c"
}

The client ID and client secret appear to be already set, so I'm not sure why you need to set it.

I'm going to leave this issue open for now as I found a few things to tweak while investigating this, but for the original issue you need to either remove your interceptor (because the client ID and secret appear to be set anyway), or to use the new input values to access their values.

@martincostello martincostello added upstream-swagger-ui external An issue in an external dependency labels Jun 9, 2024
@martincostello martincostello added this to the v6.7.0 milestone Jun 9, 2024
@martincostello
Copy link
Collaborator

See #2942 for the changes I mentioned, otherwise I think this is now resolved from our perspective.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug external An issue in an external dependency upstream-swagger-ui
Projects
None yet
Development

No branches or pull requests

2 participants