-
Notifications
You must be signed in to change notification settings - Fork 2
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
Feature/secure services #249
base: main
Are you sure you want to change the base?
Changes from 4 commits
c6e10f9
1c911a1
46eecc8
ab3663a
26d14e2
4a99366
7927a88
a0af7c0
d94a7f8
f2b199f
a9009bc
460c873
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
import { parseJWT } from './parseJWT' | ||
|
||
/* | ||
* Functions are based on https://bitbucket.org/geowerkstatt-hamburg/masterportal/src/dev_vue/src/modules/login/js/utilsCookies.js | ||
* and https://bitbucket.org/geowerkstatt-hamburg/masterportal/src/dev_vue/src/modules/login/js/utilsOIDC.js. | ||
*/ | ||
|
||
/** | ||
* Set a cookie value. | ||
* | ||
* @param {string} name of cookie to set. | ||
* @param {string} value to set into a cookie. | ||
*/ | ||
export function setCookie(name, value) { | ||
const date = new Date() | ||
// Cookie is valid for 15 minutes | ||
date.setTime(date.getTime() + 15 * 1000) | ||
document.cookie = `${name}=${ | ||
value || '' | ||
}; expires=${date.toUTCString()}; secure; path=/` | ||
} | ||
|
||
/** | ||
* Gets value of a cookie. | ||
* | ||
* @param {string} name of cookie to retrieve. | ||
* @returns {string} cookie value. | ||
*/ | ||
export function getCookie(name) { | ||
const nameEQ = `${name}=` | ||
const ca = document.cookie.split(';') | ||
|
||
for (let i = 0; i < ca.length; i++) { | ||
let c = ca[i] | ||
|
||
while (c.charAt(0) === ' ') { | ||
c = c.substring(1, c.length) | ||
} | ||
if (c.indexOf(nameEQ) === 0) { | ||
return c.substring(nameEQ.length, c.length) | ||
} | ||
} | ||
return undefined | ||
} | ||
|
||
export function setCookies(token, expiresIn, refreshToken) { | ||
setCookie('token', token) | ||
setCookie('expires_in', expiresIn) | ||
setCookie('refresh_token', refreshToken) | ||
|
||
const account = parseJWT(token) | ||
|
||
setCookie('name', account?.name) | ||
setCookie('email', account?.email) | ||
setCookie('username', account?.preferred_username) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why save this? We seem to just require the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Applications typically save these values, similarly to #249 (comment). We can keep the example simple you like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, please keep the example minimal. It will be a guideline for implementers on how to achieve things, and they may mimic this and later wonder what to do with the variables. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
setCookie('expiry', account?.exp) | ||
dopenguin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
/** | ||
* Delete all cookies with names given in list | ||
* | ||
* @param {String[]} names of cookies to delete | ||
* @return {void} | ||
*/ | ||
export function eraseCookies(names) { | ||
names.forEach((name) => { | ||
document.cookie = `${name}=; Path=/; Expires=Thu, 01 Jan 1970 00:00:01 GMT;` | ||
}) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
import { eraseCookies, getCookie, setCookies } from './cookie' | ||
|
||
const clientId = 'polar' | ||
const scope = 'openid' | ||
|
||
let loggedIn = false | ||
|
||
export async function authenticate(username, password) { | ||
if (loggedIn) { | ||
await reset() | ||
return | ||
} | ||
|
||
const url = '' | ||
|
||
const response = await fetch(url, { | ||
method: 'POST', | ||
headers: { | ||
'Content-Type': 'application/x-www-form-urlencoded;charset=UTF-8', | ||
}, | ||
body: `grant_type=password&client_id=${encodeURIComponent( | ||
clientId | ||
)}&username=${encodeURIComponent(username)}&password=${encodeURIComponent( | ||
password | ||
)}&scope=${encodeURIComponent(scope)}`, | ||
}) | ||
if (response.ok) { | ||
const data = await response.json() | ||
setCookies(data.access_token, data.expires_in, data.refresh_token) | ||
document.getElementById('login-button').textContent = 'Logout' | ||
document.getElementById('username').disabled = true | ||
document.getElementById('password').disabled = true | ||
loggedIn = true | ||
return | ||
} | ||
document.getElementById('error-message').textContent = | ||
'Die angegebene Kombination aus Nutzername und Passwort ist nicht korrekt.' | ||
// TODO: Add UI element to a layer if it can only be used via authentication (lock / unlock) | ||
} | ||
|
||
async function reset() { | ||
await revokeToken(getCookie('token')) | ||
await revokeToken(getCookie('refresh_token')) | ||
eraseCookies([ | ||
'token', | ||
'expires_in', | ||
'refresh_token', | ||
'name', | ||
'username', | ||
'email', | ||
'expiry', | ||
]) | ||
window.location.reload() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this an F5? Doesn't it completely eradicate all of the user's progress? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct. But if a user would log out in an application that includes POLAR, this would be the case in most situation. Or am I on the wrong track? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, the method There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can take another look regarding the grant_type (as in: is it the correct one). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh right, I forgot that it's in the git patch. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The url used ends with |
||
} | ||
|
||
async function revokeToken(token) { | ||
const url = '' | ||
|
||
const response = await fetch(url, { | ||
method: 'POST', | ||
headers: { | ||
'Content-Type': 'application/x-www-form-urlencoded;charset=UTF-8', | ||
}, | ||
body: `grant_type=refresh_token&token=${encodeURIComponent( | ||
token | ||
)}&client_id=${encodeURIComponent(clientId)}`, | ||
}) | ||
if (response.ok) { | ||
return | ||
} | ||
document.getElementById('error-message').textContent = | ||
'Der Nutzer konnte nicht abgemeldet werden.' | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
/** | ||
* Parses jwt token. This function does *not* validate the token. | ||
* Function is based on https://bitbucket.org/geowerkstatt-hamburg/masterportal/src/dev_vue/src/modules/login/js/utilsOIDC.js. | ||
* | ||
* @param {string} token jwt token to be parsed. | ||
* @returns {object} parsed jwt token as object | ||
*/ | ||
export function parseJWT(token) { | ||
try { | ||
if (!token) { | ||
return {} | ||
} | ||
|
||
const base64Url = token.split('.')[1] | ||
if (!base64Url) { | ||
return {} | ||
} | ||
|
||
const base64 = base64Url.replace(/-/g, '+').replace(/_/g, '/') | ||
return JSON.parse( | ||
decodeURIComponent( | ||
window | ||
.atob(base64) | ||
.split('') | ||
.map(function (c) { | ||
return '%' + ('00' + c.charCodeAt(0).toString(16)).slice(-2) | ||
}) | ||
.join('') | ||
) | ||
) | ||
} catch (e) { | ||
// If token is not valid or another error occurs, return an empty object | ||
return {} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
import { authenticate } from './index.js' | ||
|
||
export function validateForm(interceptorUrlRegex) { | ||
const username = document.getElementById('username').value | ||
const password = document.getElementById('password').value | ||
const errorMessage = document.getElementById('error-message') | ||
|
||
if (!username || !password) { | ||
errorMessage.textContent = | ||
'Bitte geben Sie sowohl einen Nutzernamen als auch ein Passwort ein.' | ||
} else { | ||
errorMessage.textContent = '' | ||
authenticate(username, password, interceptorUrlRegex) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
import { getCookie } from '../getCookie' | ||
|
||
/** | ||
* Add an interceptor to fetch to add the token saved in the cookies to the | ||
* request headers. If interceptors for XMLHttpRequest or axios are needed, | ||
* add them here. | ||
* Function is based on functionality from | ||
* https://bitbucket.org/geowerkstatt-hamburg/masterportal/src/dev_vue/src/modules/login/js/utilsAxios.js | ||
* | ||
* @param interceptorUrlRegex - URLs fitting this regular expression have the token added. | ||
*/ | ||
export function addInterceptor(interceptorUrlRegex: string) { | ||
const { fetch: originalFetch } = window | ||
|
||
window.fetch = (resource, originalConfig) => { | ||
let config = originalConfig | ||
|
||
// @ts-expect-error | Has worked like charm so far. If an error occurs if resource is of type RequestInfo, take another look | ||
warm-coolguy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (interceptorUrlRegex && resource?.match(interceptorUrlRegex)) { | ||
config = { | ||
...originalConfig, | ||
// eslint-disable-next-line @typescript-eslint/naming-convention | ||
headers: { Authorization: `Bearer ${getCookie('token')}` }, | ||
} | ||
} | ||
|
||
return originalFetch(resource, config) | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we share the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you have a suggestion on how to add an interceptor here for only polar instead of the whole application? I don't seem to have come across a reasonable solution so far. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My first idea was to identify the call source, and only act if it was from within POLAR. Since The only thing left seems to be the URL itself. We may modify all URLs intended for interception on arrival. Since everything flows through the client creation (either by config or from the services.json), all URLs may run through a check on whether they fit the Ideas on how to mark URLs:
Then, in the interceptor, the flag is removed before the real URL is used with the Auth header. This way all fetch calls of the outlying application to the URL are unmodified, and all of our own calls are modified. That or you don't override all original headers and only add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've adjusted the configuration so that the name of the token is also configurable. Is it fine like this now or do you deem the marking of the URLs necessary as well? |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
/** | ||
* @param name - Name of the cookie. | ||
* @returns Returns the value of a cookie with the given name or undefined. | ||
*/ | ||
export function getCookie(name: string) { | ||
const cookie = document.cookie | ||
.split(';') | ||
.map((c) => c.trim()) | ||
.find((c) => c.startsWith(name)) | ||
return cookie ? cookie.substring(name.length + 1) : undefined | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we using cookies at all? There seems to be no technical reason to do it, but now we have to worry about cookie laws. Has there been any communication with the customer regarding this? The last state I remember is that we get the token by configuration and thus, it should be fetchable from the
store.configuration.token
or something like that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The configuration option was something I had in mind initially but scrapped quickly.
There are two options that can solve using the token:
createMap
If we'd get the token from the call to
createMap
, we'd have to handle the situation when a token expires and request a new one as the function call is already done. If we'd be doing that with the configuration, the situation would be similar.By having the token in a cookie, which is usually done by applications that use OIDC tokens for authentication anyway, we can simply request it on the go and not worry about refreshing it on our own.
Do you think an adjustment to the documentation is necessary so that it is clear that
interceptorUrlRegex
should only be used by integrated clients as we currently do not offer a login module?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
createMap
parameter combined with an action provided to update the token for our application when needed should be the way to go here. This way we initially have a token to start working with and delegate its updates to the leading application that may now refresh and keep it however it desires to – forwarding it to the map is a follow-up step.This also spares the leading application to be forced to produce a cookie
token
, which it may not have intended to, or which may also already be in use for another token since that's a very generic word – since the client is supposed to be used in multiple spots eventually, even both may hold true in time.This way we're also keeping data flow to the etablished channels and don't open up a new one.
I'd like it to be used in combination with a config parameter and an obligation to refresh it via action, which is a mechanism that should be documented, but is debated atm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of the token can now be configured, as mentioned in #249 (comment)
Whether the authentication shall be achieved by retrieving a cookie or giving the JWT value via createMap and handling the refresh in POLAR (as well) is now something I am discussing with the relevant person that will integrate POLAR later on.