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

Introduce a validation middleware with reference tokens support #589

Closed
wants to merge 1 commit into
base: dev
from

Conversation

Projects
None yet
2 participants
@kinosang
Contributor

kinosang commented Apr 16, 2018

#433 and #443 introduced the reference tokens support,

but there's no validation middleware for that,

we should use Introspection middleware and Introspection API with HTTP calling to validate the tokens on a OpenIdConnect server itself,

so I'm trying to make the middleware.

@PinpointTownes

This comment has been minimized.

Contributor

PinpointTownes commented Apr 17, 2018

Thanks for contributing to this task, it's much appreciated! 👏

Things that would be nice to do:

  • Supporting both the default token format and reference tokens. The logic is so similar that is should only require a 2/3-line change (reference tokens are just IDs for opaque tokens stored in the DB).
  • Adding a UseReferenceTokens property to OpenIddictValidationOptions.cs
  • Renaming the project to just OpenIddict.Validation and the validation handler to OpenIddictValidationHandler. I'll consider renaming the server one to OpenIddictServerHandler in a separate PR.
  • Once this PR is done, adding a similar middleware in the release branch (for 1.0).

This middleware does not work at this time

I'm pretty sure it doesn't work yet because you must replace the default access token format when reference tokens are used. Take a look at https://github.com/openiddict/openiddict-core/blob/dev/src/OpenIddict.Server/Internal/OpenIddictInitializer.cs#L72-L95 to see how it's done for the server-side.

@PinpointTownes

This comment has been minimized.

Contributor

PinpointTownes commented Apr 17, 2018

Note: instead of copying the entire validation handler to this repo, we may want to add some protected virtual methods in the aspnet-contrib validation handler and only override the methods we need in the OpenIddict validation handler. This way, we wouldn't have two implementations to keep in sync and we would avoid code duplication.

@PinpointTownes

This comment has been minimized.

Contributor

PinpointTownes commented Apr 17, 2018

Second note: we'll also need to make the handler class generic to ensure folks can use it even if they don't use our OpenIddictToken entity.

@PinpointTownes PinpointTownes self-assigned this Apr 17, 2018

@kinosang kinosang force-pushed the JoyMoe:referencetokens-validation-middleware branch from 64b5790 to dd0fdc2 Apr 18, 2018

@kinosang

This comment has been minimized.

Contributor

kinosang commented Apr 18, 2018

@PinpointTownes Now the middleware is named OpenIddict.Validation, it extends the aspnet-contrib oauth validation middleware.

The handler class is generic now and the extension also provides generic methods.

@PinpointTownes

This comment has been minimized.

Contributor

PinpointTownes commented Apr 18, 2018

Thanks! 👍

Quick question: can we make OpenIddictValidationHandler derive from OAuthValidationHandler so we can remove all the challenge stuff from OpenIddictValidationHandler?

In a future PR, we may want to consider making the validation extensions API extend OpenIddictBuilder instead of AuthenticationBuilder. This way, we wouldn't have to specify the TToken parameter a second time, as it could be retrieved from OpenIddictBuilder.TokenType:

services.AddOpenIddict<[entity types]>(builder =>
{
    builder.AddValidation(options => options.UseReferenceTokens = true);
});

services.AddOpenIddict<[entity types]>()
    .AddValidation(options => options.UseReferenceTokens = true);

We could also adopt the same pattern for the server part, instead of magically registering the server handler when Configure is called:

services.AddOpenIddict<[entity types]>(builder =>
{
    builder.AddServer(options => options.UseReferenceTokens = true);
    builder.AddValidation(options => options.UseReferenceTokens = true);
});

services.AddOpenIddict<[entity types]>()
    .AddServer(options => options.UseReferenceTokens = true)
    .AddValidation(options => options.UseReferenceTokens = true);

