Skip to content
This repository has been archived by the owner on Dec 8, 2017. It is now read-only.

Add guest user capability #3

Merged
merged 10 commits into from
Dec 14, 2014
Merged

Add guest user capability #3

merged 10 commits into from
Dec 14, 2014

Conversation

mbland
Copy link
Contributor

@mbland mbland commented Dec 13, 2014

Enables the Hub to generate authentication include files and authenticated email address entries for guest users outside of the team.

The joiner_test.rb Minitest conversion and DummyTestPage extraction are pure refactorings.

Mike Bland added 6 commits December 13, 2014 12:01
This data will be used to generate authentication artifacts for users outside
the team.
This enables users in the _data/private/hub/guest_users.yml list to access the
Hub.

Authenication includes are generated, but do not link to any pages. On top of
being a nice touch, they're necessary to avoid breaking the Server Side
Include directive in _layouts/bare.html, since there's no means of checking
the existence of an included file in SSI itself. We could write a small CGI
process to check this and generate the authentication include on the fly, but
that defeats the purpose of keeping the Hub otherwise all-static.
@@ -6,24 +6,37 @@ class Auth
# true.
# +site+:: Jekyll site object
def self.generate_artifacts(site)
return if site.config['public']
return if site.config['public'] == true
Copy link
Contributor

Choose a reason for hiding this comment

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

Ruby convention is rely on truthiness, unless there's some reason to explicitly check for the boolean value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't know why I did that. Fixed.

@afeld
Copy link
Contributor

afeld commented Dec 14, 2014

I don't really understand how the auth include works, but looks fine to me otherwise! Also, do you think that auth logic would be generally useful enough to move to a standalone gem (at some point)?

@afeld afeld assigned mbland and unassigned afeld Dec 14, 2014
@mbland
Copy link
Contributor Author

mbland commented Dec 14, 2014

Updated the comment for Auth.generate_user_authentication_include() to try to explain a little more clearly how the auth include works. Happy to discuss it further if it's not clear.

Pushing the logic into a gem is an interesting idea. Certainly happy to consider it!

@afeld
Copy link
Contributor

afeld commented Dec 14, 2014

Is there anything new that needs to be added to the deploy instructions? What sets $REMOTE_USER?

Mike Bland added 2 commits December 14, 2014 06:59
After digging deeper into the origin of $REMOTE_USER at Aidan Feldman's
request in #3, I realized that the user's entire email address was
available to nginx's Server Side Include module via the embedded
$http_x_forwarded_email variable, set by ngx_http_core_module:

http://nginx.org/en/docs/http/ngx_http_core_module.html#var_http_

Using this to create each directory under _site/auth seems preferable to
$REMOTE_USER, since having the domain name available as part of the generated
directory name may help to avoid conflicts, should we ever wish to grant
access to users from other domains.
@mbland
Copy link
Contributor Author

mbland commented Dec 14, 2014

I researched the origin of $REMOTE_USER and added those comments to the generate_user_authentication_include() comment. What's more, I realized the availability of $http_x_forwarded_email and replaced $REMOTE_USER with that variable instead, since it helps avoid potential collisions should we ever wish to grant access to someone outside the gsa.gov domain. PTAL.

@mbland
Copy link
Contributor Author

mbland commented Dec 14, 2014

Oh, and no, nothing in the deploy instructions needs to be updated.

afeld added a commit that referenced this pull request Dec 14, 2014
Add guest user capability
@afeld afeld merged commit bc85eb2 into master Dec 14, 2014
@afeld afeld deleted the guest-users branch December 14, 2014 16:14
@mbland mbland mentioned this pull request Jan 7, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants