-
Notifications
You must be signed in to change notification settings - Fork 305
Rename the idp option into multiuser #570
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
Conversation
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 change. a couple minor points, but this is good to go from my end
lib/create-server.js
Outdated
debug.settings('Base URL (--mount): ' + mount) | ||
|
||
if (argv.idp) { | ||
console.error('The idp configuration option has been renamed to multiuser.') |
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.
Nitpick: may as well use console.warn
so as not to print out a stack trace.
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.
Good idea! This was copied from a similar warning elsewhere; will adjust that too.
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.
Done in 9e1b59f.
// auth: 'tls', | ||
// webid: true, | ||
// idp: true, | ||
// multiuser: 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.
This test file is completely commented out...
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 know; I'm not sure about the purpose of this file, I just search & replaced even in the commented code in case it is ever taken up again.
* dz_oidc: (178 commits) Rename the idp option into multiuser (#570) Add a /public folder in new accounts (#569) Update Data Browser html file Remove debug overhead on ACL (#566) Fix requirement for additional verification logging in with WebID-TLS Add bootstrap.min.css.map to common/css/ Add current hash to redirect. (#562) Disable rejectUnauthorized on the WebID-TLS endpoint. (#561) Serve static common/ dir relative to __dirname Tweak account index page phrasing Remove solid:inbox from template Add support for external WebIDs registering with username & password Move RS options to oidc-auth-manager initialization Do not check for user header in oidc test Log whole error in error handler Expand error message for unverified web id Bump oidc-auth-manager dep to 0.12.0 Verify presence of test DNS entries. (#549) Expose WAC-Allow to browser clients. Remove async dependency. ...
Closes #515.
Please let me know whether you prefer
multiuser
ormultiUser
. I chose the former because it just seemed easier.