(no need to update your PR to take that into account, I'll think more about that before changing anything)

<PropertyGroup>
<Description>OpenIdDict reference tokens validation middleware for ASP.NET Core.</Description>
<Authors>Kévin Chalet,Chino Chang</Authors>

This comment has been minimized.

@PinpointTownes

PinpointTownes Apr 18, 2018

Contributor

, -> ;

@@ -1,6 +1,7 @@
<Project>
<PropertyGroup Label="Package Versions">
<AspNetContribOauthExtensionsVersion>2.0.0-rc2-final</AspNetContribOauthExtensionsVersion>

This comment has been minimized.

@PinpointTownes

PinpointTownes Apr 18, 2018

Contributor

Consider using the existing AspNetContribOpenIdExtensionsVersion prop.

<ItemGroup>
<PackageReference Include="AspNet.Security.OAuth.Validation" Version="$(AspNetContribOauthExtensionsVersion)" />
<PackageReference Include="AspNet.Security.OpenIdConnect.Server" Version="$(AspNetContribOpenIdServerVersion)" />

This comment has been minimized.

@PinpointTownes

PinpointTownes Apr 18, 2018

Contributor

Consider removing that. Feel free to hardcode the Data Protection purposes instead of using nameof.

This comment has been minimized.

@PinpointTownes

PinpointTownes Apr 18, 2018

Contributor

@PinpointTownes AspNet.Security.OpenIdConnect.Server also provides the AuthenticationProperties.GetProperty() method which is required in the handler

Consider using ticket.Properties.Items.TryGetValue("property name", out string value) instead of the convenient helper 😄

/// <returns>The authentication builder.</returns>
[EditorBrowsable(EditorBrowsableState.Advanced)]
public static AuthenticationBuilder AddOpenIddictValidation(
[NotNull] this AuthenticationBuilder builder, [NotNull] string scheme)

This comment has been minimized.

@PinpointTownes

PinpointTownes Apr 18, 2018

Contributor

Let's remove all the overloads accepting a scheme (we don't support customizing the scheme name of the server handler, so we shouldn't allow replacing the default validation scheme).

{
return AuthenticateResult.Fail("Authentication failed because the access token was invalid.");
}

This comment has been minimized.

@PinpointTownes

PinpointTownes Apr 18, 2018

Contributor

For reference tokens, don't forget to add the code logic that restores the issued/expires dates in the authentication ticket:

if (Options.UseReferenceTokens)
{
    // Restore the token identifier using the unique
    // identifier attached with the database entry.
    ticket.Properties.Items[OpenIddictConstants.Properties.TokenId] = identifier;

    // Dynamically set the creation and expiration dates.
    ticket.Properties.IssuedUtc = await Tokens.GetCreationDateAsync(token);
    ticket.Properties.ExpiresUtc = await Tokens.GetExpirationDateAsync(token);

    // Restore the authorization identifier using the identifier attached with the database entry.
    ticket.Properties.Items[OpenIddictConstants.Properties.AuthorizationId] =
        await Tokens.GetAuthorizationIdAsync(token);
}
@PinpointTownes

This comment has been minimized.

Contributor

PinpointTownes commented Apr 18, 2018

Note: we'll also need to port the validation handler tests to this repo.

@kinosang

This comment has been minimized.

Contributor

kinosang commented Apr 18, 2018

@PinpointTownes

Quick question: can we make OpenIddictValidationHandler derive from OAuthValidationHandler so we can remove all the challenge stuff from OpenIddictValidationHandler?

I've tried that, but if we derive OpenIddictValidationHandler from OAuthValidationHandler, OpenIddictValidationHandler.Options will be type of OAuthValidationOptions, so we'll got:

There is no implicit reference conversion from 'OpenIddict.Validation.OpenIddictValidationHandler<TToken>' to 'Microsoft.AspNetCore.Authentication.AuthenticationHandler<OpenIddict.Validation.OpenIddictValidationOptions>'
@PinpointTownes

This comment has been minimized.

Contributor

PinpointTownes commented Apr 18, 2018

You can workaround it by doing that:

private new OpenIddictValidationOptions Options => (OpenIddictValidationOptions) base.Options;
@kinosang

This comment has been minimized.

Contributor

kinosang commented Apr 18, 2018

@PinpointTownes Nope, the error is reported at OpenIddictValidationExtensions L88.

If we derive OpenIddictValidationHandler from OAuthValidationHandler, it will not accept OpenIddictValidationOptions as the first parameter for AuthenticationBuilder.AddScheme.

@kinosang kinosang force-pushed the JoyMoe:referencetokens-validation-middleware branch from 2bf71cc to 61916a3 Apr 18, 2018

@PinpointTownes

This comment has been minimized.

Contributor

PinpointTownes commented Apr 18, 2018

If we derive OpenIddictValidationHandler from OAuthValidationHandler, it will not accept OpenIddictValidationOptions as the first parameter for AuthenticationBuilder.AddScheme.

Replace the builder.AddScheme call by:

// Register the OpenIddict validation handler in the authentication options,
// so it can be discovered by the default authentication handler provider.
builder.Services.Configure<AuthenticationOptions>(options =>
{
    // Note: similarly to Identity, OpenIddict should be registered only once.
    // To prevent multiple schemes from being registered, a check is made here.
    if (options.SchemeMap.ContainsKey(OpenIddictValidationDefaults.AuthenticationScheme))
    {
        return;
    }

    options.AddScheme(OpenIddictValidationDefaults.AuthenticationScheme, scheme =>
    {
        scheme.HandlerType = typeof(OpenIddictValidationHandler<TToken>);
    });
});

builder.Services.Configure(OpenIddictValidationDefaults.AuthenticationScheme, configuration);

return builder;

I just tested and it works like a charm.

@@ -52,6 +52,12 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "OpenIddict.Server", "src\Op
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "OpenIddict.Server.Tests", "test\OpenIddict.Server.Tests\OpenIddict.Server.Tests.csproj", "{07B02B98-8A68-432D-A932-48E6D52B221A}"
EndProject
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "extensions", "extensions", "{C45757B6-F8F0-4556-8A8A-16BBD1C59FDA}"

This comment has been minimized.

@PinpointTownes

PinpointTownes Apr 18, 2018

Contributor

Could you please remove that and move the validation folder to the main src?

@PinpointTownes PinpointTownes added this to the 2.0.0-rc3 milestone Apr 18, 2018

@PinpointTownes

This comment has been minimized.

Contributor

PinpointTownes commented Apr 18, 2018

In a future PR, we may want to consider making the validation extensions API extend OpenIddictBuilder instead of AuthenticationBuilder.

I made a prototype for the server part: #590

@kinosang

This comment has been minimized.

Contributor

kinosang commented Apr 18, 2018

@PinpointTownes gotcha, I'll update this PR asap.

@kinosang kinosang force-pushed the JoyMoe:referencetokens-validation-middleware branch from 61916a3 to d8770e4 Apr 18, 2018

@kinosang kinosang force-pushed the JoyMoe:referencetokens-validation-middleware branch from 060efab to 461d6f4 Apr 18, 2018

@kinosang

This comment has been minimized.

Contributor

kinosang commented Apr 18, 2018

@PinpointTownes All done, and I'll follow the changes of #590 once it's merged.

@kinosang kinosang changed the title from [WIP] Create a validation middleware for ReferenceTokens to Introduce a validation middleware with reference tokens support Apr 20, 2018

@PinpointTownes

This comment has been minimized.

Contributor

PinpointTownes commented Apr 27, 2018

Merged (with some major modifications to use the new builders API): 03a2705

It was also backported to OpenIddict 1.x: 59c3d51

Thanks for your contribution! 🎉

@PinpointTownes

This comment has been minimized.

Contributor

PinpointTownes commented Apr 27, 2018

Note: I'll take some time tomorrow to publish an announcement for both the validation middleware and the revamped OpenIddict services registration APIs.

@kinosang kinosang deleted the JoyMoe:referencetokens-validation-middleware branch Apr 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment