Skip to content
This repository has been archived by the owner on Oct 5, 2024. It is now read-only.

Add OpenID as authentication mechanism #276

Merged
merged 6 commits into from
Jan 28, 2018
Merged

Add OpenID as authentication mechanism #276

merged 6 commits into from
Jan 28, 2018

Conversation

sklirg
Copy link
Member

@sklirg sklirg commented Jan 26, 2018

This PR does three main things, as explained by each of the commits' messages.

  1. 6a97412 makes app setup happen in async functions, so that it is possible to execute asynchronous calls during setup. This is used for automagically discovering an OpenID provider during setup, which is simpler and more prone to change than supplying the values manually.
  2. 1d10ed9 simply moves logic from a huge mess of a function to a smaller function (but probably just as messy). Also splits the logic of parsing our custom OAuth2 userinfo and the creation of a user. This has the side effect of making it possible to reuse the create-user function for other authentication methods.
  3. 3a6a38b adds and configures OpenID as a new authentication mechanism for the project. Currently there is no client side change, but that can be added at a later stage. The flow is exactly the same as before (click login -> redirect to OW4 OAuth2 OpenID authorization page -> accept -> logged in [notice the change?])

Some new configuration is also required if wanting to use OpenID (either instead of or in addition to OAuth2). This configuration consists of setting the address to an OpenID-capable host, supplying the client id for a valid client on the host and a redirect uri. No need to specify where the authorization, token, refresh and logout endpoints are -- they're fetched automatically. To actually activate the OIDC client it is needed to set SDF_OIDC to true.

💡 Suggestion: Read the diff of each commit on its own, they should have self-explanatory messages and it reduces the scope of the changes.

Resolves #131

@codecov
Copy link

codecov bot commented Jan 26, 2018

Codecov Report

Merging #276 into master will increase coverage by 1.96%.
The diff coverage is 94.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #276      +/-   ##
==========================================
+ Coverage   79.09%   81.06%   +1.96%     
==========================================
  Files          22       24       +2     
  Lines         751      792      +41     
==========================================
+ Hits          594      642      +48     
+ Misses        157      150       -7
Impacted Files Coverage Δ
server/auth/index.js 81.48% <100%> (+6.48%) ⬆️
server/utils/generateTestData.js 100% <100%> (ø) ⬆️
server/auth/user.js 100% <100%> (ø)
server/auth/providers/ow4.js 84.21% <40%> (+7.12%) ⬆️
server/auth/oidc.js 96.66% <96.66%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 469586f...ad56718. Read the comment docs.

@sklirg
Copy link
Member Author

sklirg commented Jan 26, 2018

I tried sooo hard to achieve the target required diff coverage, but considering the fact that we're probably to remove OAuth2 in favour of OpenID Connect, I'd rather not procrastinate more time to testing that. :-P

@sklirg sklirg merged commit 3e70d29 into master Jan 28, 2018
@sklirg sklirg deleted the feat/131-oidc branch January 28, 2018 01:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants