CODE RUB: Implent NHS Login frontend and remove MSAL by davidhayes03 · Pull Request #209 · NHSISL/IDecide

davidhayes03 · 2026-04-23T11:46:36Z

Copilot

Pull request overview

This PR switches the Manage app’s authentication approach toward an NHS Login (CIS2) / BFF-cookie + server session model, while de-emphasizing the existing MSAL-based client flow.

Changes:

Added server-side CIS2 auth endpoints (/auth/login, /auth/callback, /auth/session, /auth/logout) and a new EF User store.
Introduced SQL-backed session caching + Data Protection key persistence, plus secure token/session storage helpers.
Updated the React client to use /auth/* endpoints and cookie-based session checks instead of MSAL templates/providers.

Reviewed changes

Copilot reviewed 24 out of 24 changed files in this pull request and generated 25 comments.

Show a summary per file

File	Description
LondonDataServices.IDecide.Manage.Server/appsettings.json	Adds SessionCache connection string for SQL-backed session storage.
LondonDataServices.IDecide.Manage.Server/TokenResult.cs	Adds DTO for CIS2 token refresh responses.
LondonDataServices.IDecide.Manage.Server/Services/TokenService.cs	Adds refresh-token-based access token retrieval/refresh logic.
LondonDataServices.IDecide.Manage.Server/Services/SecureTokenStorage.cs	Adds DataProtection-encrypted token + CSRF state storage in session.
LondonDataServices.IDecide.Manage.Server/Services/ITokenService.cs	Introduces access-token retrieval abstraction.
LondonDataServices.IDecide.Manage.Server/Properties/launchSettings.json	Alters HTTPS profile binding settings.
LondonDataServices.IDecide.Manage.Server/Program.cs	Configures session, cookies, Data Protection, CIS2 SDK, and new DbContext.
LondonDataServices.IDecide.Manage.Server/Models/User.cs	Adds EF model for CIS2 users and authorisation flag.
LondonDataServices.IDecide.Manage.Server/Models/NhsUserInfo.cs	Adds CIS2 userinfo/NRBAC role models.
LondonDataServices.IDecide.Manage.Server/LondonDataServices.IDecide.Manage.Server.csproj	Adds SQL cache + NHS Digital SDK package references.
LondonDataServices.IDecide.Manage.Server/Data/ApplicationDbContext .cs	Adds EF DbContext for the new `User` table.
LondonDataServices.IDecide.Manage.Server/Controllers/AuthController.cs	Implements login/callback/session/logout endpoints and cookie sign-in.
LondonDataServices.IDecide.Manage.Client/vite.config.ts	Adds `/auth/` proxy and changes dev-server port.
LondonDataServices.IDecide.Manage.Client/src/services/foundations/authService.ts	Adds react-query hooks for session and logout.
LondonDataServices.IDecide.Manage.Client/src/models/sessions/SessionData.ts	Adds client session model (includes `expiresAt`).
LondonDataServices.IDecide.Manage.Client/src/main.tsx	Stops passing MSAL instance into `App` but still initializes MSAL.
LondonDataServices.IDecide.Manage.Client/src/hooks/useAuth.ts	Adds cookie-session polling + expiry-based auto logout.
LondonDataServices.IDecide.Manage.Client/src/components/securitys/userProfile.tsx	Switches profile display to server session data + expiry display.
LondonDataServices.IDecide.Manage.Client/src/components/root.tsx	Replaces MSAL templates with session-based route guarding.
LondonDataServices.IDecide.Manage.Client/src/components/layouts/navbar.tsx	Adds dropdown user menu and logout based on session data.
LondonDataServices.IDecide.Manage.Client/src/components/layouts/loginUnauth.tsx	Switches sign-in to `/auth/login`.
LondonDataServices.IDecide.Manage.Client/src/brokers/apiBroker.ts	Removes MSAL token injection; uses cookie credentials for requests.
LondonDataServices.IDecide.Manage.Client/src/brokers/apiBroker.authSession.ts	Adds broker calls for `/auth/session` and `/auth/logout`.
LondonDataServices.IDecide.Manage.Client/src/App.tsx	Removes `MsalProvider` wrapper and adds `/unauthorised` route.

Comments suppressed due to low confidence (1)

LondonDataServices.IDecide.Manage.Server/Program.cs:239

AddSwaggerGen() is registered twice and AddControllers() is also called twice in this method. This is redundant and can lead to confusing configuration ordering. Remove the duplicates so each service is registered once in a single place.

            builder.Services.AddSwaggerGen();
            builder.Services.AddScoped<ISecureTokenStorage, SecureTokenStorage>();
            builder.Services.AddScoped<ITokenService, TokenService>();
            builder.Services.AddSingleton(invisibleApiKey);
            builder.Services.AddAuthorization();
            builder.Services.AddDbContext<StorageBroker>();

            builder.Services.AddDbContext<ApplicationDbContext>(options =>
                options.UseSqlServer(
                    configuration.GetConnectionString("IDecideConnectionString")));

            builder.Services.AddHttpContextAccessor();
            builder.Services.AddHttpClient();

            // Learn more about configuring Swagger/OpenAPI at https://aka.ms/aspnetcore/swashbuckle
            builder.Services.AddEndpointsApiExplorer();
            builder.Services.AddSwaggerGen();
            builder.Services.AddControllers();
            AddProviders(builder.Services, builder.Configuration);
            AddBrokers(builder.Services, builder.Configuration);
            AddFoundationServices(builder.Services);
            AddOrchestrationServices(builder.Services, builder.Configuration);
            AddClients(builder.Services);
            //  AddProcessingServices(builder.Services);
            //  AddCoordinationServices(builder.Services, builder.Configuration);

            // Register IConfiguration to be available for dependency injection
            builder.Services.AddSingleton<IConfiguration>(builder.Configuration);
            JsonNamingPolicy jsonNamingPolicy = JsonNamingPolicy.CamelCase;

            builder.Services.AddControllers()
               .AddOData(options =>
               {
                   options.AddRouteComponents("odata", GetEdmModel());

cjdutoit

REJECTED: Split this PR up. Need PR's for:

Do a storage broker if you need a complete new DbContext with models and migration
Do a PR for TokenService and add its unit tests and proper exception handling and validation
Do a PR for SecureTokenStorageService. All lumped into one file and currently no tests
Can't see where TokenStorage and Token Service is used, you probably need an orchestration service and a separate PR for the EXPOSER.
All UI stuff in its own PR

cjdutoit · 2026-04-23T11:59:42Z

+{
+    [ApiController]
+    [Route("[controller]")]
+    public class AuthController : Controller


Missing all the unit tests and security tests for this controller

cjdutoit · 2026-04-23T12:02:03Z

+
+public class ApplicationDbContext : DbContext
+{
+    public ApplicationDbContext(DbContextOptions<ApplicationDbContext> options)


Why do we have a seperate DbContext? Why not use the existing IDecide database.

Dont see any DI registration for this or connection strings or migrations

cjdutoit · 2026-04-23T12:03:20Z

+
+namespace LondonDataServices.IDecide.Manage.Server.Services
+{
+    public class TokenService : ITokenService


Service implemented without unit tests

Copilot

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated 20 comments.

Comments suppressed due to low confidence (1)

LondonDataServices.IDecide.Manage.Server/Program.cs:230

AddSwaggerGen() and AddControllers() are registered twice. This is redundant and can lead to confusing configuration if options get added in both places. Keep a single registration and consolidate any configuration into one call chain.

            builder.Services.AddSwaggerGen();
            //builder.Services.AddScoped<ISecureTokenStorage, SecureTokenStorage>();
            builder.Services.AddSingleton(invisibleApiKey);
            builder.Services.AddAuthorization();
            builder.Services.AddDbContext<StorageBroker>();

            builder.Services.AddDbContext<ApplicationDbContext>(options =>
                options.UseSqlServer(
                    configuration.GetConnectionString("IDecideConnectionString")));

            builder.Services.AddHttpContextAccessor();
            builder.Services.AddHttpClient();

            // Learn more about configuring Swagger/OpenAPI at https://aka.ms/aspnetcore/swashbuckle
            builder.Services.AddEndpointsApiExplorer();
            builder.Services.AddSwaggerGen();
            builder.Services.AddControllers();
            AddProviders(builder.Services, builder.Configuration);
            AddBrokers(builder.Services, builder.Configuration);
            AddFoundationServices(builder.Services);
            AddOrchestrationServices(builder.Services, builder.Configuration);
            AddClients(builder.Services);

            // Register IConfiguration to be available for dependency injection
            builder.Services.AddSingleton<IConfiguration>(builder.Configuration);
            JsonNamingPolicy jsonNamingPolicy = JsonNamingPolicy.CamelCase;

            builder.Services.AddControllers()
               .AddOData(options =>

Copilot · 2026-04-23T14:03:02Z

+            builder.Services.AddAuthentication("bff-cookie")
+                .AddCookie("bff-cookie", options =>
+                {
+                    options.Events.OnRedirectToLogin = context =>
+                    {
+                        context.Response.StatusCode = StatusCodes.Status401Unauthorized;
+                        return Task.CompletedTask;
+                    };
+                    options.LoginPath = "/Login";
+                    options.LogoutPath = "/Logout";
+                    options.ExpireTimeSpan = TimeSpan.FromDays(7);
+                    options.Cookie.HttpOnly = true;
+                    options.Cookie.SecurePolicy = CookieSecurePolicy.Always;
+                    options.Cookie.SameSite = SameSiteMode.Lax;
+                    options.Cookie.Name = "bff-cookie";
+                });


The cookie auth options point LoginPath/LogoutPath at /Login and /Logout, but this controller exposes /auth/login and /auth/logout. If anything triggers a challenge/sign-out using these paths (outside of the custom 401 handler), it will route to non-existent endpoints. Update the cookie options to match the actual auth routes.

Copilot · 2026-04-23T14:03:03Z

+                this.logger.LogWarning("Authorization code is missing from callback");
+                return BadRequest("Authorization code is missing");
+            }
+
+            if (string.IsNullOrEmpty(state))
+            {
+                this.logger.LogWarning("State parameter is missing from callback");


Repository formatting rule: any return that is not the first statement in its block must be preceded by exactly one blank line. These return BadRequest(...) statements follow a log call without a blank line; insert a blank line before each return to match the codebase formatting rules.

Suggested change

this.logger.LogWarning("Authorization code is missing from callback");

return BadRequest("Authorization code is missing");

}

if (string.IsNullOrEmpty(state))

{

this.logger.LogWarning("State parameter is missing from callback");

this.logger.LogWarning("Authorization code is missing from callback");

return BadRequest("Authorization code is missing");

}

if (string.IsNullOrEmpty(state))

{

this.logger.LogWarning("State parameter is missing from callback");

Copilot · 2026-04-23T14:03:03Z

+export class SessionData {
+    sub: string;
+    upn: string;
+    name: string;
+    roles: string[];
+    expiresAt: string;
+
+    constructor(
+        sub: string,
+        upn: string,
+        name: string,
+        roles: string[],
+        expiresAt: string
+    ) {
+        this.sub = sub;
+        this.upn = upn;
+        this.name = name;
+        this.roles = roles;
+        this.expiresAt = expiresAt;
+    }


SessionData.expiresAt is modeled as a required field, but the current /auth/session response from the server does not include it. This will leave expiresAt undefined at runtime and break the expiry checks/UI. Make expiresAt optional (or add it to the backend response) to keep the types and runtime payload aligned.

Copilot · 2026-04-23T14:03:04Z

+            HttpContext.Session.Clear();
+            await HttpContext.SignOutAsync("bff-cookie");
+
+            return Redirect(@"\");


Redirect(@"\\") uses a backslash, which produces an unusual/invalid redirect target in browsers. Use a forward-slash root path ("/") (or return a proper logout result as discussed) so navigation is consistent across platforms.

Suggested change

return Redirect(@"\");

return Redirect("/");

Copilot · 2026-04-23T14:03:04Z

+                var claims = new List<Claim>
+                {
+                    new Claim(ClaimTypes.NameIdentifier, userInfo.Sub),
+                    new Claim(ClaimTypes.Name, userInfo.Name),
+                    new Claim(ClaimTypes.Upn, userInfo.NhsIdUserUid),
+                };
+
+                foreach (var role in userInfo.NhsIdNrbacRoles)
+                {
+                    claims.Add(new Claim(ClaimTypes.Role, role.RoleName));
+                }


This controller adds ClaimTypes.Role values from userInfo.NhsIdNrbacRoles[*].RoleName, but the rest of the API enforces roles like LondonDataServices.IDecide.Manage.Server.Administrators via [Authorize(Roles = ...)]. Unless CIS2 role names exactly match those strings, every role-protected endpoint will return 403. Introduce a mapping from CIS2/NRBAC roles to the app’s existing role names (or update the authorization policies/attributes to match the new role strategy).

Copilot · 2026-04-23T14:03:07Z

+
+

There are two consecutive blank lines before the end of useGetSession. Remove the extra blank line to keep formatting consistent with the rest of the client code and avoid noisy diffs.

Suggested change

Copilot · 2026-04-23T14:03:08Z

@@ -24,6 +26,7 @@
 using LondonDataServices.IDecide.Core.Brokers.DateTimes;
 using LondonDataServices.IDecide.Core.Brokers.Identifiers;
 using LondonDataServices.IDecide.Core.Brokers.Loggings;
+using LondonDataServices.IDecide.Core.Brokers.NhsDigitalApi;
 using LondonDataServices.IDecide.Core.Brokers.Notifications;
 using LondonDataServices.IDecide.Core.Brokers.Pds;
 using LondonDataServices.IDecide.Core.Brokers.Securities;
@@ -49,16 +52,23 @@
 using LondonDataServices.IDecide.Core.Services.Orchestrations.Consumers;
 using LondonDataServices.IDecide.Core.Services.Orchestrations.Decisions;
 using LondonDataServices.IDecide.Core.Services.Orchestrations.Patients;
-using Microsoft.AspNetCore.Authentication.JwtBearer;
+using LondonDataServices.IDecide.Manage.Server.Data;
 using Microsoft.AspNetCore.Builder;
+using Microsoft.AspNetCore.DataProtection;
+using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.OData;
 using Microsoft.EntityFrameworkCore;
 using Microsoft.Extensions.Configuration;
 using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Extensions.Hosting;
+using Microsoft.Identity.Client.Extensions.Msal;
 using Microsoft.Identity.Web;
 using Microsoft.OData.Edm;
 using Microsoft.OData.ModelBuilder;
+using NHSDigital.ApiPlatform.Sdk;
+using NHSDigital.ApiPlatform.Sdk.AspNetCore;
+using NHSDigital.ApiPlatform.Sdk.Models.Configurations;
+using static System.Collections.Specialized.BitVector32;


Several using directives appear unused (System.Runtime.InteropServices, Microsoft.Identity.Client.Extensions.Msal, Microsoft.Identity.Web, and static System.Collections.Specialized.BitVector32). These will produce compiler warnings and make the entrypoint harder to read. Remove unused usings (and any unused locals like azureAdOptions) to keep the file clean.

Copilot · 2026-04-23T14:03:08Z

+            HttpContext.Session.Clear();
+            await HttpContext.SignOutAsync("bff-cookie");
+
+            return Redirect(@"\");


Logout returns an HTTP redirect (and uses a backslash path), but the frontend logout broker expects a JSON body with a logoutUrl. This will cause response.json() to fail and logout to break. Align the contract: either return JSON from /auth/logout or update the client to follow redirects and not parse JSON.

Suggested change

return Redirect(@"\");

return Ok(new

{

logoutUrl = "/"

});

Copilot · 2026-04-23T14:03:08Z

+public class ApplicationDbContext : DbContext
+{
+    public ApplicationDbContext(DbContextOptions<ApplicationDbContext> options)
+        : base(options)
+    {
+    }
+
+    // Add your DbSets here
+    public DbSet<User> Users { get; set; }
+
+    protected override void OnModelCreating(ModelBuilder modelBuilder)
+    {
+        base.OnModelCreating(modelBuilder);
+
+        // Configure your entities here
+        modelBuilder.Entity<User>(entity =>
+        {
+            entity.HasKey(e => e.Id);
+            entity.Property(e => e.NhsIdUserUid).HasMaxLength(50).IsRequired();
+            entity.Property(e => e.Name).HasMaxLength(200).IsRequired();
+            entity.HasIndex(e => e.NhsIdUserUid).IsUnique();
+        });
+    }


This file introduces a second EF Core ApplicationDbContext + Manage.Server.Models.User, but the Core StorageBroker already has a Users DbSet and migrations for LondonDataServices.IDecide.Core.Models.Foundations.Users.User. Duplicating the model/context risks schema drift and migration-history conflicts, especially since both contexts target the same connection string. Prefer reusing the existing Core user model/context (or clearly separate databases/migration history tables if a second context is intentional).

davidhayes03 added 2 commits April 23, 2026 11:39

CODE RUB: Frontend Server updates

7d5cdc4

CODE RUB: Login with Helathcare Worker credentials

aa0f79e

davidhayes03 requested review from cjdutoit and Copilot and removed request for Copilot April 23, 2026 11:46

github-actions Bot added the CODE RUB The code rub category is for small changes (not for bug fixes) label Apr 23, 2026

Copilot started reviewing on behalf of davidhayes03 April 23, 2026 11:46 View session

CODE RUB: Lint Changes

c4c7fb3

Copilot AI review requested due to automatic review settings April 23, 2026 11:50

Copilot started reviewing on behalf of davidhayes03 April 23, 2026 11:50 View session

Copilot AI reviewed Apr 23, 2026

View reviewed changes

cjdutoit requested changes Apr 23, 2026

View reviewed changes

davidhayes03 added 2 commits April 23, 2026 14:49

CODE RUB: Remove not needed code

83587a5

CODE RUB: Port change

f52dcb7

Copilot AI review requested due to automatic review settings April 23, 2026 13:51

Copilot started reviewing on behalf of davidhayes03 April 23, 2026 13:52 View session

Copilot AI reviewed Apr 23, 2026

View reviewed changes

davidhayes03 added the draft label Apr 30, 2026

-            return Redirect(@"\");
+            return Ok(new
+            {
+                logoutUrl = "/"
+            });

Conversation

davidhayes03 commented Apr 23, 2026 • edited by azure-boards Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cjdutoit left a comment

Choose a reason for hiding this comment

Uh oh!

cjdutoit Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

cjdutoit Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

cjdutoit Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

davidhayes03 commented Apr 23, 2026 •

edited by azure-boards Bot

Loading