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
Solid-OIDC login #123
Solid-OIDC login #123
Conversation
@@ -5,7 +5,7 @@ REACT_APP_LANG="en" | |||
# This is useful to test a pod provider in a local dev environment | |||
REACT_APP_POD_PROVIDER_DOMAIN_NAME= | |||
|
|||
REACT_APP_BACKEND_DOMAIN_NAME=localhost:3001 | |||
REACT_APP_BACKEND_CLIENT_ID=http://localhost:3001/actors/app |
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.
Isn't the client identifier identifying the frontend application?
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 client_id is where the Client ID document is, and that's on the backend.
On the same URL, you have the as:Application
(which must be backend) and interop:Application
.
But I agree there is a possible confusion. Should we have two clients ID for backend and frontend ? That wouldn't make much sense since it's the same app requesting the same rights. I haven't seen anything on Solid for this case. Maybe something to discuss with them...
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.
I want to add that, for production app, I plan to use the same domain name. Traefik will redirect all /api
and /.well-known
calls to the middleware. It will thus be an hybrid application, much like NextJS API routes.
'oidc:grant_types': ['refresh_token', 'authorization_code'], | ||
'oidc:response_types': ['code'], | ||
'oidc:default_max_age': 3600, | ||
'oidc:require_auth_time': true |
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.
To the list, I would add:
"token_endpoint_auth_method": "none", <- to comply with oidc spec (where default is `client_secret_basic`)
"policy_uri": "",
"contacts": [],
"application_type": "web",
Also, maybe it's better to leave out the prefixes, to improve compatibility with services that doen't speak jsonld? And adding https://www.w3.org/ns/solid/oidc-context.jsonld
explicitly.
See also:
https://solidproject.org/TR/oidc#clientids-document
https://solid-client-identifier-helper.vercel.app/documentation
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.
To the list I would add:
"token_endpoint_auth_method": "none", <- to comply with oidc spec (where default is `client_secret_basic`) "policy_uri": "", "contacts": [], "application_type": "web",
We cannot have empty strings or arrays when we use a triple store for storage.
Adding token_endpoint_auth_method
is a good idea, thanks.
For application_type
, is it really "web" since this is a backend client ID ?
Also, maybe it's better to leave out the prefixes, to improve compatibility with services that doen't speak jsonld? And adding https://www.w3.org/ns/solid/oidc-context.jsonld explicitly.
That's the whole problem of handling contexts correctly, that I haven't tackled yet.
Unfortunately https://www.w3.org/ns/solid/oidc-context.jsonld is a terrible context, with all @id being changed to client_id
and entering into conflict with the AS context. So it cannot be used in the whole code base.
I've looked at the CSS code and, if it doesn't find the https://www.w3.org/ns/solid/oidc-context.jsonld context, it will parse the data as regular RDF and it will work.
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.
We cannot have empty strings or arrays when we use a triple store for storage.
My bad, that was suppoed to be this.settings.oidc.plicy_uri
and so on :)
For application_type, is it really "web" since this is a backend client ID ?
It doesn't have to be but it defaults to web
, if not specified.
That's the whole problem of handling contexts correctly, that I haven't tackled yet.
Unfortunately https://www.w3.org/ns/solid/oidc-context.jsonld is a terrible context, with all @id being changed to client_id and entering into conflict with the AS context. So it cannot be used in the whole code base.
I've looked at the CSS code and, if it doesn't find the https://www.w3.org/ns/solid/oidc-context.jsonld context, it will parse the data as regular RDF and it will work.
Mh, that's annoying. Would it make sense though for client identifier documents, go with both anyways and make use of jsonld context overriding?
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.
Would it make sense though for client identifier documents, go with both anyways and make use of jsonld context overriding?
I don't understand. For information, when fetching the document from the OIDC provider, we use the JsonLdContext
header with https://www.w3.org/ns/solid/oidc-context.jsonld to have nicely formatted data.
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.
For information, when fetching the document from the OIDC provider, we use the JsonLdContext header with https://www.w3.org/ns/solid/oidc-context.jsonld to have nicely formatted data.
Ah ok, I think that was the information missing for me, thanks! :)
How is it formatted, if no JsonLdContext
header is passed though?
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.
It is correct JSON-LD data, but it will not be understood by a software that only reads JSON (as mentionned elsewhere, the Community Solid Server is able to understand JSON-LD no matter the context used).
this.privateJwk = JSON.parse(fs.readFileSync(privateKeyPath)); | ||
this.publicJwk = JSON.parse(fs.readFileSync(publicKeyPath)); | ||
} | ||
}, |
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 the keys stored on the filesystem and not the db?
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.
That's what we do with the similar auth.jwt
service. But indeed it could be saved in the private settings
dataset
return Promise.reject(new E.UnAuthorizedError(E.ERR_NO_TOKEN)); | ||
}, | ||
async proxyConfig() { | ||
const res = await fetch(`${this.settings.baseUrl}.oidc/auth/.well-known/openid-configuration`); |
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 the fetch?
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 node-oidc-provider library use koa internally, and I haven't found a way to proxy directly to this route.
@@ -0,0 +1,133 @@ | |||
const Redis = require('ioredis'); |
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.
What is redis required for?
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 node-oidc-provider library needs to store data. We chose a Redis adapter since it's already in the stack, for cache and queue management.
I will go and merge this PR, and we can continue these discussions here or on issues if this feels important. |
Closes #121
Closes #120
Closes #112
Closes #113