Skip to content

Adminforth user cleanup warning dialog #223

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

Open
wants to merge 3 commits into
base: next
Choose a base branch
from

Conversation

NoOne7135
Copy link
Contributor

No description provided.

@NoOne7135 NoOne7135 requested a review from ivictbor June 12, 2025 12:20
@@ -176,6 +176,19 @@ export const admin = new AdminForth({
}
},

adminforthUserCleanupWarning: (adminUser: AdminUser) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

@NoOne7135 hm, small misunderstanding here. AF developer should not configure anything to see this error. This error is to protect AF users and AF developers from security breach, so once i will merge this PR I think it will change nothing..
How exactly AF developers will know that such feature exist at all? Do you think they will open common.ts and read for all possible options? I think this is not what you are generally doing when starting using some library of framework. If you will mention it somewhere in doc - also there is no guarantee user reads whole doc. And price for the fact he will not read whole doc should not be a security!
I see point why you make it configurable because string 'adminforth' comes from af developer code, but anyway I think message itself should be fully injected into adminforth code and should not be configurable. For now we can simply assume that user with name adminforth should not exist. Maybe as option we can pass option "auth.warnIfUserEmailIs: 'adminforth'" and make it in app created with cli...

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.

2 participants