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

[PR] Add auth #12

Merged
merged 16 commits into from
Nov 28, 2022
Merged

[PR] Add auth #12

merged 16 commits into from
Nov 28, 2022

Conversation

LuchoTurtle
Copy link
Member

closes #1

This PR adds authentication using https://github.com/dwyl/auth_plug.
Instead of usernames being randomly generated and assigned, when a user logs in, his username is shown.

@LuchoTurtle LuchoTurtle added documentation Improvements or additions to documentation enhancement New feature or enhancement of existing functionality good first issue Good for newcomers in-progress An issue or pull request that is being worked on by the assigned person labels Nov 22, 2022
@LuchoTurtle LuchoTurtle self-assigned this Nov 22, 2022
@LuchoTurtle
Copy link
Member Author

After logging in, I can't seem to get access to the socket assigns pertaining to the logged-in user.
No changes to the assigns seem to occur after logging in, contrary to the https://github.com/dwyl/auth_plug docs.

Will further look into this tomorrow, during/after standup.

Copy link
Contributor

@Arhell Arhell left a comment

Choose a reason for hiding this comment

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

@LuchoTurtle
Copy link
Member Author

Gonna have a look at this https://github.com/dwyl/mvp/blob/main/BUILDIT.md#62-create-auth-controller
this should fix whatever problems I'm having.

@LuchoTurtle
Copy link
Member Author

@nelsonic the auth tests are failing because there's no AUTH_API_KEY.
Is there an org one I should use?

The PR is ready and the auth works properly now.

@LuchoTurtle LuchoTurtle assigned nelsonic and unassigned LuchoTurtle Nov 23, 2022
@LuchoTurtle LuchoTurtle added awaiting-review An issue or pull request that needs to be reviewed and removed in-progress An issue or pull request that is being worked on by the assigned person labels Nov 23, 2022
@nelsonic
Copy link
Member

We don't have any org-wide AUTH_API_KEY that would be a security anti-pattern.
[don't worry if you didn't know... but it would be like using the same internet password on all websites ...]
We create a new "App" for each instance of auth_plug with a descriptive name
and then have an AUTH_API_KEY that can be easily reset/revoked for that App.
See: #1 (comment)

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
@nelsonic nelsonic temporarily deployed to dwylauth November 23, 2022 22:43 Inactive
@codecov
Copy link

codecov bot commented Nov 23, 2022

Codecov Report

Merging #12 (affabd9) into main (fa70062) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main       #12   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         3    +1     
  Lines           22        35   +13     
=========================================
+ Hits            22        35   +13     
Impacted Files Coverage Δ
...ib/live_cursors_web/controllers/auth_controller.ex 100.00% <100.00%> (ø)
lib/live_cursors_web/live/cursors.ex 100.00% <100.00%> (ø)
lib/live_cursors_web/router.ex 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

README.md Show resolved Hide resolved
@nelsonic nelsonic added the BLOCKED :fire: Core team's HIGHEST priority, blocking critical work label Nov 28, 2022
@nelsonic
Copy link
Member

Pending review/merge of: dwyl/auth_plug#88

@nelsonic nelsonic assigned LuchoTurtle and unassigned nelsonic Nov 28, 2022
@LuchoTurtle LuchoTurtle temporarily deployed to dwylauth November 28, 2022 13:26 Inactive
@LuchoTurtle
Copy link
Member Author

LuchoTurtle commented Nov 28, 2022

Made the changes to use AuthPlug.assign_jwt_to_socket/3, as discussed. Also formatted the files, in case CI complains about it.

Should be good to go.

@LuchoTurtle LuchoTurtle removed the BLOCKED :fire: Core team's HIGHEST priority, blocking critical work label Nov 28, 2022
@LuchoTurtle LuchoTurtle assigned nelsonic and unassigned LuchoTurtle Nov 28, 2022
@LuchoTurtle LuchoTurtle temporarily deployed to dwylauth November 28, 2022 13:32 Inactive
@nelsonic nelsonic temporarily deployed to dwylauth November 28, 2022 13:36 Inactive
@nelsonic nelsonic temporarily deployed to dwylauth November 28, 2022 13:47 Inactive
Copy link
Member

@nelsonic nelsonic left a comment

Choose a reason for hiding this comment

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

@LuchoTurtle looks great! Thanks. 👌

@nelsonic nelsonic merged commit f51b6e4 into main Nov 28, 2022
@nelsonic nelsonic deleted the add-auth branch November 28, 2022 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review An issue or pull request that needs to be reviewed documentation Improvements or additions to documentation enhancement New feature or enhancement of existing functionality good first issue Good for newcomers
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Write a Tutorial From First Principals using Phoenix LiveView to Sync Cursors on a Canvas/Page
3 participants