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

Basic auth #272

Merged
merged 12 commits into from
Jun 7, 2020
Merged

Basic auth #272

merged 12 commits into from
Jun 7, 2020

Conversation

sibsmc
Copy link
Collaborator

@sibsmc sibsmc commented Apr 11, 2020

Branch basic_auth demonstrates basic implementation of jwt token. Would appreciate feedback about controller for users.

@sibsmc sibsmc requested a review from a team as a code owner April 11, 2020 13:06
@sibsmc
Copy link
Collaborator Author

sibsmc commented Apr 16, 2020

In order to implement authentication I have made changes to the way routes and controllers behave. The main changes are:

  1. New singular resource added called profile which shows the information for the user you are currently logged in as.
  2. Routing has changed so changes to users can only be made via /profile
  3. Route /api_user remains. Users are created via this route, all other actions are views.
  4. Wishes and experiences controllers set_api_user method changed to handle case where user id is provided via /api_user/api_user_id or when called without a user id via /profile.

I would like to know if this is a sensible way to split up actions. Once we can decide on the way we handle authentication we can look at using Devise, Passwordless or Clearence

@sibsmc sibsmc added the backend Issue for backend team label Apr 17, 2020
@sibsmc sibsmc self-assigned this Apr 17, 2020
@sibsmc
Copy link
Collaborator Author

sibsmc commented May 23, 2020

Request merge of pull request, I have overwritten current API version as we don't want an api version without any authentication.

Copy link
Member

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

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

Hello, and thanks for the ping!

On this giant PR, I made a ton of (similar) comments.

Pick them off, figure them out, and deal with them at your own pace - ask for a follow-up if any of them seem weird.

👍 Thanks for building this!

bild

server/app/auth/authenticate_user.rb Outdated Show resolved Hide resolved
server/app/auth/authenticate_user.rb Outdated Show resolved Hide resolved
server/app/auth/authorize_api_request.rb Outdated Show resolved Hide resolved
server/app/auth/authorize_api_request.rb Outdated Show resolved Hide resolved
server/app/controllers/api/v1/api_users_controller.rb Outdated Show resolved Hide resolved
server/spec/requests/api/v1/wishes_spec.rb Outdated Show resolved Hide resolved
server/spec/requests/api/v1/wishes_spec.rb Outdated Show resolved Hide resolved
server/spec/requests/api/v1/wishes_spec.rb Outdated Show resolved Hide resolved
server/spec/requests/api/v1/wishes_spec.rb Outdated Show resolved Hide resolved
server/spec/requests/api/v1/wishes_spec.rb Outdated Show resolved Hide resolved
@sibsmc
Copy link
Collaborator Author

sibsmc commented May 24, 2020

Thanks for reviewing @olleolleolle :-)

@sibsmc
Copy link
Collaborator Author

sibsmc commented May 30, 2020

Hi @olleolleolle thanks for going through and making your suggestions. I have made all the changes advised. Could the request be merged now?

@@ -46,11 +45,15 @@ def wish_params
end

def set_api_user
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need the api_user_id param? I would expect that api user, would be current_user.

If we do need it, i think that logic should be moved to application_controller, so this controller doesn't need to be aware of this.

But I might be missing something

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi Dennis, if you are looking up a user other then the current user than you can pass in the parameter in the url, and that parameter will be used to set the api_user

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you are looking at profile the user will always be set to current_user, but if you are looking at api_user, wishes or experiences the parameter can be supplied, if not supplied it shows information for current user

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dennis is this ok or are changes required?

Copy link
Member

@dennis dennis Jun 7, 2020

Choose a reason for hiding this comment

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

For experiences/wishes, there is always a API User provided in the URL?

resources :api_users, only: [:index, :create, :show] do
resources :wishes, only: [:index, :show]
resources :experiences, only: [:index, :show]
end

Or can you leave it blank in order to use current_user?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi Dennis, you don't access experiences or wishes directly, it is always through a user.
profile/wishes/id_wish - returns wishes of the current user
api_users/id/wishes - returns wishes of user 'id'.
You can only create, change and delete wishes via profile.
The same goes for the experiences.

Copy link
Member

Choose a reason for hiding this comment

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

I have nothing else to add :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks alot for reviewing Dennis

server/spec/auth/authenticate_user_spec.rb Outdated Show resolved Hide resolved
@diwanow diwanow linked an issue Jun 6, 2020 that may be closed by this pull request
@diwanow diwanow added the Theme: AuthN/AuthZ Add authentication and authorization for the api label Jun 6, 2020
@sibsmc sibsmc merged commit 796beac into master Jun 7, 2020
@sibsmc sibsmc deleted the basic_auth branch June 7, 2020 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Issue for backend team Theme: AuthN/AuthZ Add authentication and authorization for the api
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Authentication Validate new user data: password
4 participants