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

SSO support #2578

Closed
wants to merge 2 commits into from
Closed

SSO support #2578

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
452f7f2
OIDC SSO Support
privacyguard Sep 18, 2024
93620c0
improvements based on review feedback
privacyguard Sep 21, 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
4 changes: 3 additions & 1 deletion src/shared/components/common/password-input.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ interface PasswordInputProps {
label?: string | null;
showForgotLink?: boolean;
isNew?: boolean;
required?: boolean;
}

interface PasswordInputState {
Expand Down Expand Up @@ -77,6 +78,7 @@ class PasswordInput extends Component<PasswordInputProps, PasswordInputState> {
label,
showForgotLink,
isNew,
required,
},
state: { show },
} = this;
Expand All @@ -98,7 +100,7 @@ class PasswordInput extends Component<PasswordInputProps, PasswordInputState> {
autoComplete={isNew ? "new-password" : "current-password"}
onInput={onInput}
value={value}
required
required={required !== false}
Copy link
Member

Choose a reason for hiding this comment

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

Are you using required !== false because you want it to default to true even if it's undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. We wanted to keep it as required by default and add the ability to disable it when needed.

pattern=".+"
title={I18NextService.i18n.t("invalid_password")}
minLength={isNew ? 10 : undefined}
Expand Down
120 changes: 119 additions & 1 deletion src/shared/components/home/admin-settings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,17 @@ import { Component } from "inferno";
import {
BannedPersonsResponse,
CreateCustomEmoji,
CreateOAuthProvider,
DeleteCustomEmoji,
DeleteOAuthProvider,
EditCustomEmoji,
EditOAuthProvider,
EditSite,
GetFederatedInstancesResponse,
GetSiteResponse,
LemmyHttp,
ListMediaResponse,
OAuthProvider,
PersonView,
} from "lemmy-js-client";
import { InitialFetchRequest } from "../../interfaces";
Expand All @@ -32,6 +36,7 @@ import { Spinner } from "../common/icon";
import Tabs from "../common/tabs";
import { PersonListing } from "../person/person-listing";
import { EmojiForm } from "./emojis-form";
import { OAuthProviderForm } from "./oauth-provider-form";
import RateLimitForm from "./rate-limit-form";
import { SiteForm } from "./site-form";
import { TaglineForm } from "./tagline-form";
Expand Down Expand Up @@ -111,6 +116,9 @@ export class AdminSettings extends Component<
this.handleToggleShowLeaveAdminConfirmation =
this.handleToggleShowLeaveAdminConfirmation.bind(this);
this.handleLeaveAdminTeam = this.handleLeaveAdminTeam.bind(this);
this.handleEditOAuthProvider = this.handleEditOAuthProvider.bind(this);
this.handleDeleteOAuthProvider = this.handleDeleteOAuthProvider.bind(this);
this.handleCreateOAuthProvider = this.handleCreateOAuthProvider.bind(this);

// Only fetch the data if coming from another route
if (FirstLoadService.isFirstLoad) {
Expand Down Expand Up @@ -250,7 +258,7 @@ export class AdminSettings extends Component<
>
<div className="row">
<TaglineForm
taglines={this.state.siteRes.taglines}
taglines={this.state.siteRes.taglines || []}
Copy link
Member

Choose a reason for hiding this comment

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

Seems like it wasn't necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We added this today because we were getting taglines as undefined and map function was being called on it.

onSaveSite={this.handleEditSite}
loading={this.state.loading}
/>
Expand Down Expand Up @@ -294,6 +302,27 @@ export class AdminSettings extends Component<
</div>
),
},
{
key: "auth",
label: I18NextService.i18n.t("authentication"),
getNode: isSelected => (
<div
className={classNames("tab-pane", {
active: isSelected,
})}
role="tabpanel"
id="auth-tab-pane"
>
<div className="row">
<OAuthProviderForm
onCreate={this.handleCreateOAuthProvider}
onDelete={this.handleDeleteOAuthProvider}
onEdit={this.handleEditOAuthProvider}
/>
</div>
</div>
),
},
]}
/>
</div>
Expand Down Expand Up @@ -490,4 +519,93 @@ export class AdminSettings extends Component<
snapToTop();
await this.fetchUploadsOnly();
}

