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

Pundit authorizations for users #18

Merged

Conversation

yuenmichelle1
Copy link
Collaborator

Continuation of #16 . Currently have the base of this PR be the branch of #16 (for easier PR readability..hopefully).

Authorization on /classifications/users/:id?__EXTRA_QUERY_PARAMS__

Keeping it simple, if we want to query a specific user's classifications, we either have to

  • be the user or
  • be a panoptes admin

Authorization will be different and a tad more complex for the /classifications/user_groups/:id callout which will check the memberships of the querying user and check the queried user_group's stats_visibility.

Currently not using panoptes_application_client on application_controller.rb, will most likely need this later on when we are working with authorizations for user groups. (Once we get to that point, we will need the following secrets:
panoptes_client_id
panoptes_client_secret
). (Just a heads up of what's to come).

@yuenmichelle1 yuenmichelle1 marked this pull request as ready for review July 21, 2023 23:09
@yuenmichelle1 yuenmichelle1 requested a review from zwolf July 21, 2023 23:09
Copy link
Member

@zwolf zwolf left a comment

Choose a reason for hiding this comment

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

Couple changes and clarifications on this one, see below:

app/controllers/application_controller.rb Outdated Show resolved Hide resolved
app/controllers/application_controller.rb Show resolved Hide resolved
spec/policies/queried_user_context_policy_spec.rb Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

What is this context providing context for? It looks like it's just passing an ID around. It can't be a User resource (those don't exist) but does the ID need to be laundered like this? Or does Pundit need a "resource" to apply a Policy to and this is a workaround?

If that's the case, I think you can use a headless policy to pass a symbol representing which policy to use directly from the controller: https://github.com/varvet/pundit#headless-policies

Copy link
Collaborator Author

@yuenmichelle1 yuenmichelle1 Jul 25, 2023

Choose a reason for hiding this comment

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

ah yeah, i was looking into how to send theparams[:id] over to the policy since pundit does not allow sending params to policy. (sending param[:id] to check is current_user['id'] equals params[:id]..i.e. is the user requesting from this endpoint requesting his/her/their own stats) . because pundit needed a "resource" to apply a policy, my thought was to send a wrapper that literally just contained the params[:id]...

looking into headless policies, i guess we could send the params[:id] (i.e. the "queried_user_id" onto the current_user for the policy to read)

change made in following commit. Let me know what you think or if you had something better in mind.

app/policies/application_policy.rb Outdated Show resolved Hide resolved
@yuenmichelle1 yuenmichelle1 marked this pull request as draft July 25, 2023 14:55
@yuenmichelle1 yuenmichelle1 marked this pull request as ready for review July 25, 2023 14:57
@yuenmichelle1
Copy link
Collaborator Author

if you're wondering why i converted to draft and then ready again...something weird happened with GH where the PR just did not pull a recent commit from the corresponding branch....strange..

@yuenmichelle1 yuenmichelle1 requested a review from zwolf July 25, 2023 15:42
@yuenmichelle1 yuenmichelle1 merged commit dc50012 into classification-counts-by-user Jul 25, 2023
1 check passed
yuenmichelle1 added a commit that referenced this pull request Jul 25, 2023
* remove classificationcounts serializer, replaced with event counts serilalizer start query logic for counts by user

* update serializer and only calculate time spent if user requests total request time

* calculating user project contributions

* update validation of param cannot query for top projects and query by project/workflow

* Update user_classification_counts_serializer.rb

* Update schema.rb

* update serializer and rename group_by to group_by_clause to not confuse with ruby's group_by method

* Update user_classification_counts_serializer.rb

* update downcase check to casecmp?

* update rake task for specs and start user_classification_controller spec

* controller specs and update typo in serializer

* hound sniffs on specs

* add count_user_classification query spec

* user_classification count serializer specs

* update serializer specs

* update regex on routes to take care of leading 0, update renaming of top_project_conrtibutions param for easier follow flow/readability

* update show_time_spent boolean to keyword arg

* Pundit authorizations for users  (#18)

* add panoptes_client and require login when querying for /user/<id>

* update faraday to get panoptes-client.rb to work. remove typo on require_login on app controller

* add policy to check if current user(i.e. querying user) can view stats of the queried user

* Update application_controller.rb

* Update user_classification_count_controller.rb

* fix user_classification_count specs to take care of authorizatons

* add spec for unauthorized user

* remove unneeded scopes in app policies

* add specs for policies

* update missing token specs

* adding specs for missing headers/token authenticaton

* update typo

* remove parens from queried_user policy and remove logged_in? method on application_policy

* rename specs for logged in user

* Update README.md to link to repo wiki

* typo on application_controller user client had hard coded env, removing wrapper object on pundit policy in favor of  headless policy, sending param to current_user,

* remove error raising in application policy for cases when we expect user to be nil

* remove spec that checks if error raised if user nil on application policy to allow more open scope
@yuenmichelle1 yuenmichelle1 deleted the pundit-authorizations-for-users- branch July 25, 2023 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants