Skip to content
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

The new dotnet backend #1395

Merged
merged 9 commits into from
Mar 7, 2024
Merged

The new dotnet backend #1395

merged 9 commits into from
Mar 7, 2024

Conversation

sweco-semtto
Copy link
Contributor

The new dotnet backend.

@jacobwod
Copy link
Member

jacobwod commented Sep 6, 2023

Thanks for the review inquiry. Our organisation has fully moved away from the .NET architecture and I'm not able to do a qualified review on it.

@jacobwod jacobwod removed their request for review September 6, 2023 07:12
var adHandler = new AdHandler(_memoryCache, _logger);

string? remoteIpAddress = adHandler.GetRemoteIpAddress(HttpContext);
if (!adHandler.IpRangeRestrictionIsSet())
Copy link
Contributor

Choose a reason for hiding this comment

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

If I have IP-range set to null in config and X-Control-Header to null I will never be able to get the map. Today, I don't use any proxy or X-Control-Header. Maybe I got something backwards...........

Copy link
Member

Choose a reason for hiding this comment

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

Just had a quick look at the code and i think you are right. If you use windows-authentication we still check the ip-restriction, which should only be done when grabbing the username from the header, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, just to clarify:

"LookupActive": true,
"IdentifyUserWithWindowsAuthentication": true,
"TrustedProxyIPs": null,
"TrustedHeader": null,

Copy link
Member

Choose a reason for hiding this comment

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

@sweco-semtto Do you have any input on this? Did you test the AD-connection with windows authentication? If so, could you guide @jesade-vbg on how it should be set up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We tested the AD connection using Windows authentication. Correct me if I'm wrong, but I don't see a valid case where TrustedHeader is set to null. I will be more than happy to guide @jesade-vbg on how set this up.

Copy link
Member

@Hallbergs Hallbergs Oct 3, 2023

Choose a reason for hiding this comment

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

We're not fetching the user name from a header-key if we're using Windows authentication?

public string PickUserNameToUse(HttpRequest request, string? userName)
{
    if (IdentifyUserWithWindowsAuthentication)
        return GetWindowsAuthenticationUserName(request.HttpContext);
    else
        return GetValueFromTrustedHeader(request, userName);
}

public string GetWindowsAuthenticationUserName(HttpContext? httpContext)
{
    if (httpContext?.User?.Identity?.IsAuthenticated == true)
    {
        var userName = httpContext.User.Identity.Name;
        var userNameWithoutDomain = userName.Split('\\').Last();

        _logger.LogInformation("Authenticated user: {0}", userNameWithoutDomain);

        return userNameWithoutDomain;
    }
    else
    {
        _logger.LogInformation("User is not authenticated.");
        return string.Empty;
    }
}

I don't get why the trusted header should be set when we're dealing with the Windows authentication approach. Am i missing something? I think the code above suggests that the trusted header can be null in the Windows authentication case. Is the header key used under the hood when using windows auth?

Copy link
Member

Choose a reason for hiding this comment

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

Also, i don't think the nullish TrustedHeader is the main problem but rather why the ip-restriction must be set even when we're using Windows authentication.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sweco-semtto I can't actually see why I would need a header with user and IP-restriction when IIS is exposed at 443 and handling users with windowsAuth. Either way.... I don't have anything to give me any X-Control-Header.

@linusfj
Copy link
Member

linusfj commented Oct 23, 2023

I have tested the new version of the .NET backend, specifically the endpoints for admin. I found issues with certain endpoints, potentially due to the setup.

Notably, I get a 500 error when trying to create, update, or delete a map config in mapsettings.

@sweco-semtto
Copy link
Contributor Author

I have tested the new version of the .NET backend, specifically the endpoints for admin. I found issues with certain endpoints, potentially due to the setup.

Notably, I get a 500 error when trying to create, update, or delete a map config in mapsettings.

Thank you for your feedback. Are you sure you have the correct permissions?

@linusfj
Copy link
Member

linusfj commented Oct 26, 2023

I have resolved the 500 errors on map config endpoints by reconfiguring the local environment and changing folder permissions.

@sweco-semtto
Copy link
Contributor Author

Great, well done!

linusfj and others added 4 commits October 30, 2023 16:54
@Hallbergs
Copy link
Member

For information: We're awaiting merge of #1437 before handling this PR, as decided in the "code group".

@jacobwod jacobwod marked this pull request as draft January 8, 2024 08:26
@jacobwod
Copy link
Member

jacobwod commented Jan 8, 2024

Converted PR to draft to reflect the current status

@jacobwod jacobwod marked this pull request as ready for review March 7, 2024 08:07
@jacobwod
Copy link
Member

jacobwod commented Mar 7, 2024

Due to recent development and fixes, this branch will now replace the legacy .NET 4.5 in the main develop branch. The legacy backend can be found in the legacy-dotnet-4.5-backend branch.

There are still unresolved issues in this backend implementation, notably lack of logging and the requirement for Windows as runtime platform.

Copy link
Member

@Hallbergs Hallbergs left a comment

Choose a reason for hiding this comment

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

Approved, see: #1395 (comment)

@jacobwod jacobwod merged commit 0c6a9fe into develop Mar 7, 2024
@jacobwod jacobwod deleted the feature/1210-new-dotnet-backend branch March 7, 2024 08:16
@jacobwod jacobwod mentioned this pull request Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants