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

feat: Add authentication and personal user endpoint #29

Merged
merged 13 commits into from
Jan 20, 2022
Merged

feat: Add authentication and personal user endpoint #29

merged 13 commits into from
Jan 20, 2022

Conversation

kylecarbs
Copy link
Member

@kylecarbs kylecarbs commented Jan 17, 2022

This contribution adds a lot of scaffolding for the database fake
and testability of coderd.

A new endpoint "/user" is added to return the currently authenticated
user to the requester.

Todo:

  • Create generated APIs for TypeScript and Go.

This contribution adds a lot of scaffolding for the database fake
and testability of coderd.

A new endpoint "/user" is added to return the currently authenticated
user to the requester.
@kylecarbs kylecarbs self-assigned this Jan 17, 2022
@codecov
Copy link

codecov bot commented Jan 17, 2022

Codecov Report

Merging #29 (931fd82) into main (36b7b20) will increase coverage by 1.18%.
The diff coverage is 70.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #29      +/-   ##
==========================================
+ Coverage   69.30%   70.49%   +1.18%     
==========================================
  Files          41       48       +7     
  Lines        1642     2159     +517     
  Branches        9        9              
==========================================
+ Hits         1138     1522     +384     
- Misses        402      496      +94     
- Partials      102      141      +39     
Flag Coverage Δ
unittest-go-macos-latest 66.36% <70.89%> (+2.89%) ⬆️
unittest-go-ubuntu-latest 70.43% <70.89%> (+0.99%) ⬆️
unittest-go-windows-latest ?
unittest-js 65.33% <ø> (ø)
Impacted Files Coverage Δ
database/time.go 0.00% <0.00%> (ø)
provisioner/terraform/parse.go 70.73% <ø> (ø)
provisioner/terraform/provision.go 63.26% <ø> (ø)
provisioner/terraform/serve.go 66.66% <ø> (ø)
codersdk/users.go 51.35% <51.35%> (ø)
coderd/users.go 62.14% <62.14%> (ø)
codersdk/client.go 62.29% <62.29%> (ø)
coderd/userpassword/userpassword.go 63.41% <63.41%> (ø)
httpapi/httpapi.go 70.14% <70.14%> (ø)
httpmw/user.go 76.00% <76.00%> (ø)
... and 9 more

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 36b7b20...931fd82. Read the comment docs.

Comment on lines 10 to 11
-- name: GetUserByID :one
SELECT * FROM users WHERE id = $1 LIMIT 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this generated code? If so - we should add it to the .gitattributes file

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope! I'll add a docstring for how this works.

coderd/users_test.go Outdated Show resolved Hide resolved
// Tracks if the API key has properties updated!
changed := false

if key.LoginType == database.LoginTypeOIDC {
Copy link
Contributor

Choose a reason for hiding this comment

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

For our 'black triangle' scenario - are you thinking of supporting Built-In and OIDC? Or just OIDC login?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was just to bring parity to this code from our implementation in v1.

This does support OIDC, but doesn't require it at all. I imagine we'll use basic auth to start, but this makes OIDC easy to add.

Comment on lines +84 to +85
// Makes sure the context properly adds the User!
_ = httpmw.User(r)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe worth validating that the return User id matches what we passed in? It's a simple invariant to check that may catch a regression later

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh good point!

Copy link
Contributor

@bryphe-coder bryphe-coder left a comment

Choose a reason for hiding this comment

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

Overall looks good to me, just some minor nits. Thanks for implementing this, @kylecarbs !

)

// AuthCookie represents the name of the cookie the API key is stored in.
const AuthCookie = "session_token"
Copy link
Contributor

Choose a reason for hiding this comment

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

Will we be able to reuse this from v1? In other words - will session tokens from v1 be usable for logging in via v2?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, they will be! The API key validation is secure in v1, so we reuse a lot of similar logic in the v2 handler.

httpapi/httpapi.go Outdated Show resolved Hide resolved
Message: "👋",
})
})
r.Post("/user", users.createInitialUser)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it expected that the front-end will have to call this? Or we'll just run curl and POST to it in our ./develop.sh, to ensure that it's created?

Copy link
Member Author

Choose a reason for hiding this comment

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

The front-end will call this!

v2 will have no concept of a "setup mode".

Co-authored-by: Bryan <bryan@coder.com>
@kylecarbs kylecarbs enabled auto-merge (squash) January 20, 2022 13:40
@kylecarbs kylecarbs merged commit 6a919ae into main Jan 20, 2022
@kylecarbs kylecarbs deleted the users branch January 20, 2022 13:46
@bryphe-coder bryphe-coder mentioned this pull request Jan 20, 2022
8 tasks
bryphe-coder added a commit that referenced this pull request Jan 21, 2022
This just implements a basic sign-in flow, using the new endpoints in #29 :
![2022-01-20 12 35 30](https://user-images.githubusercontent.com/88213859/150418044-85900d1f-8890-4c60-baae-234342de71fa.gif)

This brings over several dependencies that are necessary:
- `formik`
- `yep`

Ports over some v1 code to bootstrap it:
- `FormTextField`
- `PasswordField`
- `CoderIcon`

And implements basic sign-in:
Fixes #37 
Fixes #43

This does not implement it navbar integration (importantly - there is no way to sign out yet, unless you manually delete your `session_token`). I'll do that in the next PR - figured this was big enough to get reviewed.
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.

None yet

2 participants