Connected client to api gateway; auth and checkout service working E2E #49

nicolehaugen · 2017-05-12T17:52:16Z

This PR adds functionality for the following work items:

1.) #25 - API Gateway service: Invoked endpoints, ensure user is authenticated, and provide app entry point
Mike also started on this work item, but I reworked how auth is handled a bit compared to the PR that he has out for review. The key difference with my implementation is that I have hooked up passport to ensure that the user is authenticated prior to calling into the microservice endpoints - then, if the user is authenticated, I send the user id as request header. You can see this fully hooked up with the checkout microservice that I added.

2.) #47 - Checkout service integration
The api gateway is now hooked up to use the checkout microservice (e.g. the ASP .NET core one) so that the full E2E scenario where orders and feedback entries are created/processed through the client and persisted in Mongo DB. As part of this, I also hooked up the entire client UI to work with the API gateway; however, it's important to note that the sticker\cart functionality is currently implemented to use the "dummy" data access layer. This is only temporary until Charles integrates his microservices.

3.) #6 - Add client auth view
I have added a log in\out link that displays as part of the header view. When the user is unauthenticated, they will see a link to login and a message that says "Welcome, Guest!". When the user is authenticated, they see a link to logout and a message that says "Welcome, !". Also, the app lets you navigate to the browse sticker view without being authenticated. The user isn't required to log in until they attempt to click the Print My Stickers button - at this point, if they aren't logged in, they will be forced to. Also, the email\name fields that are located above the Print My Stickers button default to the name and address of the authenticated user. I implemented all of this behavior so that it "lights up" if you are using the new API Gateway. When you use the existing "monolith" server, the user won't be forced to authenticate and they won't see the options to log in or log out.

One final thing to note is that we still need to revist how the cart is created when the user is NOT authenticated. Today, regardless if you are authenticated or not, you get a cart created that uses a temporary "token". We should modify this behavior so that when the user is authenticated, the cart uses the user id instead of the temporary token. I'll log an issue for this so that we address it after Charles checks in his services.

chlowell

This appears to lack required files added in other commits, e.g. checkoutService/Database/MongoDbService.cs from 52e0793.

chlowell · 2017-05-12T18:14:09Z

apigateway/npm-debug.log

@@ -0,0 +1,26 @@
+0 info it worked if it ends with ok


Stowaway file.

Yep missed this - removing now.

chlowell · 2017-05-12T18:17:50Z

apigateway/package.json

@@ -10,11 +10,20 @@
    "cookie-parser": "~1.4.3",
    "debug": "~2.6.0",
    "express": "~4.14.1",
+    "express-http-proxy": "^1.0.1",


I don't see debug or express-http-proxy used anywhere.

No they're not used...removed them.

chlowell · 2017-05-12T19:30:13Z

apigateway/routes/checkout.js

