COR-5984: implement authservice #69
Conversation
tungntEmotiv
requested review from
khanhle1993,
sondhemotiv and
thamha-emotiv
| PermissionsDenied = 2, | ||
| WebSocketConnectFailed = 3, | ||
| AccessRightDenied = 4, | ||
| AuthorizationFailed = 5, |
There was a problem hiding this comment.
I would like to hide things related to web socket to simply the API, so can we hide these 3 error codes?
There was a problem hiding this comment.
i think can hide the WebSocketConnectFailed or replace by Unknown Error, for embedded lib, the connection always work , but the AccessRightDenied still need for this version, because cause UI wait the error code to open the eula terms popup, i think i can rename to NoAccessRight for better
There was a problem hiding this comment.
It's no harm if we ignore this step. The app which logins always has access right and we're sure about that inside Cortex.
There was a problem hiding this comment.
I have added some comment for the error code, i think we can keep some errorcode for logic of if else even the error occur rarely
| public enum CortexErrorCode | ||
| { | ||
| OK = 0, | ||
| PermissionsDenied = 2, // Indicates that the requested operation failed because the required permissions were not granted. |
There was a problem hiding this comment.
PermissionsDenied = 1 (not 2)?
| public enum CortexErrorCode | ||
| { | ||
| OK = 0, | ||
| PermissionsDenied = 2, // Indicates that the requested operation failed because the required permissions were not granted. |
There was a problem hiding this comment.
I think this explanation for the error is not clear enough to help developers understand.
| OK = 0, | ||
| PermissionsDenied = 2, // Indicates that the requested operation failed because the required permissions were not granted. | ||
| CortexConnectionError = 3, // Indicates that the connection to Cortex started unsuccessfully. | ||
| NoEULAAccepted = 4, // Indicates that the user has not accepted the End User License Agreement (EULA). Only for Cortex v3. Must be removed at Cortex v5 |
There was a problem hiding this comment.
AuthorizationFailed and a boolean acceptedEula in UserInfo.
| using Emotiv.Cortex.Models; | ||
| using EmotivUnityPlugin; // TODO (Tung Nguyen): remove this line when move all old code to new SDK | ||
|
|
||
| namespace Emotiv.Cortex.Service |
There was a problem hiding this comment.
In C#, namespace and folder structure are mirror, should we move IAuthService.cs and AuthService.cs files out of Auth folder?
| { | ||
| public static class ServiceFactory | ||
| { | ||
| public static IAuthService GetAuthService() |
There was a problem hiding this comment.
I think we should write a method "CreateAuthService()" instead.
public static IAuthService CreateAuthService()
{
return new AuthService();
}
There was a problem hiding this comment.
can we move these old files like Authorizer.cs into folder like __Internals__?
| { | ||
| public class AuthService : IAuthService | ||
| { | ||
| public static AuthService Instance { get; } = new AuthService(); |
There was a problem hiding this comment.
Do we need AuthService to be singleton? I think, as a SDK, we shouldn't decide if this class is a singleton or not, it should be decided by the application.
| { | ||
| OK = 0, | ||
| PermissionsDenied = 2, // Indicates that the requested operation failed because the required permissions were not granted. | ||
| CortexConnectionError = 3, // Indicates that the connection to Cortex started unsuccessfully. |
| PermissionsDenied = 2, // Indicates that the requested operation failed because the required permissions were not granted. | ||
| CortexConnectionError = 3, // Indicates that the connection to Cortex started unsuccessfully. | ||
| NoEULAAccepted = 4, // Indicates that the user has not accepted the End User License Agreement (EULA). Only for Cortex v3. Must be removed at Cortex v5 | ||
| AuthorizationFailed = 5, // Indicates that the authorization process failed overall. |
|
|
||
| namespace Emotiv.Cortex.Service | ||
| { | ||
| public interface IAuthService |
There was a problem hiding this comment.
How about the case when client id / secret is empty or invalid?
2 participants
The PR to implement AuthService include:
Please help to review. Thanks