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.

Sign up for GitHub

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

Jump to bottom

review auth controller code #154

Merged
merged 16 commits into from
Oct 25, 2021
Merged

review auth controller code #154

merged 16 commits into from
Oct 25, 2021

Conversation

SimonLab
Copy link
Member

@SimonLab SimonLab commented Oct 18, 2021 •
edited
Loading

ref: #153
Update mostly the auth controller. I've tried to make it a bit more easy to navigate between functions to make it easier to understand the authentication workflow

@SimonLab SimonLab self-assigned this Oct 18, 2021
@SimonLab
Copy link
Member Author

Github CI fails because the environment variables are not defined:

        AUTH_API_KEY: ${{ secrets.AUTH_API_KEY }}
        ADMIN_EMAIL: [email protected]
        EMAIL_APP_URL: ${{ secrets.EMAIL_APP_URL }}
        ENCRYPTION_KEYS: ${{ secrets.ENCRYPTION_KEYS }}
        GOOGLE_CLIENT_ID: ${{ secrets.GOOGLE_CLIENT_ID }}
        GOOGLE_CLIENT_SECRET: ${{ secrets.GOOGLE_CLIENT_SECRET }}
        SECRET_KEY_BASE: ${{ secrets.SECRET_KEY_BASE }}

I've added as secrets in the Github repository environments but I must misunderstand how to load them in the CI action. Reading the Github doc to see if I missed anything.

@SimonLab SimonLab temporarily deployed to dwylauth October 19, 2021 09:21 Inactive
@SimonLab SimonLab temporarily deployed to dwylauth October 19, 2021 09:39 Inactive
@SimonLab
Copy link
Member Author

After watching https://www.youtube.com/watch?v=5XfgT9A9PHw&ab_channel=MickeyGousset
I can now use the secrets. I needed to specify which environment to use in the Github action:

jobs:
  build:
    name: Build and test
    environment: dwylauth
    runs-on: ubuntu-latest

@codecov
Copy link

codecov bot commented Oct 19, 2021 •
edited
Loading

Codecov Report

Merging #154 (af59862) into main (3a9d687) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #154   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines          545       529   -16     
=========================================
- Hits           545       529   -16
Impacted Files Coverage Δ
lib/auth/apikey.ex 100.00% <100.00%> (ø)
lib/auth_web/controllers/api_controller.ex 100.00% <100.00%> (ø)
lib/auth_web/controllers/auth_controller.ex 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a9d687...af59862. Read the comment docs.

@SimonLab SimonLab temporarily deployed to dwylauth October 19, 2021 09:46 Inactive
@SimonLab SimonLab temporarily deployed to dwylauth October 19, 2021 10:22 Inactive
@SimonLab SimonLab temporarily deployed to dwylauth October 19, 2021 13:24 Inactive
@SimonLab SimonLab temporarily deployed to dwylauth October 19, 2021 13:45 Inactive
@SimonLab SimonLab marked this pull request as ready for review October 19, 2021 13:50
@SimonLab SimonLab assigned nelsonic and unassigned SimonLab Oct 19, 2021
@SimonLab SimonLab added the awaiting-review An issue or pull request that needs to be reviewed label Oct 19, 2021
nelsonic
nelsonic previously approved these changes Oct 20, 2021
View reviewed changes
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.

@SimonLab update looks good. 👍
LMK if you think it's ready to merge. Thanks.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
lib/auth_web/controllers/auth_controller.ex Outdated Show resolved Hide resolved
lib/auth/apikey.ex Show resolved Hide resolved
lib/auth_web/controllers/auth_controller.ex Outdated
@@ -467,14 +445,17 @@ defmodule AuthWeb.AuthController do
end

def verify_email(conn, params) do
id = Auth.Apikey.decode_decrypt(params["id"])
{:ok, id} = Auth.Apikey.decode_decrypt(params["id"])
Copy link
Member

Choose a reason for hiding this comment

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

what happens if this decode_decrypt fails? 💭

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch, I'll add a case to manage the {:errror, _} returned value

Copy link
Member Author

Choose a reason for hiding this comment

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

@nelsonic I've updated the function and added test. PR ready for review again.

@SimonLab SimonLab assigned SimonLab and unassigned nelsonic Oct 21, 2021
@SimonLab SimonLab removed the awaiting-review An issue or pull request that needs to be reviewed label Oct 21, 2021
@SimonLab SimonLab temporarily deployed to dwylauth October 25, 2021 11:16 Inactive
@SimonLab SimonLab assigned nelsonic and unassigned SimonLab Oct 25, 2021
@SimonLab SimonLab added the awaiting-review An issue or pull request that needs to be reviewed label Oct 25, 2021
nelsonic
nelsonic approved these changes Oct 25, 2021
View reviewed changes
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.

Thanks @SimonLab, looks good. 🙌
Thanks for adding clarifying code comments. 👍

If you feel you understand the code and how the project works well enough, do you think you could give improving the README.md a go? ref: #157 💭

@nelsonic nelsonic merged commit 165571d into main Oct 25, 2021
@nelsonic nelsonic deleted the auth-review branch October 25, 2021 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nelsonic nelsonic nelsonic approved these changes

Assignees

@nelsonic nelsonic

Labels
awaiting-review An issue or pull request that needs to be reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@SimonLab @nelsonic