4
\$\begingroup\$

This is a follow-up to this question Node.js backend login logic. I wrote the following login Angular frontend logic for my Node.js Backend (see the previous question above). Is it any good in terms of security, efficiency, building, async/sync, logging? SECURITY is my main concern. On in a prettier format the question would be:

  • SECURITY: Is my website secure in any way, shape, or form? I'm wondering if I could implement any security measures other than the ones that are built in to the methods provided by Angular. Isn't the transmission of the password in plaintext a security issue? What about XSS and similar troubles? Can't my login just simply be circumvented? That would be a critical mistake.
  • EFFICIENCY: Is how I'm checking usernames and password efficient? Is there any better way to do this?
  • BUILDING: Is how I loaded my website acceptable?
  • ASYNC/SYNC: I know I preform async and sync calls at the same time. Is there any problem to this?
  • LOGGING: I log all connections to the server, and all login attempts. Is this a good practice, or am I overdoing what logging is supposed to accomplish?
  • MISC: Are there any mistakes in the play between the backend and frontend? If I forgot some other important points about the code I would be glad if you mentioned them as well (Source: Login Server with Node.js)

My code:

authentication.service.ts:

import { Injectable } from '@angular/core';
import { HttpClient } from '@angular/common/http';
import { BehaviorSubject, Observable } from 'rxjs';
import { map } from 'rxjs/operators';

import { environment } from '../../environments/environment';
import { User } from '../models/user.model';
import { Router } from '@angular/router';
import { GlobalDataService } from './global-data.service';

@Injectable({ providedIn: 'root' })
export class AuthenticationService {
    constructor(private http: HttpClient,
                private router: Router, public DataService: GlobalDataService) {
        this.currentUserSubject = new BehaviorSubject<User>(JSON.parse(localStorage.getItem('currentUser')));
        this.currentUser = this.currentUserSubject.asObservable();
        this.LoggedIn = true;
    }
    public LoggedIn = true;
    public get currentUserValue(): User {
        return this.currentUserSubject.value;
    }
    private currentUserSubject: BehaviorSubject<User>;
    public currentUser: Observable<User>;
  getRedirectUrl() {
    throw new Error('Method not implemented.');
  }
  isUserLoggedIn() {
    throw new Error('Method not implemented.');
  }

  login(email: string, password: string) {
    return this.http.post<any>(`${environment.apiUrl}/api/login`, { email, password }, {withCredentials: true})
        .pipe(map(user => {
            // login successful if there's a jwt token in the response
            if (user && user.token) {

                // store user details and jwt token in local storage to keep user logged in between page refreshes
                // https://dev.to/rdegges/please-stop-using-local-storage-1i04
                localStorage.setItem('currentUserToken', JSON.stringify(user));
                this.currentUserSubject.next(user);
            }
            // set firstname & email of loggedin user
            this.DataService.loggedinfirstname = user['firstname'];
            this.DataService.loggedinemail = user['eMail'];
            this.redirtoDashboard();
            this.Toolbar();
            this.DataService.prefillSenderData();
            return user;
        }));
  }

  redirtoDashboard() {
      this.router.navigate(['order']);
  }

  Toolbar() {
      this.LoggedIn = !this.LoggedIn;
  }
}

login.component.ts:

import { Component, OnInit } from '@angular/core';
import { ActivatedRoute, Router } from '@angular/router';
import { FormBuilder, FormGroup, Validators } from '@angular/forms';
import { first } from 'rxjs/operators';

import { AuthenticationService } from '../services/authentication.service';

@Component({
  selector: 'app-login',
  templateUrl: './login.component.html',
  styleUrls: ['./login.component.css']
})
export class LoginComponent implements OnInit {

  returnUrl: string;
  loginForm: FormGroup;
  submitted = false;
  error = '';
  loading = false;
  public errorMsg = 'Please login to continue.';
  public redirected: boolean;
  public utm_source: string;

  constructor(private router: Router, private formBuilder: FormBuilder,
              private authenticationService: AuthenticationService, private activatedRoute: ActivatedRoute) {
      if (this.authenticationService.currentUserValue) {
        this.router.navigate(['order']);
    }
      this.activatedRoute.queryParams.subscribe(params => {
      const param = params['utm_source'];

      if (param === 'order' || param === 'work-document' || param === 'profile') {
        this.redirected = true;
        this.utm_source = param;
      } else {
        this.redirected = false;
      }
  });
  }

  ngOnInit(): void {
    this.loginForm = this.formBuilder.group({
      email: ['', [Validators.required, Validators.email]],
      password: ['', [Validators.required, Validators.minLength(6)]]
  });
  }

// convenience getter for easy access to form fields
get f() { return this.loginForm.controls; }

