ActiveLogin / ActiveLogin.Authentication

Support Swedish BankID (svenskt BankID) authentication in .NET. Unofficial package, not created by BankID.
https://activelogin.net
MIT License
216 stars 75 forks source link

Add possibility to pass personalNumber to BankIdFlowService.InitializeSign #444

Closed nahojd closed 7 months ago

nahojd commented 7 months ago

We have been using ActiveLogin for quite some time, and are just upgrading to the latest version (we've been stuck on 5.0 due to some breaking changes). We are letting our users sign with our custom SignalR-based flow, that previously used the IBankIdApiClient directly. However, since we need to provide QR-codes for signing as well now, we realised that it would be better to use the BankIdFlowService instead.

This almost works, but we are unable to provide the personalNumber of the already logged in user when we initialize the signing.

In the BankIdFlowService, you always pass null for personalNumber:

//https://github.com/ActiveLogin/ActiveLogin.Authentication/blob/main/src/ActiveLogin.Authentication.BankId.Core/Flow/BankIdFlowService.cs

public async Task<BankIdFlowInitializeResult> InitializeSign(BankIdFlowOptions flowOptions, BankIdSignData bankIdSignData, string returnRedirectUrl)
{
    var detectedUserDevice = _bankIdSupportedDeviceDetector.Detect();
    var response = await GetSignResponse(flowOptions, bankIdSignData, detectedUserDevice);

    await _bankIdEventTrigger.TriggerAsync(new BankIdInitializeSuccessEvent(personalIdentityNumber: null, response.OrderRef, detectedUserDevice, flowOptions));

...
}

It would be really helpful if it was possible to pass the personalNumber in the InitializeSign-method!

PeterOrneholm commented 7 months ago

Hi @nahojd!

Glad to hear that you are using Active Login, if you want to share your experiences using tit with SignalR, that would be awesome to have in the docs or as a feature in the future.

Anyway, not sure that what you describe is what you want?

Previously, before "Säker Start", we had access to the persona identity number in this place. But now, no personal identity number is known during the initialize phase as that is returned once the identity/sign process is done. Note that the sign event you are refering to is our "internal" eventing system used for logging etc and is not sent to the BankID backend.

If you already have a user "signed in" and need that number for this event, I don't think that is anything we should, or even can, provide as it very much depends on how you handle and store the current user. If you need the number when the event is triggered, I would implement and event handler that checks for the current user using your system.

Am I onto something or did I misunderstand your request?

The /sign endpoint DOES allow for passing a personalIdentityNumber but that is only for verificaiton, not to bypass the need for QR code etc. https://www.bankid.com/utvecklare/guider/teknisk-integrationsguide/graenssnittsbeskrivning/sign As I can remember, this property we only have support for at the API level, not in the core package, but as stated, this is only one level of extra security, won't remove the need for Säker Start.

nahojd commented 7 months ago

The verification is what I'm after, not bypassing "Säker start". If we don't pass personalNumber to /sign, it seems that anyone who can scan the QR-code can complete the signing, not just the person that is logged in.

We are using the auth component from ActiveLogin without any changes, but for signing we want a more integrated user experience, and that's why we're using a SignalR hub that does all the talking to BankID. But of course we still want to utilise ActiveLogin as much as possible.

At first I used the BankIDAppClient directly, and that worked fine, but since we had to provide the QR-code to the user, and all those classes are internal (why would you do that?), I figured that it would be easier to use the BankIdFlowService instead. And it is! Except that we are now unable to pass the personalNumber requirement to the /sign operation.

I would be happy to share our SignalR implementation once we've got it working again, but first this needs to be sorted out. :-)

nahojd commented 7 months ago

Oh, right. I read your answer again, and looked at the code again, and I understood what you meant that the event is just for internal use. Actually, where I would like you to make the change is in the GetSignRequest method:

private SignRequest GetSignRequest(BankIdFlowOptions flowOptions, BankIdSignData bankIdSignData)
  {
      var endUserIp = _bankIdEndUserIpResolver.GetEndUserIp();
      var certificatePolicies = flowOptions.CertificatePolicies.Any() ? flowOptions.CertificatePolicies : null;
      var requestRequirement = new Requirement(certificatePolicies, flowOptions.RequirePinCode, flowOptions.RequireMrtd);

      return new SignRequest(
          endUserIp,
          bankIdSignData.UserVisibleData,
          userNonVisibleData: bankIdSignData.UserNonVisibleData,
          userVisibleDataFormat: bankIdSignData.UserVisibleDataFormat,
          requirement: requestRequirement
      );
  }

If BankIdFlowOptions included the personalNumber requirement and you added it here, everything would work. And since peronalNumber is a requirement that you are supposed to be able to send to /sign, I don't see why you couldn't just add it.

nahojd commented 7 months ago

@PeterOrneholm I have created a pull request to show the very small changes that would be needed. https://github.com/ActiveLogin/ActiveLogin.Authentication/pull/445

PeterOrneholm commented 7 months ago

Fixed by #445