Wednesday, 30 December 2020

Azure Pipelines meet Jest

This post explains how to integrate the tremendous test runner Jest with the continuous integration platform Azure Pipelines. Perhaps we're setting up a new project and we've created a new React app with Create React App. This ships with Jest support out of the box. How do we get that plugged into Pipelines such that:

  1. Tests run as part of our pipeline
  2. A failing test fails the build
  3. Test results are reported in Azure Pipelines UI?

Tests run as part of our pipeline

First of all, lets get the tests running. Crack open your azure-pipelines.yml file and, in the appropriate place add the following:

        - task: Npm@1
          displayName: npm run test
          inputs:
            command: 'custom'
            workingDir: 'src/client-app'
            customCommand: 'run test'

The above will, when run, trigger a npm run test in the src/client-app folder of my project (it's here where my React app lives). You'd imagine this would just work™️ - but life is not that simple. This is because Jest, by default, runs in watch mode. This is blocking and so not appropriate for CI.

In our src/client-app/package.json let's create a new script that runs the tests but not in watch mode:

        "test:ci": "npm run test -- --watchAll=false",

and switch our azure-pipelines.yml to use it:

        - task: Npm@1
          displayName: npm run test
          inputs:
            command: 'custom'
            workingDir: 'src/client-app'
            customCommand: 'run test:ci'

Boom! We're now running tests as part of our pipeline. And also, failing tests will fail the build, because of Jest's default behaviour of exiting with status code 1 on failed tests.

Tests results are reported in Azure Pipelines UI

Pipelines has a really nice UI for reporting test results. If you're using something like .NET then you'll find that test results just magically show up there. We'd like that for our Jest tests as well. And we can have it.

The way we achieve this is by:

  1. Producing test results in a format that can be subsequently processed
  2. Using those test results to publish to Azure Pipelines

The way that you configure Jest test output is through usage of reporters. However, Create React App doesn't support these. However that's not an issue, as the marvellous Dan Abramov demonstrates here.

We need to install the jest-junit package to our client-app:

npm install jest-junit --save-dev

And we'll tweak our test:ci script to use the jest-junit reporter as well:

        "test:ci": "npm run test -- --watchAll=false --reporters=default --reporters=jest-junit",

We also need to add some configuration to our package.json in the form of a jest-junit element:

    "jest-junit": {
        "suiteNameTemplate": "{filepath}",
        "outputDirectory": ".",
        "outputName": "junit.xml"
    }

The above configuration will use the name of the test file as the suite name in the results, which should speed up the tracking down of the failing test. The other values specify where the test results should be published to, in this case the root of our client-app with the filename junit.xml.

Now our CI is producing our test results, how do we get them into Pipelines? For that we need the Publish test results task and a new step in our azure-pipelines.yml after our npm run test step:

        - task: Npm@1
          displayName: npm run test
          inputs:
            command: 'custom'
            workingDir: 'src/client-app'
            customCommand: 'run test:ci'

        - task: PublishTestResults@2
          displayName: 'supply npm test results to pipelines'
          condition: succeededOrFailed() # because otherwise we won't know what tests failed
          inputs:
            testResultsFiles: 'src/client-app/junit.xml'

This will read the test results from our src/client-app/junit.xml file and pump them into Pipelines. Do note that we're always running this step; so if the previous step failed (as it would in the case of a failing test) we still pump out the details of what that failure was. Like so:

And that's it! Azure Pipelines and Jest integrated.

Tuesday, 22 December 2020

Prettier your CSharp with dotnet-format and lint-staged

Consistent formatting is a good thing. It makes code less confusing to newcomers and it allows whoever is working on the codebase to reliably focus on the task at hand. Not "fixing curly braces because Janice messed them up with her last commit". (A git commit message that would be tragic in so many ways.)

Once you've agreed that you want to have consistent formatting, you want it to be enforced. Enter, stage left, Prettier, the fantastic tool for formatting code. It rocks; I've been using on my JavaScript / TypeScript for the longest time. But what about C#? Well, there is a Prettier plugin for C#.... Sort of. It appears to be abandoned and contains the worrying message in the README.md:

Please note that this plugin is under active development, and might not be ready to run on production code yet. It will break your code.

Not a ringing endorsement.

dotnet-format: a new hope

Margarida Pereira recently pointed me in the direction of dotnet-format which is a formatter for .NET. It's a .NET tool which:

is a code formatter for dotnet that applies style preferences to a project or solution. Preferences will be read from an .editorconfig file, if present, otherwise a default set of preferences will be used.

It can be installed with:

dotnet tool install -g dotnet-format

The VS Code C# extension will make use of this formatter, you just need to set the following in your settings.json:

    "omnisharp.enableRoslynAnalyzers": true,
    "omnisharp.enableEditorConfigSupport": true

Customising your formatting

If you'd like to deviate from the default formatting options then create yourself a .editorconfig file in the root of your project. Let's say you prefer more of the K & R style approach to braces instead of the C# default of Allman style. To make dotnet-format use that you'd set the following:

# Remove the line below if you want to inherit .editorconfig settings from higher directories
root = true

# See https://github.com/dotnet/format/blob/master/docs/Supported-.editorconfig-options.md for reference
[*.cs]
csharp_new_line_before_open_brace = none
csharp_new_line_before_catch = false
csharp_new_line_before_else = false
csharp_new_line_before_finally = false
csharp_new_line_before_members_in_anonymous_types = false
csharp_new_line_before_members_in_object_initializers = false
csharp_new_line_between_query_expression_clauses = true

With this in place it's K & R all the way baby!

lint-staged integration

It's become somewhat standard to use the marvellous husky and lint-staged to enforce code quality. To quote the docs:

Run linters against staged git files and don't let 💩 slip into your code base!

So, before I happened upon dotnet-format I was already enforcing TypeScript / JavaScript style with the following entry in my package.json:

"husky": {
    "hooks": {
        "pre-commit": "lint-staged"
    }
},
"lint-staged": {
    "*.{js,ts,tsx}": "prettier --write"
}

The above configuration runs Prettier against files which have been staged for commit, provided they have the suffix .js or .ts or .tsx. How can we get dotnet-format in the mix also? Like so:

"husky": {
    "hooks": {
        "pre-commit": "lint-staged --relative"
    }
},
"lint-staged": {
    "*.cs": "dotnet format --include",
    "*.{js,ts,tsx}": "prettier --write"
}

We've done two things here. First, we've changed the lint-staged command to include the parameter --relative. This is because dotnet-format only deals with relative paths. Prettier is pretty flexible, so we can make this change without breaking anything.

Secondly we've added the *.cs task of dotnet format --include. This is the task that will be run on commit, when lint-staged runs, it will pass a list of relative file paths to dotnet format, the --include accepts a list of relative file or folder paths to include in formatting. So if you'd staged two files it might end up executing a command like this:

dotnet format --include src/server-app/Server/Controllers/UserController.cs src/server-app/Server/Controllers/WeatherForecastController.cs

By and large we don't have to think about this; the important take home is that we're now enforcing standardised formatting for all C# files upon commit. Everything that goes into the codebase will be formatted in a consistent fashion.

Monday, 21 December 2020

How to make Azure AD 403

This post is about how you can customise ASP.NETs integration with Azure Active Directory to customise the behaviour that redirects unauthorized requests to the AccessDenied endpoint. If you're using the tremendous Azure Active Directory for authentication with ASP.NET then there's a good chance you're using the Microsoft.Identity.Web library. It's this that allows us to drop the following statement into the ConfigureServices method of our Startup class:

services.AddMicrosoftIdentityWebAppAuthentication(Configuration);

Which (combined with configuration in our appsettings.json files) hooks us up with Azure AD for authentication. This is 95% awesome. The 5% is what we're here for. Here's a screenshot of the scenario that troubles us:

We've made a request to /WeatherForecast; a secured endpoint (a controller decorated with the Authorize attribute). We're authenticated; the app knows who we are. But we're not authorized / allowed to access this endpoint. We don't have permission. The HTTP specification caters directly for this scenario with status code 403 Forbidden:

The 403 (Forbidden) status code indicates that the server understood the request but refuses to authorize it.

However, Microsoft.Identity.Web is ploughing another furrow. Instead of returning 403, it's returning 302 Found and redirecting the browser to https://localhost:5001/Account/AccessDenied?ReturnUrl=%2FWeatherForecast. Now the intentions here are great. If you wanted to implement a page in your application at that endpoint that displayed some kind of useful message it would be really useful. However, what if you want the more HTTP-y behaviour instead? In the case of a HTTP request triggered by JavaScript (typical for Single Page Applications) then this redirect isn't that helpful. JavaScript doesn't really know what to do with the 302 and whilst you could code around this, it's not desirable.

We want 403 - we don't want 302.

Give us 403

You can have this behaviour by dropping the following code after your services.AddMicrosoftIdentityWebAppAuthentication:

services.Configure<CookieAuthenticationOptions>(CookieAuthenticationDefaults.AuthenticationScheme, options =>
{
    options.Events.OnRedirectToAccessDenied = new Func<RedirectContext<CookieAuthenticationOptions>, Task>(context =>
    {
        context.Response.StatusCode = StatusCodes.Status403Forbidden;
        return context.Response.CompleteAsync();
    });
});

This code hijacks the redirect to AccessDenied and transforms it into a 403 instead. Tremendous! What does this look like?

This is the behaviour we want!

Extra customisation bonus points

You may want to have some nuance to the way you handle unauthorized requests. Because of the nature of OnRedirectToAccessDenied this is entirely possible; you have complete access to the requests coming in which you can use to direct behaviour. To take a single example, let's say we want to direct normal browsing behaviour (AKA humans clicking about in Chrome) which is not authorized to a given screen, otherwise provide 403s. What would that look like?

services.Configure<CookieAuthenticationOptions>(CookieAuthenticationDefaults.AuthenticationScheme, options =>
{
    options.Events.OnRedirectToAccessDenied = new Func<RedirectContext<CookieAuthenticationOptions>, Task>(context =>
    {
        var isRequestForHtml = context.Request.Headers["Accept"].ToString().Contains("text/html");
        if (isRequestForHtml) {
            context.Response.StatusCode = StatusCodes.Status302Found;
            context.Response.Headers["Location"] = "/unauthorized";
        }
        else {
            context.Response.StatusCode = StatusCodes.Status403Forbidden;
        }

        return context.Response.CompleteAsync();
    });
});

So above, we check the request Accept headers and see if they contain "text/html"; which we're using as a signal that the request came from a users browsing. (This may not be bulletproof; better suggestions gratefully received.) If the request does contain a "text/html" Accept header then we redirect the client to an /unauthorized screen, otherwise we return 403 as we did before. Super flexible and powerful!

Sunday, 20 December 2020

Nullable reference types; CSharp's very own strictNullChecks

'Tis the season to play with new compiler settings! I'm a very keen TypeScript user and have been merrily using strictNullChecks since it shipped. I was dimly aware that C# was also getting a similar feature by the name of nullable reference types.

It's only now that I've got round to taking at look at this marvellous feature. I thought I'd share what moving to nullable reference types looked like for me; and what code changes I found myself making as a consequence.

Turning on nullable reference types

To turn on nullable reference types in a C# project you should pop open the .csproj file and ensure it contains a <Nullable>enable</Nullable>. So if you had a .NET Core 3.1 codebase it might look like this:

  <PropertyGroup>
    <TargetFramework>netcoreapp3.1</TargetFramework>
    <Nullable>enable</Nullable>
  </PropertyGroup>

When you compile from this point forward, possible null reference types are reported as warnings. Consider this C#:

    [ApiController]
    public class UserController : ControllerBase
    {
        private readonly ILogger<UserController> _logger;

        public UserController(ILogger<UserController> logger)
        {
            _logger = logger;
        }

        [AllowAnonymous]
        [HttpGet("UserName")]
        public string GetUserName()
        {
            if (User.Identity.IsAuthenticated) {
                _logger.LogInformation("{User} is getting their username", User.Identity.Name);
                return User.Identity.Name;
            }

            _logger.LogInformation("The user is not authenticated");
            return null;
        }
    }

A dotnet build results in this:

dotnet build --configuration release

Microsoft (R) Build Engine version 16.7.1+52cd83677 for .NET
Copyright (C) Microsoft Corporation. All rights reserved.

  Determining projects to restore...
  Restored /Users/jreilly/code/app/src/server-app/Server/app.csproj (in 471 ms).
Controllers/UserController.cs(38,24): warning CS8603: Possible null reference return. [/Users/jreilly/code/app/src/server-app/Server/app.csproj]
Controllers/UserController.cs(42,20): warning CS8603: Possible null reference return. [/Users/jreilly/code/app/src/server-app/Server/app.csproj]
  app -> /Users/jreilly/code/app/src/server-app/Server/bin/release/netcoreapp3.1/app.dll
  app -> /Users/jreilly/code/app/src/server-app/Server/bin/release/netcoreapp3.1/app.Views.dll

Build succeeded.

Controllers/UserController.cs(38,24): warning CS8603: Possible null reference return. [/Users/jreilly/code/app/src/server-app/Server/app.csproj]
Controllers/UserController.cs(42,20): warning CS8603: Possible null reference return. [/Users/jreilly/code/app/src/server-app/Server/app.csproj]
    2 Warning(s)
    0 Error(s)

You see the two "Possible null reference return." warnings? Bingo

Really make it hurt

This is good - information is being surfaced up. But it's a warning. I could ignore it. I like compilers to get really up in my face and force me to make a change. I'm not into warnings; I'm into errors. Know what works for you. If you're similarly minded, you can upgrade nullable reference warnings to errors by tweaking the .csproj a touch further. Add yourself a <WarningsAsErrors>nullable</WarningsAsErrors> element. So maybe your .csproj now looks like this:

  <PropertyGroup>
    <TargetFramework>netcoreapp3.1</TargetFramework>
    <Nullable>enable</Nullable>
    <WarningsAsErrors>nullable</WarningsAsErrors>
  </PropertyGroup>

And a dotnet build will result in this:

dotnet build --configuration release

Microsoft (R) Build Engine version 16.7.1+52cd83677 for .NET
Copyright (C) Microsoft Corporation. All rights reserved.

  Determining projects to restore...
  Restored /Users/jreilly/code/app/src/server-app/Server/app.csproj (in 405 ms).
Controllers/UserController.cs(38,24): error CS8603: Possible null reference return. [/Users/jreilly/code/app/src/server-app/Server/app.csproj]
Controllers/UserController.cs(42,20): error CS8603: Possible null reference return. [/Users/jreilly/code/app/src/server-app/Server/app.csproj]

Build FAILED.

Controllers/UserController.cs(38,24): error CS8603: Possible null reference return. [/Users/jreilly/code/app/src/server-app/Server/app.csproj]
Controllers/UserController.cs(42,20): error CS8603: Possible null reference return. [/Users/jreilly/code/app/src/server-app/Server/app.csproj]
    0 Warning(s)
    2 Error(s)

Yay! Errors!

What do they mean?

"Possible null reference return" isn't the clearest of errors. What does that actually amount to? Well, it amounts to the compiler saying "you're a liar! (maybe)". Let's look again at the code where this error is reported:

        [AllowAnonymous]
        [HttpGet("UserName")]
        public string GetUserName()
        {
            if (User.Identity.IsAuthenticated) {
                _logger.LogInformation("{User} is getting their username", User.Identity.Name);
                return User.Identity.Name;
            }

            _logger.LogInformation("The user is not authenticated");
            return null;
        }

We're getting that error reported where we're returning null and where we're returning User.Identity.Name which may be null. And we're getting that because as far as the compiler is concerned string has changed. Before we turned on nullable reference types the compiler considered string to mean string OR null. Now, string means string.

This is the same sort of behaviour as TypeScripts strictNullChecks. With TypeScript, before you turn on strictNullChecks, as far as the compiler is concerned, string means string OR null OR undefined (JavaScript didn't feel one null-ish value was enough and so has two - don't ask). Once strictNullChecks is on, string means string.

It's a lot clearer. And that's why the compiler is getting antsy. The method signature is string, but it can see null potentially being returned. It doesn't like it. By and large that's good. We want the compiler to notice this as that's the entire point. We want to catch accidental nulls before they hit a user. This is great! However, what do you do if have a method (as we do) that legitimately returns a string or null?

Widening the type to include null

We change the signature from this:

        public string GetUserName()

To this:

        public string? GetUserName()

That's right, the simple addition of ? marks a reference type (like a string) as potentially being null. Adding that means that we're potentially returning null, but we're sure about it; there's intention here - it's not accidental. Wonderful!

Wednesday, 9 December 2020

azure-pipelines-task-lib and isOutput setVariable

Some blog posts are insightful treatises on the future of web development, some are "here's how I solved my problem". This is most assuredly the latter.

I'm writing an custom pipelines task extension for Azure Pipelines. It's written with TypeScript and the azure-pipelines-task-lib.

The pipeline needs to output a variable. Azure Pipelines does that using the setvariable command combined with isOutput=true. This looks something like this: ##vso[task.setvariable variable=myOutputVar;isOutput=true]this is the value".

The bad news is that the lib doesn't presently support isOutput=true. Gosh it makes me sad. Hopefully in future it will be resolved. But what now?

For now we can hack ourselves a workaround:

import * as tl from 'azure-pipelines-task-lib/task';
import * as tcm from 'azure-pipelines-task-lib/taskcommand';
import * as os from 'os';

/**
 * Sets a variable which will be output as well.
 *
 * @param     name    name of the variable to set
 * @param     val     value to set
 * @param     secret  whether variable is secret.  Multi-line secrets are not allowed.  Optional, defaults to false
 * @returns   void
 */
export function setOutputVariable(name: string, val: string, secret = false): void {
    // use the implementation of setVariable to set all the internals,
    // then subsequently set the output variable manually
    tl.setVariable(name, val, secret);

    const varValue = val || '';

    // write the command
    // see https://docs.microsoft.com/en-us/azure/devops/pipelines/process/variables?view=azure-devops&tabs=yaml%2Cbatch#set-a-multi-job-output-variable
    _command(
        'task.setvariable',
        { variable: name || '', isOutput: 'true', issecret: (secret || false).toString() },
        varValue
    );
}

const _outStream = process.stdout;

function _writeLine(str: string): void {
    _outStream.write(str + os.EOL);
}

function _command(command: string, properties: any, message: string) {
    const taskCmd = new tcm.TaskCommand(command, properties, message);
    _writeLine(taskCmd.toString());
}

The above is effectively a wrapper for the existing setVariable. However, once it's called into the initial implementation, setOutputVariable then writes out the same variable once more, but this time bolting on isOutput=true.

Finally, I've raised a PR to see if isOutput can be added directly to the library. You can track progress on that here.