  onSubmit(loginsubmit) {
    this.submitted = true;
    // stop here if form is invalid
    if (this.loginForm.invalid) {
        return console.log('LoginForm Invalid');
    }
    this.loading = true;
    this.authenticationService.login(this.f.email.value, this.f.password.value)
        .pipe(first())
        .subscribe(
            data => {
                if (this.redirected) {
                  this.router.navigate([this.utm_source]);
                } else {
                  this.router.navigate(['order']);
                }

            },
            error => {
                console.log('Login->authservice->err: ', error);
                this.error = error;
                this.loading = false;
            });
}

}

login.component.html:

<div class="container">
  <div class="row">
    <div class="col-sm-9 col-md-7 col-lg-5 mx-auto">
      <div class="card card-signin my-5">
        <div class="card-body">
          <h5 class="card-title text-center">Login</h5>
          <br>
            <form [formGroup]="loginForm" class="form-signin" (ngSubmit)="onSubmit(this.loginForm.value)">
              <div class="form-label-group">
                <input #userName formControlName="email" type="text" id="inputUser" class="form-control" placeholder="E-Mail" required autofocus [ngClass]="{ 'is-invalid': submitted && f.email.errors }">
                  <div *ngIf="submitted && f['email'].errors" class="invalid-feedback">
                    <div *ngIf="f['email'].errors.required">E-Mail is required</div>
                  </div>
                </div>
                <br>
                  <div class="form-label-group">
                    <input #password type="password" formControlName="password" id="inputPassword" class="form-control" placeholder="Password" required [ngClass]="{ 'is-invalid': submitted && f.password.errors }">
                      <div *ngIf="submitted && f['password'].errors" class="invalid-feedback">
                        <div *ngIf="f['password'].errors.required">Password is required</div>
                      </div>
                    </div>
                    <br>
                      <div *ngIf="redirected">
                        <mat-error>
                          <p class="alert alert-danger">
                            {{errorMsg}}
                          </p>
                        </mat-error>
                      </div>
                      <button [disabled]="!loginForm.valid" class="btn btn-dark btn-block" id="loginSubmit" type="submit">Login</button>
                      <div class="forgot-password-link">
                        <a routerLink="/forgot-password">Forgot password</a>
                      </div>
                    </form>
                  </div>
                </div>
              </div>
            </div>
          </div>

\$\endgroup\$
2
  • \$\begingroup\$ In the AuthenticationService class the property LoggedIn is declared with default value true, and also the constructor sets that property to true... it appears to then be set to the negated value of itself in the Toolbar() method, called from the callback to map() within the login method... should it ever explicitly be set to false or is the negation within the Toolbar currently the only way to assign a value of false (other than outside access)? \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Oct 19 '20 at 21:09
  • \$\begingroup\$ @SᴀᴍOnᴇᴌᴀ the variable LoggedIn is used to hide not to be shown elements from the navbar/toolbar using [hidden]. \$\endgroup\$ – Munchkin Oct 20 '20 at 10:51
5
+50
\$\begingroup\$

Sorry I didn't took a look at your backend code, so this is just half a review.

For your questions:

  • Security
    As Sam already analyzed, there is no obvious problem on the frontend part (as long as you run with HTTPS). I expect that the password is hashed (with a salt) in the backend, and only the hash is stored in the database, so that nobody can extract the real passwords from the database.

  • Efficiency
    Yep, Validators is the way to go in Angular. It would be technically a bit more performant to use HTML Validation, but those fractals of milliseconds are definitely not worth to lose the flexibility of Validators.

  • Building

  • Async / Sync
    The problem with calling an async method without handling the result is, that you will not realize if something did not worked as expected. Thats okay if you know that the code that you are using was developed and maintained by a godlike developer who is above errors. If the developer is a human, you should always expect that there may be a problem. And if you call a method and you know that any problem there is NOT a problem for your code, then make it explicit in you code, so that the following developers (e.g. you in 3 months) will know that too. :-)

  • Logging
    The question is, what you intend with the logging. If you would like to monitor everything to get knowledge about your users, your infrastructure, etc etc, than thats fine. Okay, i would then use one of the existing frameworks to do that for me and not reinvent the wheel.
    If you are just interested in the bad things, then i would only log those (like failing login attempts).
    So as always, there is no "YES" or "NO". It depends on your intention.
    As a clarification console.log is not "logging" for me, because its only visible to the user and for him only if he has the console open.

  • MISC
    See the following

Refactoring

I would like to first refactor the code a bit for readability. In my experience it makes it easier to spot mistakes. You can ignore it and skip to the interesting part if you want.

In general i really like to use always privateand public and be as restrictive as possible. It shows the reader that i thought about the scope of a method/variable. And it reduces the chance of misuse. If i am not sure, then i start with private.
If nothing is used, its public by default. And as a reader then i do not know if the developer choose that by intention or just forgot it.

AuthenticationService

LoggedIn is set to true at definition time (public LoggedIn = true) and again in the constructor. I personally prefere the assignment of the initial value at definition time. Also this value is used as "true means not logged in". Thats irritating, therefore i would change the name to isLogedIn and initialize it with false.

DataService is public but seems not be used outside the class.

I don't like constant strings in my code, so i extract them into constants. Like
private loginUrl: string = `${environment.apiUrl}/api/login`;

I like to use "speaking" RxJs Operators. The mapMethod in login does not change the stream, therefor i would use tapinstead. That makes it obvious that there is only a side effect.`Also we then could skip the return line.

I like speedreading code. Therefor, if i can extract some lines of code into a speaking method, i am doing that, because then i only have to read the methodname, not all the code behind it and can decide if i would like to dive deeper or just continue.
Therefor i would change the code in the mapof the login method a bit.

Also it seems that the "Toolbar" method is only used to change the LogedInstatus once. So we could just set the value, without a toggle.
There is also an issue here (see later in the issue chapter), therefor i will move the call of that functionality into the if statement.

Normally the redirect is the last thing that should happen, therefor i move it to the end of the tap

export class AuthenticationService {
    public isLoggedIn = false;
    public currentUser: Observable<User>

    public get currentUserValue(): User {
        return this.currentUserSubject.value;
    }    
    
    private currentUserSubject: BehaviorSubject<User> = new BehaviorSubject<User>(JSON.parse(localStorage.getItem('currentUser')));
    private loginUrl: string = `${environment.apiUrl}/api/login`;

    constructor(private DataService: GlobalDataService,
                private http: HttpClient,
                private router: Router) {
        this.currentUser = this.currentUserSubject.asObservable();
    }
       
  private getRedirectUrl() {
    throw new Error('Method not implemented.');
  }
  
  private isUserLoggedIn() {
    throw new Error('Method not implemented.');
  }

  public login(email: string, password: string):Observable<User> {
    return this.http.post<User>(loginUrl, { email, password }, {withCredentials: true})
        .pipe(
            tap(user => {
            // login successful if there's a jwt token in the response
            if (this.isLoginSuccessful(user)) {
                this.setLogedInUser(user);
                this.isLoggedIn = true;
            }
            this.setDataServiceForUser(user);            
            this.toggleLoginStatus();
            this.redirectToDashboard();
        })
    );
  }

    private isLoginSuccessful(user:User):boolean{
        return user && user.token;
    }

  private redirectToDashboard():void {
      this.router.navigate(['order']);
  }

    private setLogedInUser(user: User):void{
        // store user details and jwt token in local storage to keep user logged in between page refreshes
        // https://dev.to/rdegges/please-stop-using-local-storage-1i04
        localStorage.setItem('currentUserToken', JSON.stringify(user));
        this.currentUserSubject.next(user);
    }

    private setDataServiceForUser(user:User):void{
        // set firstname & email of loggedin user
        this.DataService.loggedinfirstname = user['firstname'];
        this.DataService.loggedinemail = user['eMail'];
        this.DataService.prefillSenderData();
    }
}

LoginComponent

In onSubmityou do not need a first(). Behind this.authenticationService.login is a http request. And those terminate automatically after the first result. Because of the same reason you do not need to unsubscribe those subscriptions.

Issues

AuthenticationService

In the loginmethod it seems that even in the case of a not successful login (no user or no token information), it still tries to send data into the DataService and does stuff. Here i would very clearly separate those things that should ALWAYS happen after a login attempt, and those that only may happen after a successful login.
Specifically `currently it will change LogedIneven if the login was not successful.

LoginComponent

Best Practices

Here some Best Practices (at least in my eyes :-) )

In the code, the User Information is used for two things. For detailed information about the user and secondly as an implicit "the user is logged in". In the AuthentificationService this connection is valid. But to the outside, i would provide User-Information and additionally a "isLogedIn" information. In that way, a developer don't have to "know" that userinformation implies that a user is logged in.

It's a good habit to unsubscribe, when you leave the component. Therefor I normally do something like this

private subscriptions: Subscription() = new Subscription();
...
this.subscriptions.add(
    sourceA.subscribe(...)
)
this.subscriptions.add(
    sourceB.subscribe(...)
)

