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.

Sign up for GitHub

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

Jump to bottom

[Feature Request] Allow to disable default password log in after SSO is configured #406 #502

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+37 −14
Open

[Feature Request] Allow to disable default password log in after SSO is configured #406 #502

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6d8f664
[Feature Request] Allow to disable default password log in after SSO …
kamtschatka Oct 6, 2024
721c7ab
[Feature Request] Allow to disable default password log in after SSO …
kamtschatka Oct 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions apps/browser-extension/src/SignInPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,7 @@ export default function SignInPage() {
break;
}
if (loginError) {
if (loginError.data?.code == "UNAUTHORIZED") {
errorMessage = "Wrong username or password";
} else {
errorMessage = loginError.message;
}
errorMessage = loginError.message || "Wrong username or password";
}

return (
Expand Down
12 changes: 11 additions & 1 deletion apps/web/components/signin/CredentialsForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ function SignIn() {
const [signinError, setSigninError] = useState("");
const router = useRouter();
const searchParams = useSearchParams();
const clientConfig = useClientConfig();

const oAuthError = searchParams.get("error");
if (oAuthError && !signinError) {
setSigninError(`${OAUTH_FAILED} ${oAuthError}`);
Expand All @@ -44,6 +46,14 @@ function SignIn() {
resolver: zodResolver(signInSchema),
});

if (clientConfig.auth.disablePasswordAuth) {
return (
kamtschatka marked this conversation as resolved.
Show resolved Hide resolved
<p className="text-center">
Password authentication is currently disabled.
</p>
);
}

return (
<Form {...form}>
<form
Expand Down Expand Up @@ -234,7 +244,7 @@ export default function CredentialsForm() {
</TabsContent>
<TabsContent value="signup">
{clientConfig.auth.disableSignups ||
clientConfig.auth.disablePasswordSignups ? (
clientConfig.auth.disablePasswordAuth ? (
<p className="text-center">Signups are currently disabled.</p>
) : (
<SignUp />
Expand Down
2 changes: 1 addition & 1 deletion apps/web/lib/clientConfig.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ export const ClientConfigCtx = createContext<ClientConfig>({
demoMode: undefined,
auth: {
disableSignups: false,
disablePasswordSignups: false,
disablePasswordAuth: false,
},
inference: {
inferredTagLang: "english",
Expand Down
4 changes: 2 additions & 2 deletions docs/docs/03-configuration.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@ When setting up OAuth, the allowed redirect URLs configured at the provider shou
:::

| Name | Required | Default | Description |
| ------------------------------------------- | -------- | ---------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| ------------------------------------------- | -------- | ---------------------- |---------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| DISABLE_SIGNUPS | No | false | If enabled, no new signups will be allowed and the signup button will be disabled in the UI |
| DISABLE_PASSWORD_SIGNUPS | No | false | If enabled, only signups using OAuth are allowed and the signup button for a local account will be disabled in the UI |
| DISABLE_PASSWORD_AUTH | No | false | If enabled, only signups and logins using OAuth are allowed and the signup button and login form for local accounts will be disabled in the UI |
| OAUTH_WELLKNOWN_URL | No | Not set | The "wellknown Url" for openid-configuration as provided by the OAuth provider |
| OAUTH_CLIENT_SECRET | No | Not set | The "Client Secret" as provided by the OAuth provider |
| OAUTH_CLIENT_ID | No | Not set | The "Client ID" as provided by the OAuth provider |
Expand Down
6 changes: 3 additions & 3 deletions packages/shared/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ const stringBool = (defaultValue: string) =>
const allEnv = z.object({
API_URL: z.string().url().default("http://localhost:3000"),
DISABLE_SIGNUPS: stringBool("false"),
DISABLE_PASSWORD_SIGNUPS: stringBool("false"),
DISABLE_PASSWORD_AUTH: stringBool("false"),
OAUTH_ALLOW_DANGEROUS_EMAIL_ACCOUNT_LINKING: stringBool("false"),
OAUTH_WELLKNOWN_URL: z.string().url().optional(),
OAUTH_CLIENT_SECRET: z.string().optional(),
Expand Down Expand Up @@ -54,7 +54,7 @@ const serverConfigSchema = allEnv.transform((val) => {
apiUrl: val.API_URL,
auth: {
disableSignups: val.DISABLE_SIGNUPS,
disablePasswordSignups: val.DISABLE_PASSWORD_SIGNUPS,
disablePasswordAuth: val.DISABLE_PASSWORD_AUTH,
oauth: {
allowDangerousEmailAccountLinking:
val.OAUTH_ALLOW_DANGEROUS_EMAIL_ACCOUNT_LINKING,
Expand Down Expand Up @@ -114,7 +114,7 @@ export const clientConfig = {
demoMode: serverConfig.demoMode,
auth: {
disableSignups: serverConfig.auth.disableSignups,
disablePasswordSignups: serverConfig.auth.disablePasswordSignups,
disablePasswordAuth: serverConfig.auth.disablePasswordAuth,
},
inference: {
inferredTagLang: serverConfig.inference.inferredTagLang,
Expand Down
4 changes: 4 additions & 0 deletions packages/trpc/auth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import * as bcrypt from "bcryptjs";

import { db } from "@hoarder/db";
import { apiKeys } from "@hoarder/db/schema";
import serverConfig from "@hoarder/shared/config";

// API Keys

Expand Down Expand Up @@ -79,6 +80,9 @@ export async function hashPassword(password: string) {
}

export async function validatePassword(email: string, password: string) {
if (serverConfig.auth.disablePasswordAuth) {
throw new Error("Password authentication is currently disabled");
}
const user = await db.query.users.findFirst({
where: (u, { eq }) => eq(u.email, email),
});
Expand Down
8 changes: 8 additions & 0 deletions packages/trpc/routers/apiKeys.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { and, eq } from "drizzle-orm";
import { z } from "zod";

import { apiKeys } from "@hoarder/db/schema";
import serverConfig from "@hoarder/shared/config";

import { authenticateApiKey, generateApiKey, validatePassword } from "../auth";
import { authedProcedure, publicProcedure, router } from "../index";
Expand Down Expand Up @@ -74,6 +75,13 @@ export const apiKeysAppRouter = router({
.output(zApiKeySchema)
.mutation(async ({ input }) => {
let user;
// Special handling as otherwise the extension would show "username or password is wrong"
if (serverConfig.auth.disablePasswordAuth) {
throw new TRPCError({
message: "Password authentication is currently disabled",
code: "FORBIDDEN",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think UNAUTHORIZED is more suitable here.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/401

A 401 Unauthorized is similar to the 403 Forbidden response, except that a 403 is returned when a request contains valid credentials, but the client does not have permissions to perform a certain action.

In that case, we don't have valid creds. So I think a 401 is more suitable. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. I put the reason for this in the comment above. If we return 401, the extension will show "Wrong username or password", which is not correct, since they might be correct, they might be wrong. We did not check. Additionally it will make problem solving harder, since it is very misleading to tell the users that their credentials are wrong, when they are possibly correct, but the admin decided to turn off the login via credentials. I have looked at changing the way we show the message in the extension, but that would mean just taking whatever the server sends as a message, which is currently very revealing (e.g. user exists, but password is wrong), so that is not good from a security perspective.
  2. I find "forbidden" much better as it is actively forbidden to sign in this way. It does not fulfill the first part of the spec though (valid credentials, as we never check), but the second part is fulfilled. But I also do not find 401 correct, since you might have valid credentials, but we did not check.

});
}
try {
user = await validatePassword(input.email, input.password);
} catch (e) {
Expand Down
4 changes: 2 additions & 2 deletions packages/trpc/routers/users.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,9 @@ export const usersAppRouter = router({
.mutation(async ({ input, ctx }) => {
if (
serverConfig.auth.disableSignups ||
serverConfig.auth.disablePasswordSignups
serverConfig.auth.disablePasswordAuth
) {
const errorMessage = serverConfig.auth.disablePasswordSignups
const errorMessage = serverConfig.auth.disablePasswordAuth
? "Local Signups are disabled in the server config. Use OAuth instead!"
: "Signups are disabled in server config";
throw new TRPCError({
Expand Down
Loading