-
Notifications
You must be signed in to change notification settings - Fork 301
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: Adding helper libraries for organization api and support for bearer token auth and no auth rest calls #751
Conversation
Quality Gate failedFailed conditions |
/// | ||
/// <param name="accessToken">access token for requests</param> | ||
/// <param name="accountSid">account sid to make requests for</param> | ||
/// <param name="region">region to make requests for</param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to accessToken and accountSid here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, removed
/// <param name="region">region to make requests for</param> | ||
/// <param name="httpClient">http client used to make the requests</param> | ||
/// <param name="edge">edge to make requests for</param> | ||
public TwilioBearerTokenRestClient( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comments can be corrected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
corrected
/// <summary> | ||
/// Twilio region to make requests to | ||
/// </summary> | ||
public string Region { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Customer has to set region and Edge for each client, try to check how can we improve, so that customer only need to set Region and Edge only once and it can be re-used for each client.
Note: It should not break exist customer code.
// var expirationTime = jwtToken.Payload.Exp.HasValue | ||
// ? DateTimeOffset.FromUnixTimeSeconds(jwtToken.Payload.Exp.Value) | ||
// : DateTimeOffset.MinValue; | ||
// return expirationTime <= DateTimeOffset.UtcNow; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure that this gives same result, irrespective of system time.
Example: If run on system with IST time zone and if run on different time zone, result should not vary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function implementation is bit tricky, tracked as a different ticket in next sprint
/// Default Twilio Client for bearer token authentication | ||
/// </summary> | ||
public class TwilioOrgsTokenAuthClient | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Name should be consistent 'TwilioOrgsTokenAuthClient'
Either Bearer or Orgs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
named bearer for all classes which is generic for token authentication. This class is named specific to orgs because this fetched the access token passing client id, client secret and grant type.
The classes with name bearer in it can be re used with another client class which fetches token passing some other parameters?
Please suggest if this needs to be renamed to bearer?
src/Twilio/TwilioNoAuth.cs
Outdated
return _restClient; | ||
} | ||
|
||
_restClient = new TwilioNoAuthRestClient(region: _region, edge: _edge) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If customer has set region and edge in TwilioOrgsRestClient, then NoAuthClient call should also go to provided region and edge.
In short, customer won't be initialising TwilioNoAuthRestClient.
Also think, do we need TwilioNoAuthRestClient ?
Check this in java as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we might be able to use no auth client, but even when customer only wants to do a rest call without auth, he/she should pass username and password. is this fine? if yes we can remove the no auth class
src/Twilio/TwilioNoAuth.cs
Outdated
|
||
_restClient = new TwilioNoAuthRestClient(region: _region, edge: _edge) | ||
{ | ||
LogLevel = _logLevel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_logLevel
is not set in this file.
Make sure _logLevel
is fetched from TwilioOrgsTokenAuth
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added function to set log level here, should we make these classes dependent on each other? is that design fine?
var authBytes = request.AccessToken; | ||
httpRequest.Headers.Authorization = new AuthenticationHeaderValue("Bearer", authBytes); | ||
|
||
httpRequest.Headers.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
application/json
this should come from API instead of setting it here application/json
for all APIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the time being we support only application/json should we add support for all types? please suggest
httpRequest.Content = new StringContent(request.Body, Encoding.UTF8, "application/json"); | ||
|
||
else if (Equals(request.ContentType, EnumConstants.ContentTypeEnum.SCIM)) | ||
httpRequest.Content = new StringContent(request.Body, Encoding.UTF8, "application/scim"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check if there is any SCIM type request.
* Adding jwt token expiry check function * Adding jwt token expiry check function * sharing variables between clients * sharing variables between clients
src/Twilio/TwilioNoAuth.cs
Outdated
/// <summary> | ||
/// Default Twilio Client | ||
/// </summary> | ||
public class TwilioNoAuthClient |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed over call, Athira will check if we remove this class, what is the downside as we are not taking input from customer.
src/Twilio/TwilioOrgsTokenAuth.cs
Outdated
private static TwilioBearerTokenRestClient _restClient; | ||
private static string _logLevel; | ||
private static TokenManager _tokenManager; | ||
private static ClientProperties clientProperties; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ClientProperties can be removed
{ | ||
private static string _accessToken; | ||
private static string _region; | ||
private static string _edge; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private static variables can be access outside of class ? If yes, then either these should be only static or private.
src/Twilio/TwilioOrgsTokenAuth.cs
Outdated
public static void Init(string grantType, string clientId, string clientSecret) | ||
{ | ||
validateCredentials(grantType, clientId, clientSecret); | ||
_tokenManager = new OrgsTokenManager(grantType, clientId, clientSecret); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Steps.
- Init
- Call Orgs API
- Init with different client_id and client_secret.
- call Orgs API
It should pick new client_id and client_secret
Quality Gate failedFailed conditions |
Fixes
A short description of what this PR does.
Adding helper libraries for organization api and support for bearer token auth and no auth rest calls
Checklist
If you have questions, please file a support ticket, or create a GitHub Issue in this repository.