-
Notifications
You must be signed in to change notification settings - Fork 11
chore(console, api): soft and hard regexes for usernames #525
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
chore(console, api): soft and hard regexes for usernames #525
Conversation
packages/utilities/src/utilities.ts
Outdated
export function isForbiddenUsername(username: string): boolean { | ||
return !!username.match(APIFY_ID_REGEX) || !!username.match(FORBIDDEN_REGEXP); | ||
export function isForbiddenUsername(username: string, options: { isAdmin?: boolean, isApifier?: boolean } = { isAdmin: false, isApifier: false }): boolean { | ||
return !!username.match(APIFY_ID_REGEX) || !!username.match(FORBIDDEN_REGEXP) |
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 do we need isApifier
? IMO isAdmin
check is sufficient, as 90% Apifiers are admins anyway - simpler => better
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.
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.
IMHO it would make sense to remove this and rename "Apify test user" to something else (Internal test user?) to keep the same behaviour for test user as for users. It will simplify adding tests for 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.
The option isApifier will be removed and apify test users will be renamed to internal users.
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.
Cool. The fewer special cases the better
packages/utilities/src/utilities.ts
Outdated
export function isForbiddenUsername(username: string): boolean { | ||
return !!username.match(APIFY_ID_REGEX) || !!username.match(FORBIDDEN_REGEXP); | ||
export function isForbiddenUsername(username: string, options: { isAdmin?: boolean, isApifier?: boolean } = { isAdmin: false, isApifier: false }): boolean { | ||
return !!username.match(APIFY_ID_REGEX) || !!username.match(FORBIDDEN_REGEXP) |
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.
IMHO it would make sense to remove this and rename "Apify test user" to something else (Internal test user?) to keep the same behaviour for test user as for users. It will simplify adding tests for this.
packages/consts/src/consts.ts
Outdated
MAX_LENGTH: DNS_SAFE_NAME_MAX_LENGTH, // DNS-safe string length | ||
REGEX: DNS_SAFE_NAME_REGEX, | ||
REGEX: /^(?!.*apify)([a-z0-9]|[a-z0-9][a-z0-9-]*[a-z0-9])$/i, | ||
ADMIN_REGEX: DNS_SAFE_NAME_REGEX, |
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.
@mtrunkat If I add the min and max length in the DNS_SAFE_NAME_REGEX, it might potentially throw weird errors in forms because in some cases, it is only necessary to match the expression but not the length.
Example: https://github.com/apify/apify-core/blob/develop/src/packages/schemas/src/key_value_stores.ts#L9-L15
Should I add the length there?
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.
The advantage of separating DNS_SAFE_NAME_MAX_LENGTH
and DNS_SAFE_NAME_REGEX
is that you get better error messages: Xxx is too long vs. Xxx doesn't match regular expression
So I'd be careful with updating these constants that are used across our code base
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.
BTW ther are 1,700 Actors that contain "apify" in their name - will these keep working ?
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.
Very valid question. I need to investigate what actions are going to be affected by this but from my changes alone, I already know that updateActor (API PUT endpoint) will stop working IMMEDIATELY unless they change the actor name.
The other PR: https://github.com/apify/apify-core/pull/22269/files#diff-4b79096b13a59997f18f26ea5c1fedd7f620bfdd1f5c1268e08d3dfa11362cd2R265
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.
We may give them a pass if the actor names already contains "apify" (they would still need to pass the less strict regex). If it is a new one, they will need to comply with the new regex.
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.
Looking at the other PR, I don't have a very good feeling about this change, it can cause more problems than benefits...
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.
The updateActor will not throw an error if the actor already had "apify" in its name -> https://github.com/apify/apify-core/pull/22269/files#diff-4b79096b13a59997f18f26ea5c1fedd7f620bfdd1f5c1268e08d3dfa11362cd2R263
Prod DB insights:
Actors with apify in the name: 1716
- out of it managed by Apify: 15
- managed by Apify and public: 4
- managed by community: 1701
- managed by community and public: 41
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.
Original REGEX kept. New REGEX - RESTRICTED_REGEX - only applied in places where we want to make the change.
No current Actor which already has the substring "Apify" in their name or title will be affected --> https://github.com/apify/apify-core/pull/22269/files#diff-4b79096b13a59997f18f26ea5c1fedd7f620bfdd1f5c1268e08d3dfa11362cd2R263-R265
Only new actors will be affected and have to comply with the "no apify substring in actor name or title" requirement.
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.
Hmm looking at the numbers, this is a problematic change; we can't enforce this for existing Actors because we disable users from updating them. But if they changed the technical names, the integrations break. So there is no good option for developers to solve this.
Let's discuss this in person? It will be faster.
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.
That's a great point. Honestly I'm not convinced about the whole reasoning behind this change. It makes sense to prohibit use of "apify" in username to avoid confusion, but why in Actor names? We allow "amazon", "google", etc. no problem but ban "apify". But there are legitimate cases for third-party Actors having "apify" in the name, e.g. "apify-airbyte-integration" or "apify-store-scraper".
Altogether, let's just abandon this idea.
packages/utilities/src/utilities.ts
Outdated
export function isForbiddenUsername(username: string): boolean { | ||
return !!username.match(APIFY_ID_REGEX) || !!username.match(FORBIDDEN_REGEXP); | ||
export function isForbiddenUsername(username: string, options: { isAdmin?: boolean, isApifier?: boolean } = { isAdmin: false, isApifier: false }): boolean { | ||
return !!username.match(APIFY_ID_REGEX) || !!username.match(FORBIDDEN_REGEXP) |
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.
The option isApifier will be removed and apify test users will be renamed to internal users.
test/utilities.test.ts
Outdated
expect(utils.isForbiddenUsername('karel.novak')).toBe(false); | ||
|
||
// Regular user vs Admin | ||
expect(utils.isForbiddenUsername('aPifY')).toBe(true); |
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.
Pls let's test cases where Apify is in the middle of the string
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 discussed in #525 (comment), let's abandon prohibiting "apify" in actor names, it solves a problem that doesn't even exist and opens a lot more actual problems.
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.
Great, thanks
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 mentioned on slack. I do not believe that this change should be done here and should instead be done directly in console backend. This is open source package + it is used on the web. There is no reason why people should know that we allow apify
for admins and it would be much cleaner if we just had a condition for apify
directly in the update username method. Which is the only place it is used except for the web.
As for the web, If we keep this as it is, and you merge it, you also need to update the logic there, because on the web it's used to check in routing if the path is a valid username, to redirect users to correct user profile pages.
Here: https://github.com/apify/apify-web/blob/e410bac5e1f2943b37b5d51c43cba2d5ecf2c5bc/src/server/lib/helpers.ts#L93
So this path: https://apify.com/apify
would no longer work
And to fix it on the web you would have to call it with { isAdmin: true }
which is weird.
So I think either you should move this logic (checking apify
) to backend, or rename the argument to be isApifyAllowed
instead of isAdmin
so that it's clearer when used on the web.
Thank you for the insights. The option will be removed and the condition will be moved to backend. |
…actors-for-non-admins
Regular users can no longer use "apify" in their usernames. Admins still can.