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

feat: add JWT support for all the clients #13

Merged
merged 5 commits into from
Jan 10, 2024
Merged

Conversation

yquansah
Copy link
Contributor

@yquansah yquansah commented Jan 9, 2024

This PR mainly has the functionality of support JWT headers provided by the client on a HTTP request. It also refactors some clients to match the exact semantics of the old fern generated clients.

@yquansah yquansah requested a review from markphelps January 9, 2024 19:21
@yquansah yquansah requested a review from GeorgeMac January 9, 2024 19:31
Copy link
Contributor

@markphelps markphelps left a comment

Choose a reason for hiding this comment

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

one minor thing. (rust)

Also, I know we aimed to maintain backward compatibility with the existing rest SDKs, but now is the time to break anything we don't like such as naming conventions. So do you all prefer FliptAPIClient or Flipt or FliptClient, etc @yquansah @GeorgeMac

pub enum AuthScheme {
None,
BearerToken(String),
JWTToken(String),
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
JWTToken(String),
JWT(String),

Otherwise its like JSONWebTokenToken

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@markphelps Yeah That is fine. I am up for standardization as well, I like FliptClient.

Copy link
Contributor

@markphelps markphelps left a comment

Choose a reason for hiding this comment

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

question around visibility for Java constructors. I think we may be able to make them private so that only the builder can instantiate those objects

@@ -7,10 +7,10 @@
public class FliptClient {
private Evaluation evaluation;

public FliptClient(String url, String token, int timeout) {
public FliptClient(String url, String clientToken, String jwtToken, int timeout) {
Copy link
Contributor

Choose a reason for hiding this comment

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

does this constructor need to be public? We want to probably want to require users to user the builder right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@markphelps You are right. I'll make that change right away

private final ObjectMapper objectMapper;

public Evaluation(OkHttpClient httpClient, String baseURL, String token) {
public Evaluation(OkHttpClient httpClient, String baseURL, String clientToken, String jwtToken) {
Copy link
Contributor

Choose a reason for hiding this comment

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

same q

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this would need to stay public in order for the FliptClient to access it, we could make a builder for Evaluation, but I wonder if that makes sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah you are right. Fine to leave it as is

@yquansah yquansah requested a review from markphelps January 9, 2024 23:32
@yquansah yquansah merged commit 13496e2 into main Jan 10, 2024
5 checks passed
@yquansah yquansah deleted the jwt-token-support branch January 10, 2024 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants