refactor(apiv1): accounts api #825

fmartingr · 2024-01-27T08:35:03Z

Refactor Accounts API to API v1
Renamed current AccountsDomain to AuthDomain to handle authentication logic in a separate layer.
Adds AccountsDomain which contains logic related to managing Accounts and is the layer that communicates with teh database.
New AccountDTO object. Account is the exact database representation of the row, AccountDTO is the object we can transfer around in the rest of the code with more fields, helper methods, etc. Is also the object used in creates/updates for the Accounts with included validation.
- Changed the Account usages around the code to be AccountDTO.
Database interface changes:
- Added new Database.ListAccounts (formerly Database.GetAccounts). Using List* to better differentiate between the method that return one account and the one that return many.
  - Accepts new ListAccountOptions to filter by keyword, username and owner. Also allows retrieving the password field which is disabled by default.
- DeleteAccount replaces DeleteAccounts. Now only deletes one account, iteration logic moved to the AccountsDomain.
- SaveAccount is not called CreateAccount
New middleware: AdminRequired
Changed /api/v1/auth/account to allow updating the entire user data, not only the settings. This should be backwards compatible.
Added model.Ptr helper that return an object as a pointer of itself
Added ValidationError error type to return directly from domains and have details available to expose to the API.
Testutil additions: NewAdminUser and options.WithAuthToken to avoid boilerplate in tests.

Closes #657

codecov · 2024-06-03T17:05:04Z

Codecov Report

Attention: Patch coverage is 69.85981% with 129 lines in your changes missing coverage. Please review.

Project coverage is 38.05%. Comparing base (72aecd2) to head (ed7027e).

Files	Patch %	Lines
internal/domains/auth.go	8.10%	34 Missing ⚠️
internal/database/pg.go	70.96%	11 Missing and 7 partials ⚠️
internal/database/sqlite.go	70.68%	11 Missing and 6 partials ⚠️
internal/http/routes/api/v1/accounts.go	79.48%	12 Missing and 4 partials ⚠️
internal/database/mysql.go	72.00%	7 Missing and 7 partials ⚠️
internal/domains/accounts.go	76.27%	8 Missing and 6 partials ⚠️
internal/cmd/root.go	0.00%	6 Missing ⚠️
internal/testutil/accounts.go	66.66%	2 Missing and 2 partials ⚠️
internal/http/routes/api/v1/tags.go	0.00%	2 Missing ⚠️
internal/http/routes/api/v1/api.go	0.00%	1 Missing ⚠️
... and 3 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #825      +/-   ##
==========================================
+ Coverage   34.66%   38.05%   +3.39%     
==========================================
  Files          61       66       +5     
  Lines        4050     4262     +212     
==========================================
+ Hits         1404     1622     +218     
+ Misses       2424     2388      -36     
- Partials      222      252      +30

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Fixes #657

Monirzadeh · 2024-06-11T11:28:56Z

internal/database/mysql.go

 // GetAccount fetch account with matching username.
 // Returns the account and boolean whether it's exist or not.
-func (db *MySQLDatabase) GetAccount(ctx context.Context, username string) (model.Account, bool, error) {
+func (db *MySQLDatabase) GetAccount(ctx context.Context, id model.DBID) (*model.Account, bool, error) {


Comment say get account with matching username but you get ID in this function

Monirzadeh · 2024-06-11T11:32:42Z

internal/database/mysql.go

+// DeleteAccount removes record with matching username.
+func (db *MySQLDatabase) DeleteAccount(ctx context.Context, id model.DBID) error {


same here again you get model.DBID that is int.

Monirzadeh · 2024-06-11T11:47:15Z

internal/database/pg.go

+		query, err := tx.PrepareContext(ctx, `INSERT INTO account
+			(username, password, owner, config) VALUES ($1, $2, $3, $4)
+			ON CONFLICT(username) DO UPDATE SET
+			password = $2,
+			owner = $3
+			RETURNING id`)


as i see if conflict happen it can rewrite password.
we have an update account for this. so why you done that this way?

Monirzadeh · 2024-06-11T11:49:48Z

internal/database/pg.go

 // GetAccount fetch account with matching username.
 // Returns the account and boolean whether it's exist or not.
-func (db *PGDatabase) GetAccount(ctx context.Context, username string) (model.Account, bool, error) {
+func (db *PGDatabase) GetAccount(ctx context.Context, id model.DBID) (*model.Account, bool, error) {


same problem in comment that say username but get id

Monirzadeh · 2024-06-11T11:50:49Z

internal/database/pg.go

+// DeleteAccount removes record with matching username.
+func (db *PGDatabase) DeleteAccount(ctx context.Context, id model.DBID) error {


Monirzadeh · 2024-06-11T11:54:43Z

internal/database/sqlite.go

+		query, err := tx.PrepareContext(ctx, `INSERT INTO account
+			(username, password, owner, config) VALUES (?, ?, ?, ?)
+			ON CONFLICT(username) DO UPDATE SET
+			password = ?, owner = ?
+			RETURNING id`)


i don't get conflict part

Monirzadeh · 2024-06-11T12:01:58Z

internal/database/sqlite.go

+func (db *SQLiteDatabase) UpdateAccount(ctx context.Context, account model.Account) error {
+	if account.ID == 0 {


not a big deal but because you have comment for updateAccount in other databases please add that comment here too.

Monirzadeh · 2024-06-11T12:05:45Z

internal/database/sqlite.go

 // GetAccount fetch account with matching username.
 // Returns the account and boolean whether it's exist or not.
-func (db *SQLiteDatabase) GetAccount(ctx context.Context, username string) (model.Account, bool, error) {
+func (db *SQLiteDatabase) GetAccount(ctx context.Context, id model.DBID) (*model.Account, bool, error) {


comment matching username but you get id

Monirzadeh · 2024-06-11T12:06:32Z

internal/database/sqlite.go

+// DeleteAccount removes record with matching username.
+func (db *SQLiteDatabase) DeleteAccount(ctx context.Context, id model.DBID) error {


comment should change

Monirzadeh · 2024-06-11T12:37:37Z

internal/domains/accounts.go


-		return d.deps.Config.Http.SecretKey, nil
-	})
+	hashedPassword, err := bcrypt.GenerateFromPassword([]byte(account.Password), 10)


i am thinking cost factor 10 for bcrypt is enough?
should we increase that? what do you think?

Monirzadeh · 2024-06-11T14:35:44Z

internal/model/ptr.go

+func Ptr[t any](a t) *t {
+	return &a
+}


Monirzadeh · 2024-06-11T14:39:43Z

internal/testutil/accounts.go

+	"github.com/go-shiori/shiori/internal/model"
+)
+
+func NewAdminUser(deps *dependencies.Dependencies) (*model.AccountDTO, string, error) {


it can be good if add comment to explain what it is exactly do and what is the use case of that for others others.

Monirzadeh · 2024-06-11T14:49:14Z

internal/view/assets/js/page/setting.js

+					fetch(new URL(`api/v1/accounts/${account.id}`, document.baseURI), {
+						method: "patch",


why done this? why not use payload?

Monirzadeh · 2024-06-11T14:49:53Z

internal/view/assets/js/page/setting.js

@@ -372,9 +355,8 @@ export default {
 				secondText: "No",
 				mainClick: () => {
 					this.dialog.loading = true;
-					fetch(`api/accounts`, {
+					fetch(`api/v1/accounts/${account.id}`, {


i don't get advantage of this?

Monirzadeh

Hi @fmartingr Thanks for this PR. you make things easear in long run and i like the way you simplify complex code.
I test this PR in different way and find some bugs in it

when you try to logout you will get interface conversion: interface {} is *model.AccountDTO, not model.Account (500) error
if you create a user as visitor it always create as owner in webui and you can't change that in webui
when you logout the token not remove form browser storage left last user account token in there (maybe because of bug in logout)
i see you change user settings (theme, showId,etc) api in api/v1/auth/account and i thinking with myself can i hack that? and yes i can change my status from visitor to owner just by settings API
you can done that with send this request with a visitor token (the new logic of update setting let me to do that)

{
  "owner": true,
  "config": {
    "CreateEbook": true,
    "HideExcerpt": true,
    "HideThumbnail": true,
    "KeepMetadata": true,
    "ListMode": true,
    "MakePublic": true,
    "NightMode": true,
    "ShowId": true,
    "UseArchive": true
  }
}

you even can change things that should not available for the visitor user

i am thinking about a mechanism that disable token in server side when user logout. so if i logout i can't authorize with that token again (you can still use token even if you done a logout until it expire )
some part of code is not clear to me so i left comment on some of them

I always learn form your code. thanks

list account and create account

463f16f

fmartingr self-assigned this Jan 27, 2024

deleteaccount (wip)

795b39c

fmartingr changed the title ~~refactor: migrate accounts API~~ refactor(apiv1): accounts api to v1 Jun 3, 2024

fmartingr added 4 commits June 3, 2024 16:53

Merge remote-tracking branch 'origin/master' into fmartingr/issue657

14e54a4

remove old accounts code

da169fe

fix from merge

8c02f9d

remove serve method from makefile

e88bfe3

go-shiori deleted a comment from codecov bot Jun 3, 2024

fmartingr added 2 commits June 3, 2024 18:57

ListAccounts, password hash on domain

21d3066

make lint

56f441e

go-shiori deleted a comment from codecov bot Jun 3, 2024

more permissive assertion

0835804

fmartingr force-pushed the fmartingr/issue657 branch from cfe1345 to 0835804 Compare June 3, 2024 17:18

fmartingr marked this pull request as ready for review June 3, 2024 17:53

fmartingr changed the title ~~refactor(apiv1): accounts api to v1~~ refactor(apiv1): accounts api to v1 (WIP) Jun 3, 2024

Merge remote-tracking branch 'origin/master' into fmartingr/issue657

744b2d5

fmartingr marked this pull request as draft June 3, 2024 21:43

fmartingr changed the title ~~refactor(apiv1): accounts api to v1 (WIP)~~ refactor(apiv1): accounts api to v1 Jun 3, 2024

fmartingr changed the title ~~refactor(apiv1): accounts api to v1~~ refactor(apiv1): accounts api Jun 3, 2024

fmartingr added 9 commits June 3, 2024 23:50

rename test

7c09384

update account

fee6cf7

Authorization

4f41104

updated api calls

a397c59

apis, pointers, auth

6ba4722

swagger

e126bee

stylecheck

a90e01a

APIv1: Accounts

eaf10f2

Fixes #657

Merge remote-tracking branch 'origin' into fmartingr/issue657

64f533d

fmartingr added 3 commits June 8, 2024 09:39

domain validation

b79340a

tests

9c48019

swagger

046863c

fmartingr mentioned this pull request Jun 8, 2024

feat: new system info panel in webui settings #926

Merged

fmartingr added 6 commits June 8, 2024 18:07

error handling

3069f00

Merge remote-tracking branch 'origin/master' into fmartingr/issue657

1dcf601

fix system account changes

2ba22f2

Cleanup database interface

7618d4a

test cleanup

4386df7

fixed nil references

ed7027e

fmartingr marked this pull request as ready for review June 8, 2024 16:29

fmartingr requested a review from Monirzadeh June 8, 2024 16:32

Monirzadeh reviewed Jun 11, 2024

View reviewed changes

internal/model/ptr.go

Comment on lines +3 to +5

func Ptr[t any](a t) *t {

return &a

}

Copy link

Collaborator

Monirzadeh Jun 11, 2024

Choose a reason for hiding this comment

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

Nice :)

Monirzadeh reviewed Jun 11, 2024

View reviewed changes

Monirzadeh suggested changes Jun 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(apiv1): accounts api #825

refactor(apiv1): accounts api #825

fmartingr commented Jan 27, 2024 •

edited

Loading

codecov bot commented Jun 3, 2024 •

edited

Loading

Monirzadeh Jun 11, 2024

Monirzadeh Jun 11, 2024

Monirzadeh Jun 11, 2024

Monirzadeh Jun 11, 2024

Monirzadeh Jun 11, 2024

Monirzadeh Jun 11, 2024

Monirzadeh Jun 11, 2024

Monirzadeh Jun 11, 2024

Monirzadeh Jun 11, 2024

Monirzadeh Jun 11, 2024

Monirzadeh Jun 11, 2024

Monirzadeh Jun 11, 2024 •

edited

Loading

Monirzadeh Jun 11, 2024

Monirzadeh Jun 11, 2024

Monirzadeh left a comment •

edited

Loading

		// DeleteAccount removes record with matching username.
		func (db *MySQLDatabase) DeleteAccount(ctx context.Context, id model.DBID) error {

		// DeleteAccount removes record with matching username.
		func (db *PGDatabase) DeleteAccount(ctx context.Context, id model.DBID) error {

		func (db *SQLiteDatabase) UpdateAccount(ctx context.Context, account model.Account) error {
		if account.ID == 0 {

		// DeleteAccount removes record with matching username.
		func (db *SQLiteDatabase) DeleteAccount(ctx context.Context, id model.DBID) error {

		fetch(new URL(`api/v1/accounts/${account.id}`, document.baseURI), {
		method: "patch",

refactor(apiv1): accounts api #825

Are you sure you want to change the base?

refactor(apiv1): accounts api #825

Conversation

fmartingr commented Jan 27, 2024 • edited Loading

codecov bot commented Jun 3, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Monirzadeh Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Monirzadeh left a comment • edited Loading

Choose a reason for hiding this comment

fmartingr commented Jan 27, 2024 •

edited

Loading

codecov bot commented Jun 3, 2024 •

edited

Loading

Monirzadeh Jun 11, 2024 •

edited

Loading

Monirzadeh left a comment •

edited

Loading