Update social.coffee to generalize the Login to View system #44
Conversation
Generalizes the allowedToView function to enable login-to-view with github, twitter, and oauth2.
remove code for old bug that's gone now, and enable allowed_usernames to be "*" to allow any user who can authenticate.
made it so that both argv.allowed_domains and argv.allowed_usernames can exist at the same time, and the system will still work.
slightly changed console log when there is an error processing the config parameters.
|
@Bortseb - See the comments in the code review. It would be really good if you had a written description of what you are trying to achieve with this change. Especially with the possible interaction between allowed usernames and domains. I would also expect to see some documenation on how this feature is configured, with examples. |
server/social.coffee
Outdated
| # accommodate copy bug to be fixed soon | ||
| # https://github.com/fedwiki/wiki/blob/4c6eee69e78c1ba3f3fc8d61f4450f70afb78f10/farm.coffee#L98-L103 | ||
| for k, v of argv.allowed_usernames | ||
| allowed_usernames.push |
There was a problem hiding this comment.
iterating over the parameter, but not pushing anything into allowed_usernames?
There was a problem hiding this comment.
This was a copying mistake, I didn't mean to remove the v being pushed in the example I copied.
However, I'll add it all back in if @WardCunningham says it is still needed. In my more recent commits I removed all of the code relating to the bug that was expected to be fixed soon. Now that all of the code is being done within a try...catch block, if either allowed_domains or allowed_usernames isn't an array, it will be caught and a console.log explains that they should both be arrays in the config. Is it fine to just require that they be arrays, rather than try and account for the case that they aren't?
server/social.coffee
Outdated
| return true if want == have | ||
| catch error | ||
| return false | ||
| if argv.allowed_usernames? |
There was a problem hiding this comment.
the way this is written if allowed_domains has a value then allowed_usernames will be ignored. not sure that is what is intended.
There was a problem hiding this comment.
@Bortseb As per my earlier comment above, which is hopefully now visible to you.
There was a problem hiding this comment.
On my more recent commits, I changed the code so that both allowed_domains and allowed_usernames can exist at the same time and neither will be ignored...
There was a problem hiding this comment.
If allowed_domains exists, then you are either allowed access if domains match, or not - and further tests are ignored. Also, if you are not using google as the provider you don't get access - any allowed_usernames will be ignored.
There was a problem hiding this comment.
Are you referring to my lastest code here? because I changed things to try and account for this comment... I removed the returns in the catch block... So if the allowed_domains section doesn't return true, it should just move on to the next case of allowed_usernames correct?
| if Array.isArray(argv.allowed_domains) | ||
| allowed = argv.allowed_domains | ||
| else | ||
| # accommodate copy bug to be fixed soon | ||
| # https://github.com/fedwiki/wiki/blob/4c6eee69e78c1ba3f3fc8d61f4450f70afb78f10/farm.coffee#L98-L103 | ||
| for k, v of argv.allowed_domains | ||
| allowed.push v |
There was a problem hiding this comment.
This was like it for a reason. @WardCunningham are we really sure it is not still needed?
server/social.coffee
Outdated
| idProvider = _.head(_.keys(req.session.passport.user)) | ||
| switch idProvider | ||
| when 'github', 'twitter', 'oauth2' | ||
| username = req.session.passport.user[idProvider].username |
There was a problem hiding this comment.
Not all auth providers provide a username, and even when they do it is not immutable. The only thing that is immutable, and unique, is the id.
There was a problem hiding this comment.
do you mean providers beyond the 4 that we currently account for in this plugin? (google, twitter, github, oauth2) Other than Google, which is dealt with separately in the allowed_domains case, the other 3 providers all provide a username, which would be unique within their system... (maybe you are worried about cases where a farm sets up multiple ID providers at the same time, and then there would be the potential for username collisions?)
Is it a big issue that the usernames aren't immutable? (if someone changed their username, the config would need to be changed to match) I kind of like that the owner.json files aren't filled with any secret or unknown information in this case (like with some internal ID). Its just each person's public username, so we can do interesting things like composing owner.json files ourselves with known information, rather than only being able to generate them by claiming a site the typical way and having the IDProvider fill in the unknown information like their internal unique ID.
There was a problem hiding this comment.
The single prior use case for login to view which created allowed_domains just happened to use google. There is no reason why this is something that is restricted to Google as an authentication provider.
Authentication is done using the unique ID that the identity provider provides - and is stored in owner.json. All the other values there are simply there as that is the blob of data that the identity provider returns when the wiki is claimed.
There was a problem hiding this comment.
not sure what other providers also provide an email in the token, but the use of Google was kind of hard-coded into the existing code here on line 400.
wiki-security-passportjs/server/social.coffee
Line 400 in 99e2594
There was a problem hiding this comment.
I'll just switch to using id, instead of username then.
ok, I'll add a new page to the existing documentation about how to set up a restricted/login-to-view site with examples. I'll also change my PR description to add more context for what this is all about. |
Add explanation and link to configuring the login-to-view system.
I'll keep editing this more soon, just wanted to see how it showed up on GitHub, and if it would be added to my existing PR.
adding examples
switched to authorizing users to view based on their id rather than username.
better explain how to configure config properties: allowed_domains and allowed_ids
change to more generic example of allowed_domains
Bortseb
changed the title
Generalizes the allowedToView function to enable login-to-view with github, twitter, and oauth2.
Currently wiki's login-to-view system only works for those who want to use Google Auth. The goal of this PR is to enable the login-to-view system for the rest of the identity provider options already available within wiki. (GitHub, Twitter, and generic OAuth2) The existing system with Google relies on email domains, which aren't possible to rely on with the other 3 providers. The other providers do however all require a username, so I have chosen to determine which users are "allowedToView" based on their username being present in a new array that needs to be put into the wiki config called
allowed_usernames, I have also accounted for the case that the farm admin wants to allow all users to view, in that caseallowed_usernamesshould have the value["*"]. This case where the admin wants to allow all users to view makes more sense when they are using a self-hosted idP like keycloak, where its their own users they are allowing, rather than all of github or twitter.EDIT: Rather than using username, I switched to using ID instead.