+router.post('/', function stickerRouteCheckout(req, res) {
+
+    var orderJson = {
+        'Id': guid.raw(),


These properties needn't be strings.

chlowell · 2017-05-12T19:33:42Z

apigateway/services/auth.js

 };

 exports.findUserProfile = findUserProfile;
-function findUserProfile(userId){
+function findUserProfile(userId) {


exports.findUserProfile = userId => { ... } ?

Thanks for catching this - if you see other places where I can simplify my Node.js code like this, definitely let me know...I'm still a bit awkward in writing it. :)

chlowell · 2017-05-12T19:40:34Z

apigateway/Dockerfile

+LABEL Name=stickerapp-apigateway Version=0.1.0 
+COPY package.json /tmp/package.json
+RUN cd /tmp && npm install
+RUN mkdir -p /usr/src/app && mv /tmp/node_modules /usr/src


I'm curious: what's the advantage here over installing dependencies directly to /usr/src?

I inadvertently included the docker files - I still have to finish this part of the check in before I have it reviewed. Also, I think Mike actually created this docker file and included it with his PR. Anyway, I will remove this file from this PR - you should add your comment to his.

I just copied the Dockerfile Chris used for the web client. I agree that it seems like an unusual pattern; I'm not sure what the reasoning is.

chlowell · 2017-05-12T20:29:43Z

apigateway/app.js

+
+const app = express();
+
+const PROJECT_ROOT = path.join(__dirname, '..');


This path is incorrect for the given Docker configuration, and awkward in general. I think required files should be below the application in the tree.

Good catch - I'm going to put a TODO comment above this line. I'd like to get all of the changes Mike made for docker in and then adjust this accordingly.

chlowell · 2017-05-12T20:41:35Z

apigateway/services/auth.js

 };

-exports.isUserLoggedIn = function isAuthenticated(req, res, next){
+exports.isUserLoggedIn = function isAuthenticated(req, res, next) {


It confused me to see a function named isAuthenticated immediately call another function named isAuthenticated, and not return a boolean value.

I am going to change all my exported functions to follow this syntax:

exports.verifyUserLoggedIn= (req, res, next) => { ...}

This will removed the extra 'isAuthenticated' method name. Also, notice above that I've changed the method to "verifyUserLoggedIn" - hopefully this solves the confusion....let me know if it doesn't.

chlowell · 2017-05-12T20:46:43Z

apigateway/routes/checkout.js

+
+//Ensures that the user is authenticated prior to calling into this route
+const authService = require('../services/auth');
+router.use(authService.isUserLoggedIn);


This middleware could be moved into app.js, before the routes requiring authentication.

Sure, I had it there originally, and then moved it since not all the services require it...but I do agree that it would be clearer to have it in the app.js...so I'll move that back now. :)

chlowell · 2017-05-12T21:03:01Z

checkoutService/Controllers/HomeController.cs

@@ -0,0 +1,15 @@
+using System;


Do we need this for anything? A health check would be nice, but it should be more elaborate.

We talked about this over IM too...but for everyone's awareness, when we get to point of deployment with kubernetes, I'll need to add specific endpoint for determining whether the service is alive and healthy - right now the Home controller just returns a string. I'll address this later.

nicolehaugen · 2017-05-15T17:14:21Z

We talked about this over IM, but all of the files you mentioned that were missing are already checked into master...so it should all be there. Let me know if you run into issues with this.

lostintangent · 2017-05-16T20:14:22Z

client/src/views/normal-cart-view/normal-cart-view.js

@@ -101,9 +102,13 @@ export default React.createClass({

                    <div className="gs-cartview-normal-rightpane">
                        <div className="gs-cartview-normal-rightpane-label">Name</div>
-                        <input placeholder="required" className="gs-cartview-normal-rightpane-input" name="checkout-name" min="1" />
+                        <input placeholder="required"
+                            value={this.props.userProfile != null && this.props.userProfile.isAuthenticated ? this.props.userProfile.fullName : ''}


I've vote to define name and email vars at the top of the render method, in order to keep the JSX a little more lean. But what you have now is cool.

lostintangent · 2017-05-16T20:15:22Z

client/src/views/header-view/header-view.js

    },

    render() {
        return (
            <div className="gs-header">
                <div className="gs-header-decorator">
                    <a href="/"><img src="/img/Logo.png" /></a>
+                    {this.showAuth()}
+                </div>
+                <div className="gs-header-info">


I removed this because it was a leftover from Glimpse. I don't think we need it, since it takes up a lot of vertical real-estate.

I should mention that I removed all references of Glimpse in the UI as part of this.

lostintangent · 2017-05-16T20:23:38Z

client/src/stores/auth-store.js

+
+    reduce(state, action) {
+        switch (action.actionType) {
+            case AUTH_ACTIONS.AUTH_CHANGED_ACTION : {


This action name doesn't feel specific enough, and we're also not capturing the success or failure of the action to authenticate. Should we have a AUTH_REQUEST, AUTH_SUCESS and AUTH_FAILURE action (and LOGOUT?), as opposed to a single "state changed" action? That would seem to be more idiomatic with how async actions are done.

This will take me a little bit, so I'm going to go ahead and check in now, and then will fix this after we get everything working in master.

mjrousos · 2017-05-16T22:01:10Z

apigateway/routes/checkout.js

+const guid = require('guid');
+
+//This route calls into the ASP.NET Core checkout microservice
+const checkoutServiceUrl = require('../config/services-config').checkoutServiceUrl; //TODO: add env url lookup


As mentioned in the TODO, we should prefer a URL from the environment since that will allow docker-compose files to organize services, as needed.

Yes agree - I am going to address this once Charles gets all the docker stuff checked in.

mjrousos · 2017-05-16T22:02:54Z

apigateway/routes/browse.js

+        tags = req.query.tags.split(',');
+    }
+
+    dataAccess.getStickers(tags, (items) => {


Is this something that will move into its own microservice eventually? I feel like doing data access for stickers from the API gateway muddies our microservice architecture and should be handled by a separate service.

Yes, on my dev branch I've rewritten this to get data from the sticker service.

mjrousos · 2017-05-16T22:09:13Z

checkoutService/Controllers/OrderController.cs

@@ -41,18 +42,20 @@ public OrderController(IMongoDbService database)
        [HttpPost]
        public async Task<IActionResult> Create([FromBody] Order order)
        {
-            if (order == null)
+            StringValues userId = String.Empty;
+            if (order == null || !this.Request.Headers.TryGetValue("stickerUserId", out userId))


As a future improvement, we could remove the need for checking the stickerUserId header in each action by creating custom middleware or an MVC filter to do that check locally in order to make our code more DRY.

mjrousos · 2017-05-16T22:12:20Z

checkoutService/Dockerfile

@@ -1,13 +0,0 @@
-FROM microsoft/dotnet:1.1.0-sdk


Why remove the dockerfile? Are you planning to just add all the docker support at once later?

Yes I was going to do that separately since I hadn't finished it - but, sounds like Charles has done this so my changes aren't needed anyway.

mjrousos · 2017-05-16T22:13:14Z

client/src/containers/feedback-container.js

 import './base.css';

 const FeedbackContainer = React.createClass({
    render() {
        return (
            <div>
-                <HeaderView pageName="cart" cartCount="0"/>
+                {/*<HeaderView pageName="cart" cartCount="0" isAuthenticated={this.state.auth.isAuthenticated} userFriendlyId={this.state.auth.userFriendlyId}/>*/}


Can we get rid of this comment or is it still useful?

Nope it's not needed...removed.

mjrousos · 2017-05-16T22:13:59Z

I added a few nit-picky comments and questions, but otherwise this looks good to me!

Connected client to api gateway; auth and checkout service working E2E

9175e57

chlowell reviewed May 12, 2017

View reviewed changes

Move auth routing; simplified exports; other misc review feedback

5c09948

lostintangent reviewed May 16, 2017

View reviewed changes

mjrousos reviewed May 16, 2017

View reviewed changes

added email\name vars; removed glimpse stuff; other cleanup

fc16ff5

nicolehaugen merged commit c88ad17 into master May 17, 2017


		const app = express();

		const PROJECT_ROOT = path.join(__dirname, '..');

Connected client to api gateway; auth and checkout service working E2E #49

Connected client to api gateway; auth and checkout service working E2E #49

Conversation

nicolehaugen commented May 12, 2017 • edited Loading

chlowell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolehaugen May 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolehaugen May 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolehaugen commented May 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chlowell May 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjrousos commented May 16, 2017

nicolehaugen commented May 12, 2017 •

edited

Loading

nicolehaugen May 15, 2017 •

edited

Loading

nicolehaugen May 15, 2017 •

edited

Loading

chlowell May 16, 2017 •

edited

Loading