Why client_id must not contain spaces? #315

Closed
xperiandri opened this Issue Jul 17, 2016 · 6 comments

Comments

3 participants
@xperiandri

Is there a specification requirement that client_id must not contain spaces?
https://github.com/aspnet-contrib/AspNet.Security.OpenIdConnect.Server/blob/dev/src/AspNet.Security.OpenIdConnect.Extensions/OpenIdConnectExtensions.cs#L1133
Is this a purpose of this check existence?

@PinpointTownes

This comment has been minimized.

Show comment
Hide comment
@PinpointTownes

PinpointTownes Jul 17, 2016

Member

Is there a specification requirement that client_id must not contain spaces?

No. Spaces are forbidden because multiple values are (currently) stored as space-separated values.

Do you have a concrete scenario where supporting spaces would be important for you?

Member

PinpointTownes commented Jul 17, 2016

Is there a specification requirement that client_id must not contain spaces?

No. Spaces are forbidden because multiple values are (currently) stored as space-separated values.

Do you have a concrete scenario where supporting spaces would be important for you?

@xperiandri

This comment has been minimized.

Show comment
Hide comment
@xperiandri

xperiandri Jul 17, 2016

I've done this

                context.Applications.Add(new OpenIddictApplication<Guid>
                {
                    ClientId = "Android test",
                });
                context.Applications.Add(new OpenIddictApplication<Guid>
                {
                    ClientId = "Android",
                });
                context.Applications.Add(new OpenIddictApplication<Guid>
                {
                    ClientId = "Windows test",
                });

And we could not figure out what is wrong for a week!

The exception is absolutely not descriptive and when it bubbles up you have no idea what is wrong.
Why it is necessary to store this values separated by spaces?

xperiandri commented Jul 17, 2016

I've done this

                context.Applications.Add(new OpenIddictApplication<Guid>
                {
                    ClientId = "Android test",
                });
                context.Applications.Add(new OpenIddictApplication<Guid>
                {
                    ClientId = "Android",
                });
                context.Applications.Add(new OpenIddictApplication<Guid>
                {
                    ClientId = "Windows test",
                });

And we could not figure out what is wrong for a week!

The exception is absolutely not descriptive and when it bubbles up you have no idea what is wrong.
Why it is necessary to store this values separated by spaces?

@PinpointTownes

This comment has been minimized.

Show comment
Hide comment
@PinpointTownes

PinpointTownes Jul 17, 2016

Member

Why it is necessary to store this values separated by spaces?

It's not. But it's an efficient serialization and until now, nobody has ever complained about this "limitation" (because well, using spaces in client_id is totally uncommon). If we wanted to support spaces, I guess we'd have to switch to a more complex serialization scheme, like JSON or XML.

Member

PinpointTownes commented Jul 17, 2016

Why it is necessary to store this values separated by spaces?

It's not. But it's an efficient serialization and until now, nobody has ever complained about this "limitation" (because well, using spaces in client_id is totally uncommon). If we wanted to support spaces, I guess we'd have to switch to a more complex serialization scheme, like JSON or XML.

@xperiandri

This comment has been minimized.

Show comment
Hide comment
@xperiandri

xperiandri Jul 17, 2016

We must either implement a check in https://github.com/openiddict/openiddict-core/blob/dev/src/OpenIddict.EntityFramework/Models/OpenIddictApplication.cs#L35 and rethrow a more descriptive exception up on stack.
Or eliminate this limitation.

To avoid issues.
I would prefer not to add limitations over the standard.

We must either implement a check in https://github.com/openiddict/openiddict-core/blob/dev/src/OpenIddict.EntityFramework/Models/OpenIddictApplication.cs#L35 and rethrow a more descriptive exception up on stack.
Or eliminate this limitation.

To avoid issues.
I would prefer not to add limitations over the standard.

@PinpointTownes PinpointTownes added enhancement and removed question labels Jul 17, 2016

@PinpointTownes PinpointTownes added this to the 1.0.0-beta7 milestone Jul 17, 2016

@PinpointTownes PinpointTownes self-assigned this Jul 17, 2016

@jayrulez

This comment has been minimized.

Show comment
Hide comment
@jayrulez

jayrulez Jul 18, 2016

+1 for removing the limitation.

+1 for removing the limitation.

@PinpointTownes

This comment has been minimized.

Show comment
Hide comment
@PinpointTownes

PinpointTownes Jul 18, 2016

Member

Alright. I'll consider fixing that in the next milestone 😄

Member

PinpointTownes commented Jul 18, 2016

Alright. I'll consider fixing that in the next milestone 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment