Fix required tenant in OpenId Validation settings UI

[salmattia]

Fix #17233. Now one of tenant or authority is required. Tenant can be empty when an external authority validates tokens.
Error is displayed on save only if both tenant and authority fields are not filled out

[github-actions]

Thank you for submitting your first pull request, awesome! 🚀 If you haven't already, please take a moment to review our contribution guide. This guide provides helpful information to ensure your contribution aligns with our standards. A core team member will review your pull request.

[salmattia]

@dotnet-policy-service agree company="Imatra"

[kevinchalet]

Thanks for your PR, @salmattia!

[kevinchalet]

While we're at it, let's use string.IsNullOrEmpty() instead of string.IsNullOrWhiteSpace() lines 73 and 75 😃

[salmattia]

Done!

[kevinchalet]

Looks great!

I'll let @MikeAlhayek merge it and backport it to make sure it's included in the next patch/minor release.

Happy holidays! 🎄

[salmattia]

Looks great!

I'll let @MikeAlhayek merge it and backport it to make sure it's included in the next patch/minor release.

Happy holidays! 🎄

Thank you! Happy holidays

[MikeAlhayek]

@kevinchalet I see couple of issues with the code. I think the code should look like this instead. do you agree?

I think the old code should be kept. Instead, replace the line

settings.Authority = !string.IsNullOrEmpty(model.Authority) ? new Uri(model.Authority, UriKind.Absolute) : null;

with this code

        if (string.IsNullOrEmpty(model.Authority))
        {
            context.Updater.ModelState.AddModelError(Prefix, nameof(model.Authority), S["A tenant or authority value is required."]);
        }
        else 
        {
            settings.Authority = new Uri(model.Authority, UriKind.Absolute)
        }

If Authority should be only validated if there is not Tenant, then we may need to fix the error to this instead

context.Updater.ModelState.AddModelError(Prefix, nameof(model.Authority), S["A tenant or authority value is required."]);

[kevinchalet]

I think the code should look like this instead. do you agree?

Personally, I'd even remove all this validation code and simply rely on OpenIdValidationService, that eventually performs similar checks anyway:

OrchardCore/src/OrchardCore.Modules/OrchardCore.OpenId/Services/OpenIdValidationService.cs

