Skip to content
This repository was archived by the owner on Jun 29, 2024. It is now read-only.

Refactor oauth #317

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Refactor oauth #317

wants to merge 5 commits into from

Conversation

malmers
Copy link
Member

@malmers malmers commented Sep 10, 2017

Use omniauth to fetch data using the oauth 2 client credentials flow. Also request the access token on startup to simplify installation.

mock.post "/users.json", {"Content-Type"=>"application/json", "authorization"=>"Bearer "}, @smurf.to_json, 201, "Location" => "/users/smurf.json"
mock.get "/users/smurf.json", {"Accept"=>"application/json", "authorization"=>"Bearer "}, @smurf.to_json
mock.post "/users.json", {"Content-Type" => "application/json", authorization: "Bearer "}, @smurf.to_json, 201, "Location" => "/users/smurf.json"
mock.get "/users/smurf.json", {"Accept" => "application/json", authorization: "Bearer "}, @smurf.to_json
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to use strings here? can't we use symbols?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could, but the dash in "Content-Type" still needs the quotes. It would look something like this:

{"Content-Type": "application/json", authorization: "Bearer "}

We can do that or change back to using strings as keys. I'm not sure which option is the best, but I can agree that the mixing of styles should be avoided.

Copy link
Contributor

Choose a reason for hiding this comment

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

@malmers But it does look like that? I agree that we shouldn't mix the styles - so make "Authorization" a string too! 😄

@malmers malmers force-pushed the feature/refactor-oauth branch from 0b9f169 to 7a30a49 Compare September 26, 2017 16:06
@lindskogen
Copy link
Contributor

Looks fine, but maybe we should look into not creating a new token every time?

@malmers
Copy link
Member Author

malmers commented Oct 4, 2017

Yes, we should probably upgrade doorkeeper at chalmersit-account-rails before this is merged.

I'm also not that happy with the extra startup time this PR introduces, maybe we should only use automatic token fetching as a fallback if there isn't one provided.

@lindskogen
Copy link
Contributor

Sounds like a good idea!

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.

3 participants