Skip to content
This repository has been archived by the owner on Dec 13, 2018. It is now read-only.

Remote auth expiration fix #893

Merged
merged 3 commits into from Jul 19, 2016
Merged

Conversation

tuespetre
Copy link
Contributor

Fixes #855, which documents a bug where the RemoteAuthenticationTimeout (intended to be used for a correlation cookie) also ends up getting used for the ExpiresUtc value of the AuthenticationProperties of the actual authentication ticket that gets issued.

I did not see a generic set of tests for RemoteAuthenticationHandler so I refrained from adding a test for now.

@dnfclas
Copy link

dnfclas commented Jul 8, 2016

Hi @tuespetre, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

@Tratcher Tratcher self-assigned this Jul 19, 2016
@Tratcher
Copy link
Member

Notes: Moving ISystemClock to AuthenticationOptions should be OK, it was already on every child class in this repo.

@@ -149,7 +149,7 @@ protected virtual void GenerateCorrelationId(AuthenticationProperties properties
{
HttpOnly = true,
Secure = Request.IsHttps,
Expires = properties.ExpiresUtc
Expires = Options.SystemClock.UtcNow.Add(Options.RemoteAuthenticationTimeout),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add this to TwitterHandler.HandleUnauthorizedAsync as well.

@Tratcher
Copy link
Member

:shipit: pending the update to Twitter.

@Tratcher Tratcher added this to the 1.1.0 milestone Jul 19, 2016
@tuespetre
Copy link
Contributor Author

🆙📅

@HaoK
Copy link
Member

HaoK commented Jul 19, 2016

Should we consider putting the system clock in DI as a service instead of hanging them off the options? Its not like we would ever expect more than one system clock across all the different middlewares, etc right?

@HaoK
Copy link
Member

HaoK commented Jul 19, 2016

Basically what I'm suggesting instead of putting the clock in AuthenticationOptions, instead add it as a property to AuthenticationMiddleware/Handler that gets injected optionally (and defaults to a new instance of SystemClock same as today)

   // On AuthenticationMiddleware and AuthenticationHandler
   public virtual ISystemClock SystemClock { get; set; } = new SystemClock()

  // Invoke needs to set the system clock on AuthenticationHandler after its created (patching intialize basically)
        public async Task Invoke(HttpContext context)
        {
            var handler = CreateHandler();  
            handler.SystemClock = SystemClock;
            ...

  // Update all of the various AuthMiddleware ctors 
        public CookieAuthenticationMiddleware(
            RequestDelegate next,
            IDataProtectionProvider dataProtectionProvider,
            ILoggerFactory loggerFactory,
            UrlEncoder urlEncoder,
            IOptions<CookieAuthenticationOptions> options,
            ISystemClock clock = null)  
        if (clock != null) {
            SystemClock = clock;
        }
        ...


@Tratcher
Copy link
Member

We've considered moving it to a service before, but it's only used for unit tests and its easier to set in options.

@Tratcher Tratcher merged commit 6cd46a5 into aspnet:dev Jul 19, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants