-
Notifications
You must be signed in to change notification settings - Fork 551
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
refactor(apiv1): accounts api #825
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
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. |
cfe1345
to
0835804
Compare
// 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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment say get account with matching username but you get ID in this function
// DeleteAccount removes record with matching username. | ||
func (db *MySQLDatabase) DeleteAccount(ctx context.Context, id model.DBID) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here again you get model.DBID
that is int.
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`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as i see if conflict happen it can rewrite password.
we have an update account for this. so why you done that this way?
// 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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same problem in comment that say username but get id
// DeleteAccount removes record with matching username. | ||
func (db *PGDatabase) DeleteAccount(ctx context.Context, id model.DBID) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
query, err := tx.PrepareContext(ctx, `INSERT INTO account | ||
(username, password, owner, config) VALUES (?, ?, ?, ?) | ||
ON CONFLICT(username) DO UPDATE SET | ||
password = ?, owner = ? | ||
RETURNING id`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't get conflict part
func (db *SQLiteDatabase) UpdateAccount(ctx context.Context, account model.Account) error { | ||
if account.ID == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a big deal but because you have comment for updateAccount
in other databases please add that comment here too.
// 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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment matching username but you get id
// DeleteAccount removes record with matching username. | ||
func (db *SQLiteDatabase) DeleteAccount(ctx context.Context, id model.DBID) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment should change
|
||
return d.deps.Config.Http.SecretKey, nil | ||
}) | ||
hashedPassword, err := bcrypt.GenerateFromPassword([]byte(account.Password), 10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am thinking cost factor 10 for bcrypt is enough?
should we increase that? what do you think?
func Ptr[t any](a t) *t { | ||
return &a | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice :)
"github.com/go-shiori/shiori/internal/model" | ||
) | ||
|
||
func NewAdminUser(deps *dependencies.Dependencies) (*model.AccountDTO, string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
fetch(new URL(`api/v1/accounts/${account.id}`, document.baseURI), { | ||
method: "patch", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why done this? why not use payload?
@@ -372,9 +355,8 @@ export default { | |||
secondText: "No", | |||
mainClick: () => { | |||
this.dialog.loading = true; | |||
fetch(`api/accounts`, { | |||
fetch(`api/v1/accounts/${account.id}`, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't get advantage of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
AccountsDomain
toAuthDomain
to handle authentication logic in a separate layer.AccountsDomain
which contains logic related to managing Accounts and is the layer that communicates with teh database.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.Account
usages around the code to beAccountDTO
.Database
interface changes:Database.ListAccounts
(formerlyDatabase.GetAccounts
). UsingList*
to better differentiate between the method that return one account and the one that return many.ListAccountOptions
to filter by keyword, username and owner. Also allows retrieving the password field which is disabled by default.DeleteAccount
replacesDeleteAccounts
. Now only deletes one account, iteration logic moved to theAccountsDomain
.SaveAccount
is not calledCreateAccount
AdminRequired
/api/v1/auth/account
to allow updating the entire user data, not only the settings. This should be backwards compatible.model.Ptr
helper that return an object as a pointer of itselfValidationError
error type to return directly from domains and have details available to expose to the API.NewAdminUser
andoptions.WithAuthToken
to avoid boilerplate in tests.Closes #657