async handleEditOAuthProvider(form: EditOAuthProvider) {
this.setState({ loading: true });

const res = await HttpService.client.editOAuthProvider(form);

if (res.state === "success") {
const newOAuthProvider = res.data;
this.setState(s => {
s.siteRes.admin_oauth_providers = (
s.siteRes.admin_oauth_providers ?? []
).map(p => {
return p?.client_id === newOAuthProvider.client_id
? newOAuthProvider
: p;
});
return s;
});
toast(I18NextService.i18n.t("site_saved"));
} else {
toast(I18NextService.i18n.t("couldnt_edit_oauth_provider"), "danger");
}

this.setState({ loading: false });

return res;
}

async handleDeleteOAuthProvider(form: DeleteOAuthProvider) {
this.setState({ loading: true });

const res = await HttpService.client.deleteOAuthProvider(form);

let succeeded = false;
if (res.state === "success") {
this.setState(s => {
s.siteRes.admin_oauth_providers = (
s.siteRes.admin_oauth_providers ?? []
).filter(p => p?.id !== form.id);
return s;
});
toast(I18NextService.i18n.t("site_saved"));
succeeded = true;
} else {
toast(I18NextService.i18n.t("couldnt_delete_oauth_provider"), "danger");
}

this.setState({ loading: false });

return succeeded;
}

