Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update social.coffee to generalize the Login to View system #44

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Update social.coffee to generalize the Login to View system #44

Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
719f93e
Update social.coffee
Bortseb Jul 24, 2024
6b0b771
Update social.coffee
Bortseb Jul 27, 2024
a71ad0b
Update social.coffee
Bortseb Jul 27, 2024
0a087d5
Update social.coffee
Bortseb Jul 27, 2024
5b01de0
Update configuration.md
Bortseb Jul 29, 2024
9652970
Create config-login-to-view.md
Bortseb Jul 29, 2024
e9ab6be
Update config-login-to-view.md
Bortseb Jul 29, 2024
94f729b
Update config-login-to-view.md
Bortseb Jul 29, 2024
ec8e765
switching to authorizing based on id
Bortseb Jul 29, 2024
93a87b1
Update config-login-to-view.md
Bortseb Jul 30, 2024
f10a887
Update config-login-to-view.md
Bortseb Jul 30, 2024
0dfc857
Update config-login-to-view.md
Bortseb Nov 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 21 additions & 16 deletions server/social.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,24 +385,29 @@ module.exports = exports = (log, loga, argv) ->
# see http://ward.asia.wiki.org/login-to-view.html

if argv.restricted?

allowedToView = (req) ->
allowed = []
if argv.allowed_domains?
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
Comment on lines -392 to -398
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was like it for a reason. @WardCunningham are we really sure it is not still needed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

# emails = [ { value: '[email protected]', type: 'account' } ]
emails = req.session?.passport?.user?.google?.emails
return false unless emails
for entry in emails
have = entry.value.split('@')[1]
for want in allowed
return true if want == have
try
allowed_domains = argv.allowed_domains
emails = req.session.passport.user.google.emails
for entry in emails
have = entry.value.split('@')[1]
for want in allowed_domains
return true if want == have
catch error
console.log "argv.allowed_domains exists, but there was an error. Make sure it's value is an array in your config."
if argv.allowed_usernames?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the way this is written if allowed_domains has a value then allowed_usernames will be ignored. not sure that is what is intended.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Bortseb As per my earlier comment above, which is hopefully now visible to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Bortseb Bortseb Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

try
allowed_usernames = argv.allowed_usernames
idProvider = _.head(_.keys(req.session.passport.user))
switch idProvider
when 'github', 'twitter', 'oauth2'
Bortseb marked this conversation as resolved.
Show resolved Hide resolved
username = req.session.passport.user[idProvider].username
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

emails = req.session?.passport?.user?.google?.emails

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll just switch to using id, instead of username then.

return true if (allowed_usernames.length == 1 and allowed_usernames[0] == "*")
for want in allowed_usernames
return true if want == username
catch error
console.log "argv.allowed_usernames exists, but there was an error. Make sure it's value is an array in your config."
false

app.all '*', (req, res, next) ->
Expand Down