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.

Sign up for GitHub

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

Jump to bottom

[oauth] User-defined scope in AccessTokenResponse #1891

Merged
merged 10 commits into from
Dec 23, 2020
Merged

[oauth] User-defined scope in AccessTokenResponse #1891

merged 10 commits into from
Dec 23, 2020

Conversation

clinique
Copy link
Contributor

@clinique clinique commented Dec 5, 2020 •
edited
Loading

Would this be acceptable as a solution to my problem faced with Netatmo presented in #1888 ?
Signed-off-by: clinique [email protected]

@clinique
Copy link
Contributor Author

clinique commented Dec 6, 2020

@kaikreuzer or @cweitkamp : can somebody have a look at this ? Sorry to be pushy, the rewritting of the Netatmo binding is pending this. Thanks.

@cweitkamp
Copy link
Contributor

Thank you for this draft. To be honest I am not feeling comfortable with it right now even if it looks good. I am not convinced to add workarounds in OHC for bad vendor implementations. But I agree with you that we need a solution for it and a similar issue may occur in the future for other external APIs.

What about moving the code for the AccessTokenDeserializer into the add-on bundle and just open the OHC OAuth implementation to allow a bindings developer pass a customized deserializer if needed?

@clinique
Copy link
Contributor Author

clinique commented Dec 6, 2020 •
edited
Loading

@cweitkamp : I like your approach and will think on it.
Do you think supercharging the OAuthFactory createOAuthClientService method with a serializer class is the good approach ?

@clinique
Copy link
Contributor Author

clinique commented Dec 6, 2020

@cweitkamp : can you give me your feelings regarding this approach ?

@clinique
Copy link
Contributor Author

@cweitkamp : had a chance to take a look ?

@cweitkamp
Copy link
Contributor

@clinique Thanks for the reminder. I am afraid I was not able to look into it yet. It looks better than before but I have to admit that I am not feeling comfortable with this solution either. Sry to be nit-picking here.

Do you think we can design it like a builder pattern for the OAuthClientService without touching the factory itself? Similar to classes in Jetty library? Imo the method to create services are already overloaded by parameters. E.g. a developer will be able to do something like this:

OAuthClientService service = OAuthFactory.createOAuthClientService(String, String, String, String, String, String, Boolean).withDeserializer(Class<T>);

This might need a little bit more refactoring but will be a lot more flexible to add more features in the future.

@clinique
Copy link
Contributor Author

Do you think we can design it like a builder pattern for the OAuthClientService without touching the factory itself? Similar to classes in Jetty library? Imo the method to create services are already overloaded by parameters. E.g. a developer will be able to do something like this:

OAuthClientService service = OAuthFactory.createOAuthClientService(String, String, String, String, String, String, Boolean).withDeserializer(Class<T>);

This might need a little bit more refactoring but will be a lot more flexible to add more features in the future.

Hello @cweitkamp , thanks for the suggestion but it is unclear to me how I could reach this with only a single method withDeserializer(Class <T>), I think I would need a build() also and this would be a breaking change.

Can you give me some insights on how you imagine this ?

@clinique
Copy link
Contributor Author

@cweitkamp : thinking of it twice, I think I found the solution. Can you tell me what you think of it ? Thanks

cweitkamp
cweitkamp reviewed Dec 22, 2020
View reviewed changes
Copy link
Contributor

@cweitkamp cweitkamp left a comment

Choose a reason for hiding this comment

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

Thanks for your patience. One final comment. After that I am fine with merging this nice and useful improvement.

....oauth2client/src/main/java/org/openhab/core/auth/oauth2client/internal/PersistedParams.java
@@ -32,6 +32,7 @@
String state;
String redirectUri;
int tokenExpiresInSeconds = 60;
String deserializerClassName;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
String deserializerClassName;
@Nullable String deserializerClassName;

...client/src/main/java/org/openhab/core/auth/oauth2client/internal/OAuthClientServiceImpl.java Show resolved Hide resolved
@clinique clinique requested a review from cweitkamp December 22, 2020 12:26
cweitkamp
cweitkamp approved these changes Dec 22, 2020
View reviewed changes
Copy link
Contributor

@cweitkamp cweitkamp left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

Please run 'mvn spotless:apply'. Jenkins is not happy with the formatting.

@clinique
Applying spotless
535fd64 
Signed-off-by: clinique <[email protected]>
@clinique clinique requested a review from cweitkamp December 23, 2020 07:44
@clinique
spotless apply
497b8cf 
Signed-off-by: clinique <[email protected]>
@cweitkamp cweitkamp merged commit f7e0339 into openhab:master Dec 23, 2020
@cweitkamp cweitkamp added the enhancement An enhancement or new feature of the Core label Dec 23, 2020
@cweitkamp cweitkamp added this to the 3.1 milestone Dec 23, 2020
@clinique clinique deleted the AccessTokenResponse_Scope branch December 23, 2020 15:02
@cweitkamp cweitkamp changed the title Adressing issue #1888 [oauth] User-defined scope in AccessTokenResponse Jun 22, 2021
@clinique clinique mentioned this pull request May 14, 2022
Merged
@jlaur jlaur mentioned this pull request Apr 7, 2023
Closed
jlaur added a commit to jlaur/openhab-core that referenced this pull request Apr 10, 2023
@jlaur
Add possibility to inject custom GsonBuilder
e4675a5 
Reverts openhab#1891

Fixes openhab#1888

Signed-off-by: Jacob Laursen <[email protected]>
@jlaur jlaur mentioned this pull request Apr 10, 2023
Merged
J-N-K pushed a commit that referenced this pull request Apr 14, 2023
@jlaur
[oauth] Add support for custom deserialization of AccessTokenResponse (…
eb6b6b9 
…#3537)

* Add possibility to inject custom GsonBuilder

Reverts #1891

Fixes #1888

Signed-off-by: Jacob Laursen <[email protected]>
splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 11, 2023
@clinique @splatch
[OAuth] Added capability for custom deserializer (openhab#1891)
5a342ee 
* Added capability for custom deserializer

Closes openhab#1888

Signed-off-by: clinique <[email protected]>
GitOrigin-RevId: f7e0339
splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 12, 2023
@jlaur @splatch
[oauth] Add support for custom deserialization of AccessTokenResponse (…
b58736a 
…openhab#3537)

* Add possibility to inject custom GsonBuilder

Reverts openhab#1891

Fixes openhab#1888

Signed-off-by: Jacob Laursen <[email protected]>
GitOrigin-RevId: eb6b6b9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cweitkamp cweitkamp cweitkamp approved these changes

Assignees
No one assigned
Labels
enhancement An enhancement or new feature of the Core
Projects
None yet
Milestone
3.1
Development

Successfully merging this pull request may close these issues.
2 participants
@clinique @cweitkamp