async handleCreateOAuthProvider(
form: CreateOAuthProvider,
): Promise<OAuthProvider | null> {
this.setState({ loading: true });

const res = await HttpService.client.createOAuthProvider(form);
let newOAuthProvider: OAuthProvider;

if (res.state === "success") {
newOAuthProvider = res.data;

this.setState(s => {
s.siteRes.admin_oauth_providers = (
s.siteRes.admin_oauth_providers ?? []
).slice();
const index = s.siteRes.admin_oauth_providers.findIndex(
x => x?.id === newOAuthProvider?.id,
);
if (index >= 0) {
Object.assign(
s.siteRes.admin_oauth_providers[index] || {},
Copy link
Member

Choose a reason for hiding this comment

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

I greatly doubt this Object.assign is necessary. Check out the editListImmutable function we have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

editListImmutable seems to require a key in every object. Example:

let list = [{ provider: { id: 1 } }, { provider: { id: 2 } }];
let item = { provider: { id: 2, new: value } };

In our case, the data structure is:

let siteRes = { admin_oauth_providers: [{id: 1}, {id: 2}] };
let item = { id: 2, new: value };

newOAuthProvider,
);
} else {
s.siteRes.admin_oauth_providers.push(newOAuthProvider);
}
});
toast(I18NextService.i18n.t("site_saved"));
this.setState({ loading: false });
return newOAuthProvider;
}

// handle failure
toast(I18NextService.i18n.t("couldnt_create_oauth_provider"), "danger");
this.setState({ loading: false });
return null;
}
}
24 changes: 13 additions & 11 deletions src/shared/components/home/emojis-form.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,17 +45,19 @@ export class EmojiForm extends Component<EmojiFormProps, EmojiFormState> {
private itemsPerPage = 15;
private emptyState: EmojiFormState = {
siteRes: this.isoData.site_res,
customEmojis: this.isoData.site_res.custom_emojis.map((x, index) => ({
id: x.custom_emoji.id,
category: x.custom_emoji.category,
shortcode: x.custom_emoji.shortcode,
image_url: x.custom_emoji.image_url,
alt_text: x.custom_emoji.alt_text,
keywords: x.keywords.map(x => x.keyword).join(" "),
changed: false,
page: 1 + Math.floor(index / this.itemsPerPage),
loading: false,
})),
customEmojis: (this.isoData.site_res.custom_emojis || []).map(
Copy link
Member

Choose a reason for hiding this comment

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

This change was probably inadvertant also.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one also we were getting undefined as custom_emojis.

(x, index) => ({
id: x.custom_emoji.id,
category: x.custom_emoji.category,
shortcode: x.custom_emoji.shortcode,
image_url: x.custom_emoji.image_url,
alt_text: x.custom_emoji.alt_text,
keywords: x.keywords.map(x => x.keyword).join(" "),
changed: false,
page: 1 + Math.floor(index / this.itemsPerPage),
loading: false,
}),
),
page: 1,
};
state: EmojiFormState;
Expand Down
79 changes: 78 additions & 1 deletion src/shared/components/home/login.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,12 @@ import { isBrowser, refreshTheme } from "@utils/browser";
import { getQueryParams } from "@utils/helpers";
import { Component, linkEvent } from "inferno";
import { RouteComponentProps } from "inferno-router/dist/Route";
import { GetSiteResponse, LoginResponse } from "lemmy-js-client";
import {
GetSiteResponse,
LoginResponse,
OAuthProvider,
PublicOAuthProvider,
} from "lemmy-js-client";
import { I18NextService, UserService } from "../../services";
import {
EMPTY_REQUEST,
Expand Down Expand Up @@ -52,6 +57,9 @@ async function handleLoginSuccess(i: Login, loginRes: LoginResponse) {

if (site.state === "success") {
UserService.Instance.myUserInfo = site.data.my_user;
const isoData = setIsoData(i.context);
isoData.site_res.oauth_providers = site.data.oauth_providers;
isoData.site_res.admin_oauth_providers = site.data.admin_oauth_providers;
Copy link
Member

Choose a reason for hiding this comment

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

This is really strange. IsoData is only for the initial fetch, and you should just use siteRes if you need these providers afterward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's been a while since we wrote this... We needed to update the the site_res data because the oauth_providers from site_res changes depending on whether the user is logged in as an admin or not.

When you first load the page (not authenticated), you only get the public oauth_provider data needed to display the SSO login buttons. If you log in as an admin, we override this data. If you log out we override the data too.

That said this is not critical data (it does not include any secrets).

refreshTheme();
}

Expand Down Expand Up @@ -107,6 +115,45 @@ async function handleLoginSubmit(i: Login, event: any) {
}
}

export async function handleUseOAuthProvider(params: {
oauth_provider: OAuthProvider;
username?: string;
prev?: string;
answer?: string;
show_nsfw?: boolean;
}) {
const redirectUri = `${window.location.origin}/oauth/callback`;

const state = crypto.randomUUID();
const requestUri =
params.oauth_provider.authorization_endpoint +
"?" +
[
`client_id=${encodeURIComponent(params.oauth_provider.client_id)}`,
`response_type=code`,
`scope=${encodeURIComponent(params.oauth_provider.scopes)}`,
`redirect_uri=${encodeURIComponent(redirectUri)}`,
`state=${state}`,
].join("&");

// store state in local storage
localStorage.setItem(
"oauth_state",
JSON.stringify({
state,
oauth_provider_id: params.oauth_provider.id,
redirect_uri: redirectUri,
prev: params.prev ?? "/",
username: params.username,
answer: params.answer,
show_nsfw: params.show_nsfw,
expires_at: Date.now() + 5 * 60_000,
}),
);

window.location.assign(requestUri);
}

function handleLoginUsernameChange(i: Login, event: any) {
i.setState(
prevState => (prevState.form.username_or_email = event.target.value.trim()),
Expand Down Expand Up @@ -146,6 +193,7 @@ export class Login extends Component<LoginRouteProps, State> {
super(props, context);

this.handleSubmitTotp = this.handleSubmitTotp.bind(this);
this.handleLoginWithProvider = this.handleLoginWithProvider.bind(this);
}

get documentTitle(): string {
Expand Down Expand Up @@ -174,6 +222,28 @@ export class Login extends Component<LoginRouteProps, State> {
<div className="row">
<div className="col-12 col-lg-6 offset-lg-3">{this.loginForm()}</div>
</div>
{(this.state.siteRes.oauth_providers?.length || 0) > 0 && (
<div className="row">
<div className="col-12 col-lg-6 offset-lg-3">
<span>{I18NextService.i18n.t("or")}</span>
{(this.state.siteRes.oauth_providers ?? []).map(
(oauth_provider: PublicOAuthProvider) => (
<button
className="btn btn-secondary m-2"
onClick={linkEvent(
{ oauth_provider },
this.handleLoginWithProvider,
)}
>
{I18NextService.i18n.t("oauth_login_with_provider", {
provider_name: oauth_provider.display_name,
})}
</button>
),
)}
</div>
</div>
)}
</div>
);
}
Expand All @@ -196,6 +266,13 @@ export class Login extends Component<LoginRouteProps, State> {
return successful;
}

async handleLoginWithProvider(params: { oauth_provider: OAuthProvider }) {
handleUseOAuthProvider({
oauth_provider: params.oauth_provider,
prev: this.props.prev ?? "/",
});
}

loginForm() {
return (
<div>
Expand Down
Loading