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.

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

Refactor authorization #93

Merged
merged 14 commits into from
Jun 1, 2016
Merged

Refactor authorization #93

merged 14 commits into from
Jun 1, 2016

Conversation

smellsblue
Copy link
Contributor

Refactor authorization. This includes #92 along with a refactoring to isolate authorization in a way to be easily replaceable.

Remove duplication issues from lib/gemstash/cli/setup
Add tests around some of the changed code
…by another authorization mechanism (like LDAP)
@rjocoleman
Copy link
Contributor

@smellsblue would named parameters make sense for the serve methods? In the context of something like Gemstash::SpecBuilder I was thinking of creating a boolean param for prerelease then using the generic serve method.

Gemstash::SpecsBuilder.serve(nil, {}, {prerelease: true}) vs Gemstash::SpecsBuilder.serve(config: {prerelease: true}) or Gemstash::SpecsBuilder.serve(auth_key: "foo")

@smellsblue
Copy link
Contributor Author

@rjocoleman well, I kind of think such options should be wrapped in the object before it gets to the serve method. So in the Gemstash::SpecsBuilder case, I think it might be better if the object passed to authenticated were an instance rather than a class (so you could pass Gemstash::SpecsBuilder.new(prerelease: true)), and then the serve(auth, request, params) method would be an instance method rather than a class method.

My intent for the serve method is that it has all it needs.... but as I think this through more, I think I can drop all the arguments in favor of just the Sinatra app object (well... private gem source that delegates to a Sinatra app).

The ultimate goal of this refactoring was to pave the way for the authorization setup to be replaceable with other authentication/authorization via a plugin, specifically LDAP (which someone had requested in the slack channel). I'm hoping it just has raw access to everything so the plugin could do much more complicated things if necessary.

Anyways, I think I have another commit to do for this now....... thanks for getting me to think a little deeper on this :-)

@rjocoleman
Copy link
Contributor

@rjocoleman
Copy link
Contributor

@smellsblue Are there any further blockers here you need assistance with?

@smellsblue
Copy link
Contributor Author

@rjocoleman I think I'm good with this as is, but I was hoping to get @pcarranza to review it before I merged it in. If you would like to do the reviewing on what I've done, I think I'd be ok merging it in now :-)

@pcarranza
Copy link
Contributor

Sorry, been extremely busy. Will review today.

@pcarranza pcarranza merged commit b08a84b into master Jun 1, 2016
@segiddins segiddins deleted the refactor-authorization branch June 24, 2016 23:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants