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

Authentication #70

Merged
merged 32 commits into from
Mar 3, 2022
Merged

Authentication #70

merged 32 commits into from
Mar 3, 2022

Conversation

fredx30
Copy link
Contributor

@fredx30 fredx30 commented Sep 15, 2021

Although we may consider making this a major change instead of a patch to make it easier to track this inclusion.

Summary

This adds authentication to the frontend. While this change in itself does not cause any breaking change its a fairly big version change. When the backend is set to validate OIDC tokens this branch is required as it allows the sending of OIDC tokens which will bypass the unauthorised issued that occurs otherwise.

  • Adds angular proxy configurations simplifying cors under development. (May cause issues in prod? We need a check here.)
  • Introduce the angular-auth-oidc-client framework to login to azure. Lazy loading this module in with settings specified in a config file may come as a future imporvement but will not be in the scope for this PR.
  • Add the access token to any request going to the wharf-api (see interceptor).
  • Adds a debug page for logins on /login. I think it may be good to keep this into production as im sure we are going to discover new and intresting way to setup the allowed login sources that each will come with intresting setup issues. This page makes it easy to determine if something critical is missing from the login provider setup to make it work with what we have setup thus far.

Introduces pages/routes:

  • /login
  • /unauthorized
  • /forbidden

We should consider implementing some of the following:

  • Insecure warning? (when querying backend on http) - This may be sufficiently handled by the browser.
  • Warning http request fail 401
  • Warning http request fail 403
  • Redirection to /unauthorized?
  • Redirection to /forbidden?
  • Automatic redirection to login on 401 error? (This sort of solves the issues of requireing login for the intro screen)

Motivation

Authentication is a requested feature. See more about the initial plan and requirements set fourth by the RFC 13 - Authentication

src/app/nav/nav.component.ts Outdated Show resolved Hide resolved
src/app/app.component.ts Outdated Show resolved Hide resolved
@fredx30 fredx30 force-pushed the feature/auth branch 7 times, most recently from 94af961 to 369a74b Compare December 9, 2021 18:07
@fredx30 fredx30 changed the title WIP: Authentication Authentication Dec 9, 2021
@fredx30 fredx30 self-assigned this Dec 9, 2021
@fredx30 fredx30 added the enhancement New feature or request label Dec 9, 2021
@fredx30 fredx30 added this to In progress in Backlog via automation Dec 9, 2021
@fredx30 fredx30 marked this pull request as ready for review December 9, 2021 18:50
Copy link
Member

@Alexamakans Alexamakans left a comment

Choose a reason for hiding this comment

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

Checked for big picture stuff.
I'm not very well-versed in angular/OIDC things, but to me the logic and structure looks solid 👍🏻

Not going to approve per-request, will do more thorough review on request :)
Will also check back now and then between doing other things to see if I notice anything 👍🏻

CHANGELOG.md Outdated Show resolved Hide resolved
src/app/auth/auth-config.module.ts Outdated Show resolved Hide resolved
src/app/app-routing.module.ts Show resolved Hide resolved
Backlog automation moved this from In progress to Review in progress Jan 10, 2022
Copy link
Contributor

@applejag applejag left a comment

Choose a reason for hiding this comment

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

I tried out your branch and there's some stuff that isn't working.

Otherwise great work, this is looking very promising!

src/app/auth/wharf-auth.interceptor.ts Show resolved Hide resolved
src/app/shared/config/config.ts Outdated Show resolved Hide resolved
src/assets/config.json Show resolved Hide resolved
src/app/auth/wharf-auth.interceptor.ts Show resolved Hide resolved
src/app/shared/config/config.service.ts Show resolved Hide resolved
src/app/shared/config/config.service.ts Show resolved Hide resolved
angular.json Show resolved Hide resolved
src/app/nav/nav.component.ts Show resolved Hide resolved
src/app/nav/nav.component.html Outdated Show resolved Hide resolved
src/app/shared/config/config.service.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@applejag applejag left a comment

Choose a reason for hiding this comment

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

LGTM! As discussed in meeting, this maybe needs further improvements, such as the error handling and whatnot.

Let's merge it for now and keep improving in future PRs

Copy link
Member

@Alexamakans Alexamakans left a comment

Choose a reason for hiding this comment

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

Great work! 👍🏻

Fredrik Dyrvold and others added 25 commits March 3, 2022 14:11
While working with https://web.dev/cross-origin-resource-sharing/#share-credentials-with-cors type requests
i realised that 'Access-Control-Allow-Origin: *' was not allowed. I have not yet found a great way of handling
these headers with angular. This seems the best way- however it does not seem compatible with our current config.
Based on motivation from #101:

The rfc4648.js author was kind enough to publish an update with the license:
swansontec/rfc4648.js#18

This makes `npm run collect-licenses` work.
@fredx30 fredx30 merged commit 0c150d3 into master Mar 3, 2022
@fredx30 fredx30 deleted the feature/auth branch March 3, 2022 13:22
@applejag applejag mentioned this pull request May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
No open projects
Backlog
Review in progress
Development

Successfully merging this pull request may close these issues.

None yet

3 participants