Skip to content

Consider replacing AuthorizationGrantType and ClientAuthenticationMethod #10506

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

Closed
philwebb opened this issue Oct 3, 2017 · 8 comments
Closed
Assignees
Labels
type: task A general task
Milestone

Comments

@philwebb
Copy link
Member

philwebb commented Oct 3, 2017

We could replace our own AuthorizationGrantType and ClientAuthenticationMethod enums with Spring Security classes and use custom meta-data for hints.

@philwebb philwebb added this to the 2.0.0.M6 milestone Oct 3, 2017
@snicoll
Copy link
Member

snicoll commented Oct 4, 2017

Can you explain why you've created those enums in the first place because I meant to ask and forgot.

Two things:

  1. There are no meta-data required for enums. As long as the IDE is able to see the class, it should provide value auto-completion for it. If the source of the enum is available, it should attempt to display the documentation (so that's important the enum's javadoc as something consistent with the way we document things if possible)
  2. There is no meta-data for Map-based key. So even if this was necessary it wouldn't be possible right now. We need bot a breaking change in the format of the key (possibly) and an update of all IDEs support to make that happen. I am quite sad that the provider property has no auto-completion for the providers we know. The hints section has all that's required for it but we can't write a hint for a property that's referenced via a map.

@mbhave mbhave self-assigned this Oct 5, 2017
@mbhave
Copy link
Contributor

mbhave commented Oct 5, 2017

@snicoll The enums were originally added for meta-data. But if I understood what you said correctly, generating meta-data or a hint for keys in a map is not possible today. So even if we removed the enums, it wouldn't be possible to add custom meta-data to get hints in this case?

@snicoll
Copy link
Member

snicoll commented Oct 5, 2017

I am not sure that accurately describes what I meant.

Enums don't need metadata, they are processed by the IDE. So if you have a 100% matching enum in Spring Security, use that.

Yes, we can't add hints for something that's bound to a map or a list in the hierarchy.

@philwebb
Copy link
Member Author

philwebb commented Oct 5, 2017

I think I see the confusion. The versions in Spring Security are not enums. They're regular classes with a few static final constants that you can use if you want.

In Spring Security you can do:

AuthorizationGrantType.AUTHORIZATION_CODE, but you can also do new AuthorizationGrantType("something-else").

@mbhave mbhave closed this as completed in 3ced841 Oct 6, 2017
@mbhave
Copy link
Contributor

mbhave commented Oct 6, 2017

Enums don't need metadata

@snicoll I'm still slightly unsure what that means for enums that are used as values in maps.
I replaced the enums with the Spring Security classes. If anyone thinks we need to keep the enums, please feel to reopen.

@snicoll snicoll reopened this Oct 7, 2017
@snicoll
Copy link
Member

snicoll commented Oct 7, 2017

Nothing specific. Enums don't need metadata. Ever.

Perhaps the confusing part is that what I mean here is that you'll get auto-completion for enums regardless where it's defined (maps or non maps). That change however broke the meta-data as you no longer have auto-completion.

You can test it by using the key in your IDE. Can we restore proper meta-data for those keys please? Or do we want to support arbitrary values for those?

@mbhave
Copy link
Contributor

mbhave commented Oct 9, 2017

I'm on the fence about that. Spring Security supports arbitrary values although BASIC and POST seem like the most common ones. Maybe we should support arbitrary values too.

@philwebb
Copy link
Member Author

philwebb commented Oct 9, 2017

In this case I think we should forgo the IDE support in favor of matching what Spring Security does. If we stick with the enum there's no way for the user to pick anything other than BASIC and POST.

Once #9945 is fixed we can retrofit meta-data to give IDE hints.

It's also worth considering that most users will hopefully be using one of the common providers and will only need to venture into custom configuration very occasionally.

@philwebb philwebb closed this as completed Oct 9, 2017
@philwebb philwebb modified the milestones: 2.0.0.M6, 2.0.0.M5 Oct 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: task A general task
Projects
None yet
Development

No branches or pull requests

3 participants