ngOnDestroy(){
    this.subscriptions.unsubcribe();
}

That way, as soon as the component gets destroyed, those subscriptions all get automatically unsubscribed. Be aware, a component gets only destroyed if its completely removed from the DOM. If it's hidden, then it's still alive.

I hope one or two things were helpful for you.

warm regards

\$\endgroup\$
4
  • \$\begingroup\$ No need to apologize, because the backend code was already reviewed by other SE members :-) You seem to have to put together the most descriptive answer so you will get a bounty unless somehow someone magically manages to outperform your code analysis \$\endgroup\$ – Munchkin Oct 21 '20 at 13:07
  • \$\begingroup\$ loginUrl requires this.loginUrl AFAIK. Also, I tried commenting the localStorage logic out (since it's not the best practice) and I'm not sure whether I did the right thing. Also, if I add public login(email: string, password: string):User I get the error Type 'Observable<any>' is missing the following properties from type 'User': _id, loginId, lastname, firstname, and 2 more. I as well get the error Type 'string' is not assignable to type 'boolean'. at the function private isLoginSuccessful(user:User):boolean{ return user && user.token; }. \$\endgroup\$ – Munchkin Nov 25 '20 at 9:38
  • \$\begingroup\$ also my linter complains about the string literals, even though you and Sam recommended me to use those... \$\endgroup\$ – Munchkin Nov 25 '20 at 10:13
  • \$\begingroup\$ In my example the return type of the methods was set (''public login(....):User {...}''). Now it is checked if the value you return fits to the type. In the login it expects "User" but delivers an "Observable<any>" from the post. My Fault. Make the return type "Observable<User>" and the POST call "return this.http.post<User>( ...)" I changed the code accordingly \$\endgroup\$ – JanRecker Nov 26 '20 at 6:55
2
\$\begingroup\$

Preface

I used AngularJS a few years ago but didn't get into Angular2+ so my knowledge of it is slim-to-none. I do however have a fair amount of familiar with Javascript and various frameworks.

Question responses

Security Isn't the transmission of the password in plaintext a security issue?

I found posts about this question on multiple SE sites. For example, I found Is it ok to send plain-text password over HTTPS? [duplicate]. To quote the accepted answer by Buffalo5ix:

It is standard practice to send "plaintext" passwords over HTTPS. The passwords are ultimately not plaintext, since the client-server communication is encrypted as per TLS.

That question is marked as a duplicate of two other posts, including this one: I just send username and password over https. Is this ok?. It has two answers and the second answer by Steve offers an option:

One additional thing you could do would be to use client certificates. The server can only guarantee to itself that there is no MitM by requiring a client cert. Otherwise, he has to trust the client to properly validate the absence of a MitM. This is more than a lot of services should be willing to trust.

I haven't heard of anyone doing that but perhaps it is done and we just don't know about it.

There is even a Stack Overflow question about the question, with the accepted answer very similar to the accepted answer of the first question (from Security SE) mentioned above.

EFFICIENCY Is how I'm checking usernames and password efficient? Is there any better way to do this?

I am not aware of any better way to do this, but I do notice these lines in AuthenticationService.login() that should be able to use dot notation:

 this.DataService.loggedinfirstname = user['firstname'];
 this.DataService.loggedinemail = user['eMail'];

Method name

I asked about the updating of the property LoggedIn and noted that it was set in the constructor and then modified in the method Toolbar. A name like Toolbar seems like it might be associated with fetching a toolbar. The other methods in that class have a verb - e.g. login, redirtoDashboard. A more appropriate method name for that method might be ToggleLoggedIn or something along those lines.

Simplifying OR conditions

This line in LoginComponent::constructor():

       if (param === 'order' || param === 'work-document' || param === 'profile') {

could be simplified using Array.prototype.includes() which does a strict comparison1 2:

  if ([ 'order', 'work-document', 'profile'].includes(param)) {
\$\endgroup\$
4
  • \$\begingroup\$ If noone else gives a deeper analysis I will hand you the bounty tomorrow so it doesn't go to waste.. \$\endgroup\$ – Munchkin Oct 21 '20 at 9:54
  • \$\begingroup\$ P.S: does includes(param) do the same thing as a == or a ===? \$\endgroup\$ – Munchkin Oct 21 '20 at 9:56
  • \$\begingroup\$ Perhaps you saw my edit but I forgot to add a comment- yes the includes method would do a strict comparison. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Nov 3 '20 at 12:57
  • \$\begingroup\$ I see, thanks for the comment and answer. I was unaware of this includes. \$\endgroup\$ – Munchkin Nov 3 '20 at 13:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.