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

[Feature Request] Make internal constants class public #548

Closed
UM001 opened this issue Sep 4, 2020 · 8 comments
Closed

[Feature Request] Make internal constants class public #548

UM001 opened this issue Sep 4, 2020 · 8 comments
Assignees
Labels
enhancement New feature or request fixed
Milestone

Comments

@UM001
Copy link

UM001 commented Sep 4, 2020

In the AccountController example constants are used. However, those constants are internal and not public usable.
Maybe make them usable? Same for AzureAdB2C, I used it from other library instead this one.

https://github.com/AzureAD/microsoft-identity-web/blob/master/src/Microsoft.Identity.Web.UI/Areas/MicrosoftIdentity/Controllers/AccountController.cs

namespace Microsoft.Identity.Web
{
//
// Summary:
// General constants.
[NullableAttribute(0)]
[NullableContextAttribute(1)]
internal static class Constants
{
public const string TenantDiscoveryEndpoint = "tenant_discovery_endpoint";
public const string AzureAdB2C = "AzureAdB2C";
public const string PreferredUserName = "preferred_username";
public const string NameClaim = "name";
public const string Tfp = "tfp";
public const string Consent = "consent";
public const string ConsentUrl = "consentUri";
`

@UM001 UM001 added the enhancement New feature or request label Sep 4, 2020
@jennyf19
Copy link
Collaborator

jennyf19 commented Sep 4, 2020

makes sense. @jmprieur thoughts? only issue i see is breaking changes if we change the constant values, we have flexibility to do that right now.

@jennyf19 jennyf19 changed the title [Feature Request] [Feature Request] Make internal constants class public Sep 4, 2020
@jmprieur
Copy link
Collaborator

jmprieur commented Sep 4, 2020

yes, I agree. This is the problem (avoiding breaking changes)
@UM001 : is there constants in particular you'd want to see?

@UM001
Copy link
Author

UM001 commented Sep 4, 2020

For the account controller I used next.
I believe the AzureAd and AzureAdB2c are also commonly used, and api. If that can avoid adding other references that would be great.

public static class Constants { public const string Policy = "policy"; public const string LoginHint = "loginhint"; public const string DomainHint = "domainhint"; public const string Claims = "claims"; public const string Scope = "scope"; }

@pmaytak
Copy link
Contributor

pmaytak commented Sep 4, 2020

The idea behind the Constants class was so that we internally don't have to type out all the literal strings, which can be added/changed/removed as the library develops. This class has a lot of unrelated constants, which will not be useful to the end users and, if made public, can add more confusion to the API.

There's public ClaimConstants class that has some of the constants that you mentioned. This class is more stable since it represents constants for claim types that won't change.

I am hesitant to make Constants.cs public. But maybe we can extract and move some of the constants (that are useful to the users) to a different public class like ClaimConstants.

@jmprieur
Copy link
Collaborator

@UM001 : which constants would you want to see public?

@jennyf19
Copy link
Collaborator

jennyf19 commented Sep 12, 2020

@jmprieur - @UM001 listed them above.
I went ahead and did a quick experiment w/moving some of our constants to a public. Opened a pr to gather feedback. Thought better to start w/a smaller list, as we can add to it, but not take away.

@UM001 would especially appreciate your input. I didn't include "Policy" as we use UserFlow for B2C, and AzureAD also has the concept of Policy, so I thought it might be more confusing. I've included the rest on your list.

@UM001
Copy link
Author

UM001 commented Sep 12, 2020

I think you have added them already. Great.

public static class Constants { public const string Policy = "policy"; public const string LoginHint = "loginhint"; public const string DomainHint = "domainhint"; public const string Claims = "claims"; public const string Scope = "scope"; public const string UILocales = "uilocales"; public const string AzureAd = "AzureAd"; public const string AzureAdB2c = "AzureAdB2c"; }

and those are of Woodgrove sample:

public static class ClaimTypes { public const string IdentityProvider = "http://schemas.microsoft.com/identity/claims/identityprovider"; public const string Issuer = "iss"; public const string Name = "name"; public const string ObjectIdentifier = "http://schemas.microsoft.com/identity/claims/objectidentifier"; public const string OrganizationName = "organization_name"; public const string OwnerIdentifier = "owner_id"; public const string TenantIdentifier = "http://schemas.microsoft.com/identity/claims/tenantid"; public const string TrustFrameworkPolicy = "tfp"; }

This class also holds constants.
System.Security.Claims { // // Summary: // Defines constants for the well-known claim types that can be assigned to a subject. // This class cannot be inherited. public static class ClaimTypes

@jennyf19 jennyf19 added fixed and removed fixed labels Sep 14, 2020
@jennyf19 jennyf19 added this to the GA version milestone Sep 14, 2020
@jmprieur jmprieur added the fixed label Sep 17, 2020
@jmprieur
Copy link
Collaborator

@UM001, this is available in Microsoft.Identity.Web 1.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request fixed
Projects
None yet
Development

No branches or pull requests

4 participants