Add support of multi oidcIssuer relations per one user's profile#30
Add support of multi oidcIssuer relations per one user's profile#30smalinin wants to merge 9 commits intonodeSolidServer:mainfrom
Conversation
Apply changes from https://github.com/solid/oidc-auth-manager
Merge last changes from solid/oidc-auth-manager
Sync with solid/oidc-auth-manager
|
What's happening with this PR? /cc @justinwb |
RubenVerborgh
left a comment
RubenVerborgh
left a comment
There was a problem hiding this comment.
Agree with the added functionality but disagree with the implementation.
The "discover all" functionality is not what we need. We simply want to validate whether a given party is allowed to act as an issuer for a given user. So rather than a discovery method, we need a validation method. At no point do we need a list of all possible providers.
| // drop the path (provider origin only) | ||
| if (providerUri) { | ||
| providerUri = (new URL(providerUri)).origin | ||
| if (Array.isArray(providerUri)) { |
There was a problem hiding this comment.
This is not good; should always be an array.
There was a problem hiding this comment.
Could force it into an array with [].concat(providerUri) (if there are possible cases where it's not - which I guess shouldn't be in this case; nevertheless, it's a nice "hack" to know of)
| * provider URI was found, reject with an error. | ||
| */ | ||
| function discoverProviderFor (webId) { | ||
| function discoverProviderFor (webId, issuer) { |
There was a problem hiding this comment.
So the method functionality has changed from discovering to validating? We probably need a method like isValidProvider then.
There was a problem hiding this comment.
Ok, I will update the code.
I tried to create fix with a minimal changes in the code.
| } else { | ||
| validateProviderUri(null, webId) // Throw an error if empty or invalid | ||
| } | ||
| } else { |
There was a problem hiding this comment.
The above logic is very messy.
| } | ||
|
|
||
| function discoverFromProfile (webId) { | ||
| function discoverAllFromProfile (webId) { |
There was a problem hiding this comment.
I think this should then also become validateFromProfile and accept a second issuer parameter. Will eliminate most of the messy logic above.
4 participants
No description provided.