kblincoe / VisualGit_SE701_2019_4

1 stars 0 forks source link

Created User Service to manage user information & login status #142

Closed qw closed 5 years ago

qw commented 5 years ago

Resolves #129.

hybrio commented 5 years ago

This might be big enough to be a separate issue but essentially i don't think the authentication service properly signs out...like ever. It creates the client which gets an authentication token from github but that token is never revoked.

if the functionality from octonode shown below were implemented then the authentication service would need to be called as it's the only logical place to put this kind of functionality image

r4inee commented 5 years ago

This might be big enough to be a separate issue but essentially i don't think the authentication service properly signs out...like ever. It creates the client which gets an authentication token from github but that token is never revoked.

if the functionality from octonode shown below were implemented then the authentication service would need to be called as it's the only logical place to put this kind of functionality image

AuthenticationService doesn't log them into GitHub, it just uses the credential details and pings the GET /user endpoint to check if it is valid so no revoke is required as @shurui-li and @gnawf suggested that it should be stateless. It is using the Github authenticated user api in octonode api.

qw commented 5 years ago

This might be big enough to be a separate issue but essentially i don't think the authentication service properly signs out...like ever. It creates the client which gets an authentication token from github but that token is never revoked. if the functionality from octonode shown below were implemented then the authentication service would need to be called as it's the only logical place to put this kind of functionality image

AuthenticationService doesn't log them into GitHub, it just uses the credential details and pings the GET /user endpoint to check if it is valid so no revoke is required as @shurui-li and @gnawf suggested that it should be stateless. It is using the Github authenticated user api in octonode api.

Also is revoke even a thing? user.service.ts gets ride of the client instance that is logged in when the user clicks log out so the object is trashed.

qw commented 5 years ago

@hybrio 2FA related PR

@shurui-li Nice work, but can you also add tests.

As this is a blocking dependency I will create an issue (see #150) for the tests and add them in a separate PR.

henryli333 commented 5 years ago

LGTM; unfortunately someone disregarded our request for code freeze so you have one more merge conflict to resolve :)

hybrio commented 5 years ago

This might be big enough to be a separate issue but essentially i don't think the authentication service properly signs out...like ever. It creates the client which gets an authentication token from github but that token is never revoked. if the functionality from octonode shown below were implemented then the authentication service would need to be called as it's the only logical place to put this kind of functionality image

AuthenticationService doesn't log them into GitHub, it just uses the credential details and pings the GET /user endpoint to check if it is valid so no revoke is required as @shurui-li and @gnawf suggested that it should be stateless. It is using the Github authenticated user api in octonode api.

Also is revoke even a thing? user.service.ts gets ride of the client instance that is logged in when the user clicks log out so the object is trashed.

My mistake i believe when the client simply stores the username and password gets garbage collected so that should be fine. However i think we should still be looking at ways of reusing the same client for any api calls that require authentication due to the way the client saves credentials.

WanniCode commented 5 years ago

also attempted to run the app on this branch, but just like for @NotWowe it just whitescreens

PR-142-app-errors