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

Login #15

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Login #15

wants to merge 20 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jul 30, 2018

Authenticates through edge service instead of directly going to authentication service and gets user id from access token for SSE

@ghost ghost self-requested a review July 30, 2018 21:01
MYSQL_DATABASE: 'user_service'

userservice:
build: ~/Documents/ai-testing-org/authentication-service/
Copy link
Contributor

Choose a reason for hiding this comment

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

This should pull from an image

depends_on:
- discoveryservice
- rabbitmq
build: ~/go/src/github.com/AITestingOrg/calculation-service/
Copy link
Contributor

Choose a reason for hiding this comment

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

This should pull from an image

depends_on:
- discoveryservice
- rabbitmq
build: ~/Documents/ai-testing-org/gmaps-adapter/
Copy link
Contributor

Choose a reason for hiding this comment

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

This should pull from an image

@Injectable()
export class AuthenticationService {

edgeServiceUrl = `http://localhost:8080/api/userservice/auth/oauth/token`;
Copy link
Contributor

Choose a reason for hiding this comment

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

The field name reflects just the edge service, however the full path routes to the user service. Can you update the field name to denote this?

Copy link
Author

Choose a reason for hiding this comment

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

Yes good suggestion

},
err => {
alert('Invalid credientals');
console.warn(`Failed to communicated with the user service. Err: ${JSON.stringify(err)}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Those are two unrelated error messages, how can you know if the credentials are invalid if you failed to communicate with the user service?

}

logout() {
localStorage.clear();
Copy link
Contributor

Choose a reason for hiding this comment

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

You are only using the 'accessToken' item, but you are wiping everything from local storage, can logout just delete what it needs to?


const inputElem = JSON.stringify({
'origin': this.pickupInputElementRef.nativeElement.value,
'destination': this.pickupOutputElementRef.nativeElement.value,
'userId': '560c62f4-8612-11e8-adc0-fa7ae01bbebc'
'userId': localStorage.getItem('userId')
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just ask the authentication service for this value via a new function, so we aren't leaking the implementation details?

Copy link
Author

Choose a reason for hiding this comment

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

Yes good suggestion

@jusoto jusoto dismissed dmssargent’s stale review December 7, 2018 04:08

Changes were made and this user is not available for checking review

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