Auth appended to the routing, no_auth + database_auth injected #5789

matborowczyk · 2024-05-31T07:55:03Z

Description

Auth appended to the routing, no_auth injected, database_auth partially injected.

Request for the review as proposed to just verify if the approach is correct with what was agreed

closes #5759

Self Check:

Strike through any lines that are not applicable (~~line~~) then check the box

Attached issue to pull request
Changelog entry
Code is clear and sufficiently documented
Sufficient test cases (reproduces the bug/tests the requested feature)
Correct, in line with design
End user documentation is included or an issue is created for end-user documentation (add ref to issue here: )

src/Data/Auth/AuthProvider.tsx

src/Data/Auth/Providers/AuthContext.ts

bartv · 2024-05-31T12:51:02Z

src/Data/Auth/Providers/DatabaseAuthProvider.tsx

+  children: React.ReactNode;
+}
+
+const DatabaseAuthProvider = ({ children }: AuthProviderProps) => {


Why use the context and provider indirection? If the provider has the same interface you only construct this one and the context is the same for all of them?

We create context in the AuthContext.ts file, which then is accessed in the application through provider, where we also create distinction in the logic between each implementation. I don't know if I explained it clearly enough.

LukasStordeur

I see you're mixing the namings of the files and the folders.

src/Data/Auth/AuthProvider.tsx

src/Data/Auth/Providers/DatabaseAuthProvider.tsx

src/Data/Auth/Providers/AuthContext.ts

src/Data/Auth/Providers/DatabaseAuthProvider.tsx

src/Data/Auth/Providers/KeycloakAuthContext.tsx

src/Data/Auth/AuthProvider.tsx

src/Data/Auth/Providers/DatabaseAuthProvider.tsx

src/Data/Auth/Providers/NoAuthProvider.tsx

src/Data/Auth/Providers/KeycloakAuthContext.tsx

…59-auth-refactor

LukasStordeur

I didn't focus on missing docstrings for now :)

src/Data/Auth/Providers/AuthContext.ts

src/Data/Auth/Providers/KeycloakAuthProvider.tsx

LukasStordeur · 2024-06-12T14:41:50Z

src/Data/Auth/Providers/NoAuthProvider.tsx

+      value={{
+        ...defaultAuthContext,
+        getToken: () => {
+          return "No token";


If you have refactored the file with the interface, you'll see that you won't need the override of the value here and can simply either return the <GetAuthContext.Provider>{children}</GetAuthContext.Provider>
or do that in the switch case and remove this file; choice is yours on that. I don't mind having it in a separate file for matter of cleaner cut.

As far as I know it's not possible to remove assignment of the "value" property. the interface assigned to the context is correct, but still errors is there:
Property 'value' is missing in type '{ children: ReactNode; }' but required in type 'ProviderProps<AuthContextInterface>'

I don't get why you need to override the defaults for no auth when no auth is the default. You can use the context as is in that case.

const authContext= useContext(AuthContext); // rest of code return ( <AuthContext.Provider value={authContext}> {children} </AuthContext.Provider> )

src/UI/Dependency/Dependency.tsx

LukasStordeur · 2024-06-12T14:45:11Z

src/UI/Dependency/Dependency.tsx

@@ -87,6 +94,7 @@ export const DependencyProvider: React.FC<{
      authHelper: authHelper || new DummyAuthHelper(),
      archiveHelper: archiveHelper || new DummyArchiveHelper(),
      authController: authController || new DummyAuthController(),
+      useAuth: useAuth || defaultAuthContext,


defaultAuthContext not needed anymore if it's for free in the initialization in the authContext.ts file

dependencies: Partial<Dependencies> indicates that any of dependencies properties can be undefined so it's required for the sake of the typescript(I think also tests would react differently, but that I didn't check, and I'm not 100% sure)

If you update authContext.ts based on the example I gave you, useAuth will never be undefined because it has a default initialization, which is technically the noAuth one.

but undefined that is addressed here doesn't come from the useAuth itself but from the Partial<Dependencies> . authContext file got correct type

bartv · 2024-06-13T06:18:04Z

src/Data/Auth/AuthProvider.tsx

+/**
+ * AuthProvider component renders the appropriate authentication provider based on the provided config
+ *
+ * @param config - The authentication configuration.


This "interface" should be 100% generic. I see config which makes me assume that this is the configuration provided by the backend but then there is still a keycloak specific parameter. This should not be the case:

It should be clear what config is and I cannot deduce that from the code, the naming or the comments

We should not have any authentication method specific parameters in here anymore.

changed, and removed url parameter

src/Data/Auth/Providers/NoAuthProvider.tsx

src/Data/Auth/Providers/AuthContext.ts

bartv · 2024-06-13T06:23:07Z

src/Data/Auth/Providers/AuthContext.ts

+/**
+ * Interface for the authentication context properties.
+ */
+export interface GetAuthProvider {


I don't understand why an interface can have an action verb (get) in it's name. It describes how to interact with code that implements this.

src/Data/Managers/V2/AddUser/useAddUser.ts

Co-authored-by: Bart Vanbrabant <[email protected]>

src/Data/Auth/AuthContext.ts

…59-auth-refactor

LukasStordeur · 2024-06-20T12:17:19Z

src/Data/Auth/KeycloakProvider.tsx

+  method: "oidc";
+}
+
+export interface LocalConfig {


Why is this one in the KeycloakProvider?

That was the top place that requires it, and when I had it in AuthProvider file it created circular dependency and I didn't feel like type.ts file is required here, but I can be mistaken.

Moving an export to another file should not be triggering circular dependency. If you have a circular dep issue, you might have forgotten to remove the import statement if you moved it to the file where it's used or adjust it.

it does, at least madge complains when the interface is in the authprovider, app and tests works just fine

I believe it's only used in the authProvider component, so either;

don't export it and declare it as an interface inside AuthProvider.

since we are using a switch case on the config.method, perhaps this distinction isn't needed anymore and we can go for a more generic type which has the different possibilities for 'method : "oidc" | "database" | "other" | undefined

Edit: you have it in your tests, in two place, so AuthProvider can export it there, but you need to remove the LocalConfig from your imports in that file.

It's reused, and in case of "oidc" the rest of the config is being passed to the keycloakProvider to create keycloak instance, I'll put it to the types.ts file to have it in "more correct" place

LukasStordeur · 2024-06-20T12:18:43Z

src/Data/Auth/AuthProvider.tsx

+      return <DatabaseAuthProvider>{children}</DatabaseAuthProvider>;
+    case "oidc":
+      return (
+        <KeycloakProvider config={config}>


This doesn't make sense to me. Any reason why you wrapped it like this instead of having the entire implementation in one file?

I tried it, but when it was in one file I couldn't reach the keycloak instance from the keycloakprovider. The logic has to be one level below initialisation of the context I guess

A full application in React could hypothetically be written in one single component, we are splitting it up for readability and organization, but technically speaking, everything that can be achieved with multiple components can be done in one.

If you can't figure it out, which is fine, make a ticket for it, and at least, place all the elements/wrappers/parts concerning Keycloak in on single file. You should only call one Provider for Keycloak here.

Of course it could be done, but then there would be no need for useContext. And as far as I know and searched context provider has to be one level higher that the component where we want to use given context.

I'll merge the files.

src/Data/Auth/KeycloakProvider.tsx

src/Data/Auth/AuthProvider.tsx

src/Data/Managers/V2/helpers/useHelpers.ts

src/Slices/Agents/UI/Agents.test.tsx

src/Test/Inject/TestInjector.tsx

src/UI/Root/Components/PrivateRoute/PrivateRoute.tsx

src/UI/Root/Root.test.tsx

LukasStordeur · 2024-06-25T08:47:43Z

oss-tests results

src/Data/Auth/Providers/KeycloakAuthProvider.tsx

src/Slices/Login/Page.tsx

src/Slices/ServiceInventory/UI/Spec/ServiceInventoryPrepper.tsx

LukasStordeur · 2024-06-25T13:23:49Z

src/Slices/ServiceInventory/UI/Spec/ServiceInventoryPrepper.tsx

+              <ServiceInventory serviceName={service.name} service={service} />
+            </StoreProvider>
+          </DependencyProvider>
+        </AuthProvider>{" "}


Suggested change

</AuthProvider>{" "}

</AuthProvider>

LukasStordeur · 2024-06-25T13:28:07Z

src/UI/Root/Components/Header/EnvSelector/EnvSelector.tsx

-            <StyledDiv>
+          <StyledDiv>
+            <div>
+              {authHelper.isDisabled() ? null : (


{!authHelper.isDisabled() && <StyledText>{authHelper.getUser()}</StyledText>}

LukasStordeur · 2024-06-25T13:29:57Z

src/UI/Root/Root.tsx

 import { ReactQueryDevtools } from "@tanstack/react-query-devtools";
 import { LoginPage } from "@/Slices/Login";
 import { DependencyContext } from "@/UI/Dependency";
 import { SearchSanitizer } from "@/UI/Routing";
 import { GlobalStyles } from "@/UI/Styles";
 import { NotFoundPage } from "@S/NotFound/UI";
-import { KeycloakProvider, PageFrame, Initializer } from "./Components";
+import RouteOutlet from "../Routing/RouteOutlet/RouteOutlet";


Why have a separate folder for RouteOutlet?
And if you really want a separate folder, it should use an index file.

LukasStordeur · 2024-06-25T13:30:54Z

src/UI/Routing/RouteOutlet/RouteOutlet.tsx

+  );
+};
+
+export default RouteOutlet;


avoid export defaults

LukasStordeur · 2024-06-25T13:47:40Z

src/UI/Root/Components/Header/EnvSelector/EnvSelector.tsx

-            <StyledDiv>
+          <StyledDiv>
+            <div>
+              {!authHelper.isDisabled() ? (


{!authHelper.isDisabled() && {authHelper.getUser()}}

There's no need for the ternary here. You changed the wrong line, there was nothing wrong with line 47.

LukasStordeur · 2024-06-25T13:54:24Z

src/UI/Routing/RouteOutlet/index.ts

I still don't see the point in having a specific folder for the RouteOutlet. But fine.

Co-authored-by: Lukas <[email protected]>

…sole into issue/5759-auth-refactor

inmantaci · 2024-06-25T14:44:07Z

Processing this pull request

inmantaci · 2024-06-25T14:44:09Z

Pull request rejected by merge tool. No change entry file found

inmantaci · 2024-06-25T15:19:07Z

Processing this pull request

…nable and adjustable for new authentication providers (Issue #5759, PR #5789) # Description Auth appended to the routing, no_auth injected, database_auth partially injected. Request for the review as proposed to just verify if the approach is correct with what was agreed closes #5759 # Self Check: Strike through any lines that are not applicable (`~~line~~`) then check the box - [ ] Attached issue to pull request - [ ] Changelog entry - [ ] Code is clear and sufficiently documented - [ ] Sufficient test cases (reproduces the bug/tests the requested feature) - [ ] Correct, in line with design - [ ] End user documentation is included or an issue is created for end-user documentation (add ref to issue here: )

inmantaci · 2024-06-25T15:19:18Z

Merged into branches master, iso7 in 36a46bb

Auth appended to the routing, no_auth + database_auth injected

9e30197

matborowczyk requested review from bartv and LukasStordeur May 31, 2024 07:55

bartv reviewed May 31, 2024

View reviewed changes

LukasStordeur reviewed May 31, 2024

View reviewed changes

matborowczyk added 3 commits June 11, 2024 15:08

Merge branch 'master' of github.com:inmanta/web-console into issue/57…

de73fad

…59-auth-refactor

remarks

fbea487

comments

6f5c97b

matborowczyk requested review from bartv and LukasStordeur June 12, 2024 10:05

LukasStordeur reviewed Jun 12, 2024

View reviewed changes

bartv assigned matborowczyk Jun 13, 2024

bartv requested changes Jun 13, 2024

View reviewed changes

matborowczyk and others added 3 commits June 14, 2024 12:53

remarks + keycloak part init

74aecc5

Update src/Data/Auth/Providers/NoAuthProvider.tsx

304c4ef

Co-authored-by: Bart Vanbrabant <[email protected]>

keycloak

891d131

github-advanced-security bot found potential problems Jun 18, 2024

View reviewed changes

src/Data/Auth/AuthContext.ts Fixed Show fixed Hide fixed

matborowczyk added 11 commits June 18, 2024 09:34

fix

9ade268

fix majority of tests

5ba6171

test fix

9351dec

remove old auth related components

cd02b09

fix location of test injector

0651624

tsc fix

a19e5c2

Merge branch 'master' of github.com:inmanta/web-console into issue/57…

20bd3b0

…59-auth-refactor

circ deps fix

a62ab53

remarks

6f57fa0

rename folder

08cb277

remarks

544aa65

matborowczyk marked this pull request as ready for review June 20, 2024 12:19

LukasStordeur reviewed Jun 20, 2024

View reviewed changes

matborowczyk added 2 commits June 25, 2024 13:51

remarks

36179cf

remark + test fix

12d22e4

matborowczyk requested a review from LukasStordeur June 25, 2024 13:05

LukasStordeur reviewed Jun 25, 2024

View reviewed changes

remarks

434e101

matborowczyk requested a review from LukasStordeur June 25, 2024 13:43

LukasStordeur reviewed Jun 25, 2024

View reviewed changes

matborowczyk and others added 6 commits June 25, 2024 15:54

Update src/Slices/Login/Page.tsx

6c7db40

Co-authored-by: Lukas <[email protected]>

Update src/Data/Auth/Providers/KeycloakAuthProvider.tsx

a07eea7

Co-authored-by: Lukas <[email protected]>

Update src/Data/Auth/Providers/KeycloakAuthProvider.tsx

d605031

Co-authored-by: Lukas <[email protected]>

remark

0076f66

Merge branch 'issue/5759-auth-refactor' of github.com:inmanta/web-con…

ff27a89

…sole into issue/5759-auth-refactor

remove folder

a96d646

matborowczyk requested a review from LukasStordeur June 25, 2024 14:01

LukasStordeur approved these changes Jun 25, 2024

View reviewed changes

bartv approved these changes Jun 25, 2024

View reviewed changes

matborowczyk mentioned this pull request Jun 25, 2024

Look for alternatives for Madge #5752

Open

matborowczyk added the merge-tool-ready This ticket is ready to be merged in label Jun 25, 2024

inmantaci removed the merge-tool-ready This ticket is ready to be merged in label Jun 25, 2024

changelog

1da12b9

matborowczyk added the merge-tool-ready This ticket is ready to be merged in label Jun 25, 2024

inmantaci mentioned this pull request Jun 25, 2024

Auth appended to the routing, no_auth + database_auth injected (#5789) #5843

Closed

inmantaci closed this Jun 25, 2024

inmantaci deleted the issue/5759-auth-refactor branch June 25, 2024 15:19

Auth appended to the routing, no_auth + database_auth injected #5789

Auth appended to the routing, no_auth + database_auth injected #5789

Conversation

matborowczyk commented May 31, 2024

Description

Self Check:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasStordeur left a comment

Choose a reason for hiding this comment

LukasStordeur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasStordeur Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasStordeur commented Jun 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inmantaci commented Jun 25, 2024

inmantaci commented Jun 25, 2024

inmantaci commented Jun 25, 2024

inmantaci commented Jun 25, 2024

LukasStordeur Jun 21, 2024 •

edited

Loading