Skip to content

Commit

Permalink
[BUGFIX] argilla frontend: redirect after login (#5635)
Browse files Browse the repository at this point in the history
# Description
<!-- Please include a summary of the changes and the related issue.
Please also include relevant motivation and context. List any
dependencies that are required for this change. -->

Closes #5633

**Type of change**
<!-- Please delete options that are not relevant. Remember to title the
PR according to the type of change -->

- Bug fix (non-breaking change which fixes an issue)
- New feature (non-breaking change which adds functionality)

**How Has This Been Tested**
<!-- Please add some reference about how your feature has been tested.
-->

**Checklist**
<!-- Please go over the list and make sure you've taken everything into
account -->

- I added relevant documentation
- I followed the style guidelines of this project
- I did a self-review of my code
- I made corresponding changes to the documentation
- I confirm My changes generate no new warnings
- I have added tests that prove my fix is effective or that my feature
works
- I have added relevant notes to the CHANGELOG.md file (See
https://keepachangelog.com/)

---------

Co-authored-by: José Francisco Calvo <[email protected]>
Co-authored-by: José Francisco Calvo <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
  • Loading branch information
4 people committed Nov 6, 2024
1 parent d9dce94 commit 229809e
Show file tree
Hide file tree
Showing 7 changed files with 47 additions and 23 deletions.
7 changes: 6 additions & 1 deletion argilla-frontend/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,12 @@ These are the section headers that we use:
## [Unreleased]()

### Added
- Add a high-contrast theme & improvements for the forced-colors mode ([#5661](https://github.com/argilla-io/argilla/pull/5661))

- Add a high-contrast theme & improvements for the forced-colors mode. ([#5661](https://github.com/argilla-io/argilla/pull/5661))

### Fixed

- Fixed redirection problems after users sign-in using HF OAuth. ([#5635](https://github.com/argilla-io/argilla/pull/5635))

## [2.4.0](https://github.com/argilla-io/argilla/compare/v2.3.0...v2.4.0)

Expand Down
15 changes: 7 additions & 8 deletions argilla-frontend/middleware/route-guard.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,23 +17,25 @@

import { Context } from "@nuxt/types";
import { useRunningEnvironment } from "~/v1/infrastructure/services/useRunningEnvironment";
import { useLocalStorage } from "~/v1/infrastructure/services";

const { set } = useLocalStorage();

export default ({ $auth, route, redirect }: Context) => {
const { isRunningOnHuggingFace } = useRunningEnvironment();

// By-pass unknown routes. This is needed to avoid errors with API calls.
if (route.name == null) return;

switch (route.name) {
case "sign-in":
if ($auth.loggedIn) return redirect("/");

if (route.params.omitCTA) return;

if (isRunningOnHuggingFace()) {
// eslint-disable-next-line @typescript-eslint/no-unused-vars
const { redirect: _, ...query } = route.query;

return redirect({
name: "welcome-hf-sign-in",
query,
});
}
break;
Expand All @@ -50,14 +52,11 @@ export default ({ $auth, route, redirect }: Context) => {
default:
if (!$auth.loggedIn) {
if (route.path !== "/") {
route.query.redirect = route.fullPath;
set("redirectTo", route.fullPath);
}

redirect({
name: "sign-in",
query: {
...route.query,
},
});
}
}
Expand Down
14 changes: 12 additions & 2 deletions argilla-frontend/pages/oauth/_provider/useOAuthViewModel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,11 @@ import { useFetch, useRoute } from "@nuxtjs/composition-api";
import { useResolve } from "ts-injecty";
import { ProviderType } from "~/v1/domain/entities/oauth/OAuthProvider";
import { OAuthLoginUseCase } from "~/v1/domain/usecases/oauth-login-use-case";
import { useRoutes, useTranslate } from "~/v1/infrastructure/services";
import {
useRoutes,
useTranslate,
useLocalStorage,
} from "~/v1/infrastructure/services";
import { useNotifications } from "~/v1/infrastructure/services/useNotifications";

export const useOAuthViewModel = () => {
Expand All @@ -11,24 +15,30 @@ export const useOAuthViewModel = () => {
const routes = useRoute();
const router = useRoutes();
const oauthLoginUseCase = useResolve(OAuthLoginUseCase);
const { pop } = useLocalStorage();

useFetch(async () => {
await tryLogin();
});

const redirect = () => {
const redirect = pop("redirectTo");
router.go(redirect || "/");
};

const tryLogin = async () => {
const { params, query } = routes.value;

const provider = params.provider as ProviderType;

try {
await oauthLoginUseCase.login(provider, query);
redirect();
} catch {
notification.notify({
message: t("argilla.api.errors::UnauthorizedError"),
type: "danger",
});
} finally {
router.go("/");
}
};
Expand Down
10 changes: 0 additions & 10 deletions argilla-frontend/pages/sign-in.vue
Original file line number Diff line number Diff line change
Expand Up @@ -98,18 +98,8 @@ export default {
},
},
methods: {
nextRedirect() {
const redirect_url = this.$nuxt.$route.query.redirect || "/";
this.$router.push({
path: redirect_url,
});
},
async loginUser({ username, password }) {
await this.login(username, password);

this.$notification.clear();

this.nextRedirect();
},
async onLoginUser() {
try {
Expand Down
12 changes: 12 additions & 0 deletions argilla-frontend/pages/useSignInViewModel.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,23 @@
import { useResolve } from "ts-injecty";
import { AuthLoginUseCase } from "~/v1/domain/usecases/auth-login-use-case";
import { useRoutes, useLocalStorage } from "~/v1/infrastructure/services";
import { useNotifications } from "~/v1/infrastructure/services/useNotifications";

export const useSignInViewModel = () => {
const useCase = useResolve(AuthLoginUseCase);
const router = useRoutes();
const notification = useNotifications();
const { pop } = useLocalStorage();

const redirect = () => {
const redirect = pop("redirectTo");
router.go(redirect || "/");
};

const login = async (username: string, password: string) => {
await useCase.login(username, password);
notification.clear();
redirect();
};

return {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
type Options = "showShortcutsHelper" | "layout";
type Options = "showShortcutsHelper" | "layout" | "redirectTo";

const STORAGE_KEY = "argilla";

Expand Down Expand Up @@ -34,8 +34,15 @@ export const useLocalStorage = () => {
} catch {}
};

const pop = (key: Options) => {
const value = get(key);
set(key, null);
return value;
};

return {
get,
set,
pop,
};
};
3 changes: 2 additions & 1 deletion argilla-server/tests/unit/api/handlers/v1/test_oauth2.py
Original file line number Diff line number Diff line change
Expand Up @@ -89,14 +89,15 @@ async def test_provider_huggingface_authentication(
):
with mock.patch("argilla_server.security.settings.Settings.oauth", new_callable=lambda: default_oauth_settings):
response = await async_client.get(
"/api/v1/oauth2/providers/huggingface/authentication", headers=owner_auth_header
"/api/v1/oauth2/providers/huggingface/authentication?extra=params", headers=owner_auth_header
)
assert response.status_code == 303

redirect_url = URL(response.headers.get("location"))
assert redirect_url.scheme == b"https"
assert redirect_url.host == b"huggingface.co"
assert b"/oauth/authorize?response_type=code&client_id=client_id" in redirect_url.target
assert b"&extra=params" in redirect_url.target

async def test_provider_authentication_with_oauth_disabled(
self,
Expand Down

0 comments on commit 229809e

Please sign in to comment.