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

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 1 commit
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
38 changes: 28 additions & 10 deletions server/social.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,22 +387,40 @@ module.exports = exports = (log, loga, argv) ->
if argv.restricted?

allowedToView = (req) ->
allowed = []
if argv.allowed_domains?
if Array.isArray(argv.allowed_domains)
allowed = argv.allowed_domains
allowed_domains = 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
# 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
allowed_domains.push v
try
# emails = [ { value: '[email protected]', type: 'account' } ]
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
return false
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?

if Array.isArray(argv.allowed_usernames)
allowed_usernames = argv.allowed_usernames
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_usernames
allowed_usernames.push
Copy link
Member

Choose a reason for hiding this comment

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

iterating over the parameter, but not pushing anything into allowed_usernames?

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.

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?

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

for want in allowed_usernames
return true if want == username
catch error
return false
false

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