Lines 80 to 223 in d64eb52

	public async Task<ImmutableArray<ValidationResult>> ValidateSettingsAsync(OpenIdValidationSettings settings)
	{
	ArgumentNullException.ThrowIfNull(settings);
	var results = ImmutableArray.CreateBuilder<ValidationResult>();
	if (!(settings.Authority == null ^ string.IsNullOrEmpty(settings.Tenant)))
	{
	results.Add(new ValidationResult(S["Either a tenant or an authority must be registered."], new[]
	{
	nameof(settings.Authority),
	nameof(settings.Tenant),
	}));
	}
	if (settings.Authority != null)
	{
	if (!settings.Authority.IsAbsoluteUri || !settings.Authority.IsWellFormedOriginalString())
	{
	results.Add(new ValidationResult(S["The specified authority is not valid."], new[]
	{
	nameof(settings.Authority)
	}));
	}
	if (!string.IsNullOrEmpty(settings.Authority.Query) || !string.IsNullOrEmpty(settings.Authority.Fragment))
	{
	results.Add(new ValidationResult(S["The authority cannot contain a query string or a fragment."], new[]
	{
	nameof(settings.Authority),
	}));
	}
	}
	if (settings.MetadataAddress != null)
	{
	if (!settings.MetadataAddress.IsAbsoluteUri || !settings.MetadataAddress.IsWellFormedOriginalString())
	{
	results.Add(new ValidationResult(S["The specified metadata address is not valid."], new[]
	{
	nameof(settings.MetadataAddress),
	}));
	}
	if (!string.IsNullOrEmpty(settings.MetadataAddress.Query) || !string.IsNullOrEmpty(settings.MetadataAddress.Fragment))
	{
	results.Add(new ValidationResult(S["The metadata address cannot contain a query string or a fragment."], new[]
	{
	nameof(settings.MetadataAddress),
	}));
	}
	if (!string.IsNullOrEmpty(settings.Tenant))
	{
	results.Add(new ValidationResult(S["No metatada address can be set when using another tenant."], new[]
	{
	nameof(settings.MetadataAddress),
	}));
	}
	}
	if (!string.IsNullOrEmpty(settings.Tenant) && !string.IsNullOrEmpty(settings.Audience))
	{
	results.Add(new ValidationResult(S["No audience can be set when using another tenant."], new[]
	{
	nameof(settings.Audience),
	}));
	}
	if (settings.Authority != null && string.IsNullOrEmpty(settings.Audience))
	{
	results.Add(new ValidationResult(S["An audience must be set when configuring the authority."], new[]
	{
	nameof(settings.Audience),
	}));
	}
	if (settings.Authority == null && settings.DisableTokenTypeValidation)
	{
	results.Add(new ValidationResult(S["Token type validation can only be disabled for remote servers."], new[]
	{
	nameof(settings.DisableTokenTypeValidation),
	}));
	}
	if (!string.IsNullOrEmpty(settings.Audience) &&
	settings.Audience.StartsWith(OpenIdConstants.Prefixes.Tenant, StringComparison.OrdinalIgnoreCase))
	{
	results.Add(new ValidationResult(S["The audience cannot start with the special 'oct:' prefix."], new[]
	{
	nameof(settings.Audience),
	}));
	}
	// If a tenant was specified, ensure it is valid, that the OpenID server feature
	// was enabled and that at least a scope linked with the current tenant exists.
	if (!string.IsNullOrEmpty(settings.Tenant) &&
	!string.Equals(settings.Tenant, _shellSettings.Name, StringComparison.Ordinal))
	{
	if (!_shellHost.TryGetSettings(settings.Tenant, out var shellSettings))
	{
	results.Add(new ValidationResult(S["The specified tenant is not valid."]));
	}
	else
	{
	var shellScope = await _shellHost.GetScopeAsync(shellSettings);
	await shellScope.UsingAsync(async scope =>
	{
	var options = scope.ServiceProvider.GetRequiredService<IOptionsMonitor<OpenIddictServerOptions>>().CurrentValue;
	if (options.UseReferenceAccessTokens)
	{
	results.Add(new ValidationResult(S["Selecting a server tenant for which reference access tokens are enabled is currently not supported."], new[]
	{
	nameof(settings.Tenant),
	}));
	}
	var manager = scope.ServiceProvider.GetService<IOpenIdScopeManager>();
	if (manager == null)
	{
	results.Add(new ValidationResult(S["The specified tenant is not valid."], new[]
	{
	nameof(settings.Tenant),
	}));
	}
	else
	{
	var resource = OpenIdConstants.Prefixes.Tenant + _shellSettings.Name;
	if (!await manager.FindByResourceAsync(resource).AnyAsync())
	{
	results.Add(new ValidationResult(S["No appropriate scope was found."], new[]
	{
	nameof(settings.Tenant),
	}));
	}
	}
	});
	}
	}
	return results.ToImmutable();
	}

[MikeAlhayek]

@kevinchalet The OpenIdValidationService is good. But in the driver, we assign prefix for the validation error key names. Also, if for some reason the property names in OpenIdValidationSettingsViewModel did not match the same name, some errors may not be printed on the UI.

I think its better to leave the validation in the driver "I know it repeated logic", but it's clear what we are validating and what keys we are settings with the errors.

Please check out the updated validation logic. If you are good with this, I'll merge and portback.

[kevinchalet]

Also, if for some reason the property names in OpenIdValidationSettingsViewModel did not match the same name, some errors may not be printed on the UI.

Well, if you're concerned by that, we should be duplicating all the validation checks and not just a single one? 😄

If you strongly think we should have a duplicated check, let's merge it. Otherwise, just remove it.

[MikeAlhayek]

I'll merge this one and back port it. We can submit another PR in v3 that removed the validation from the driver.

[github-actions]

Congratulations on your first PR merge! 🎉 Thank you for your contribution! We're looking forward to welcoming other contributions of yours in the future. @all-contributors please add @salmattia for code.

[allcontributors]

@github-actions[bot]

I've put up a pull request to add @salmattia! 🎉

[MikeAlhayek]

/backport to release/2.1

[github-actions]

Started backporting to release/2.1: https://github.com/OrchardCMS/OrchardCore/actions/runs/12486787988