Skip to content

Conversation

@thewheat
Copy link
Contributor

@thewheat thewheat commented Oct 2, 2018

Why?

How?

  • Removing references to API keys
  • Change API key to tokens where needed

Copy link

@apassant apassant left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@@ -237,9 +237,6 @@ private HttpURLConnection prepareConnection(HttpURLConnection conn) {

private Map<String, String> createAuthorizationHeaders() {
switch (Intercom.getAuthKeyType()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're removing API_KEY we could also remove this whole switch and just run
headers.put("Authorization", "Basic " + generateAuthString(Intercom.getToken(),""));

We can then remove getAuthKeyType() in Intercom class and get rid of AuthKeyType enum altogether.
@thewheat wdyt?

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 had initial thoughts of keeping it here in case we could have future authentication methods but for simplification, the removal of getAuthKeyType and AuthKeyType makes sense too 👍 I'll get to it 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mmartinic updated 👌

@thewheat thewheat force-pushed the timlim/remove-set-api-key branch from 556e888 to 99fe92c Compare October 16, 2018 08:26
private volatile AuthKeyType authKeyType = AuthKeyType.API_KEY;
private volatile String apiKey;
private volatile String token;
private volatile String appID;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can also remove appId here. It was used in conjunction with apiKey when generating the Auth header. Now we just need the token

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍

@thewheat thewheat force-pushed the timlim/remove-set-api-key branch from 6c0a753 to 751af43 Compare October 23, 2018 07:53
@mmartinic mmartinic merged commit 0ec97e7 into master Oct 24, 2018
@mmartinic mmartinic deleted the timlim/remove-set-api-key branch October 24, 2018 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants