Css 6646/callback endpoint #1170

ale8k · 2024-03-04T10:09:01Z

Description

feat(browser flow for dashboard): implements browser flow (without sessions)

This PR includes a small refactor to the admin device flow, such that it can share the same identity
update logic within the browser flow.

Engineering checklist

Check only items that apply

Documentation updated
Covered by unit tests
Covered by integration tests

Test instructions

Notes for code reviewers

As discussed on call, we integration test only the handler and avoid mocks to see the behaviour is as expected. This is also true when we come to implements the callback logic and are required to start the flow from scratch. The current test in auth_handler_test will be updated to cover the entire flow when implementing /callback. As for the state todo, I need to see how to track the state between requests and the TODO will be completed in the /callback PR. 6646

…ssions) This PR includes a small refactor to the admin device flow, such that it can share the same identity update logic within the browser flow.

…and auth svc not nill

… css-6646/callback-endpoint

internal/auth/oauth2.go

internal/jimmhttp/auth_handler.go

alesstimec · 2024-03-04T13:25:32Z

internal/jimmhttp/auth_handler_test.go

+// the final callback to the dashboard emulating an endpoint. See setupTestServer
+// where we create an additional handler to simulate the final callback to the dashboard
+// from JIMM.
+func TestBrowserAuth(t *testing.T) {


this only tests the "happy path".. we would need additional tests..

Yeah... But I'm not sure what or how?

exchange fails, update identity fails, etc..

I'll try do some more where we check this stuff before merging this PR, just might need to experiment a little and then I'll @ you

@alesstimec

alesstimec · 2024-03-05T11:36:54Z

internal/auth/oauth2.go

@@ -203,6 +241,34 @@ func (as *AuthenticationService) VerifySessionToken(token string, secretKey stri
 	return VerifySessionToken(token, secretKey)
 }

+// UpdateIdentity updates the database with the display name and access token set for the user.
+// And, if present, a refresh token.
+func (as *AuthenticationService) UpdateIdentity(ctx context.Context, email string, token *oauth2.Token) error {


is this unit tested?

also.. it doesn't make sense to pass in email as a separate parameter.. you could just call

idToken, err := as.ExtractAndVerifyIDToken(ctx, token) if err != nil { writeError(ctx, w, http.StatusBadRequest, err, "failed to extract and verify id token") return } email, err := email(idToken)

here to get the email from the token

It isn't tested individually it's part of the TestDevice, I need to come back and mock all of this out afterwards. That's the plan anyways, so I guess view the tests here as temporary, I just written them to help develop. And it's separate as I think the extracting email from a field that can error out is better done outside of the function? Happy to call .Email() within updateidentity but I feel this is clearer / more obvious?

also in next pr we actually use the email in another function within the handler, so i'll leave this as is

We pass it over to the session creation

// If the session is empty, it'll just be an empty session, we only check // errors for bad decoding etc. session, err := oah.SessionStore.Get(r, "jimm-browser-session") if err != nil { writeError(ctx, w, http.StatusBadRequest, err, "failed to get session") } session.Values["jimm-session"] = email

internal/dbmodel/sql/postgres/1_6.sql

internal/jimmhttp/auth_handler.go

alesstimec · 2024-03-05T11:42:24Z

internal/jimmhttp/auth_handler.go

 	AuthCodeURL() string
+	Exchange(ctx context.Context, code string) (*oauth2.Token, error)
+	ExtractAndVerifyIDToken(ctx context.Context, oauth2Token *oauth2.Token) (*oidc.IDToken, error)
+	Email(idToken *oidc.IDToken) (string, error)


Email does not need to be a method of the authenticator.. it can be an independent function because it just extracts an email from the id token that's passed in

Answered in the other comment why I did this

alesstimec · 2024-03-05T11:45:38Z

internal/auth/oauth2.go

@@ -71,6 +87,7 @@ func NewAuthenticationService(ctx context.Context, params AuthenticationServiceP
 			RedirectURL:  params.RedirectURL,
 		},
 		sessionTokenExpiry: params.SessionTokenExpiry,
+		db:                 params.Store,


some validation of input parameters should be done here, no? like.. store should not be nil, sessionTokenExpiry should not be 0.. i'm sure there's other validation we could do

I feel like validation should be done at the service.go level, but happy to add more in?

internal/jimmhttp/auth_handler.go

mina1460 · 2024-03-06T11:11:51Z

internal/auth/oauth2.go

+	u := &dbmodel.Identity{
+		Name: email,
+	}
+	// TODO(babakks): If user does not exist, we will create one with an empty


Should we create a ticket for this and link it here?

I think one might exist... i am a bit confused by it though. trhis was more babaks suggestion

internal/auth/oauth2.go

internal/jimm/user_test.go

mina1460 · 2024-03-06T11:20:45Z

internal/jimmhttp/auth_handler.go

+		return nil, errors.E("nil authenticator")
+	}
+	if dashboardFinalRedirectURL == "" {
+		return nil, errors.E("final redirect url not specified")


This is good enough, but I think we should give it another thought. All the tests are created with "no dashboard required for this test", which could have been avoided if this was not required here as we return an error.

Moreover, checking on empty strings shouldn't be enough, we need to actually verify the url is parseable, valid, and reachable. If not, return an error, too

This is because the auth handler has validation, which I agree with Ales it was necessary, just our other handlers don't have the same validation... Because we embed into jimm a lot we end up with this polluted mess of creating stuff for the sake of creating it. And ah yeah definitely, I'll add a url parse to next PR totally agree.

mina1460 · 2024-03-06T11:21:12Z

service_test.go

@@ -56,6 +56,7 @@ func TestDefaultService(t *testing.T) {
 			Scopes:             []string{oidc.ScopeOpenID, "profile", "email"},
 			SessionTokenExpiry: time.Duration(time.Hour),
 		},
+		DashboardFinalRedirectURL: "<no dashboard needed for this test>",


This is what I was referring to

Yeah I know this one...

mina1460 · 2024-03-06T11:22:12Z

service_test.go

@@ -406,6 +413,7 @@ func TestThirdPartyCaveatDischarge(t *testing.T) {
 					Scopes:             []string{oidc.ScopeOpenID, "profile", "email"},
 					SessionTokenExpiry: time.Duration(time.Hour),
 				},
+				DashboardFinalRedirectURL: "<no dashboard needed for this test>",


why force it if it is never used in the tests?

Other comment explains

ale8k added 4 commits February 28, 2024 11:26

pr comments

b9818e4

pr comments

d36300f

feat(browser flow for dashboard): implements browser flow (without se…

08677e0

…ssions) This PR includes a small refactor to the admin device flow, such that it can share the same identity update logic within the browser flow.

ale8k requested review from alesstimec, kian99, babakks and mina1460 as code owners March 4, 2024 10:09

ale8k added 3 commits March 4, 2024 10:12

feat(validation in auth handler): validates params given are correct …

93c6430

…and auth svc not nill

Merge branch 'feature-oidc' of https://github.com/canonical/jimm into…

a828285

… css-6646/callback-endpoint

Fix tests

b983288

alesstimec reviewed Mar 4, 2024

View reviewed changes

ale8k added 4 commits March 4, 2024 14:03

pr comments

f7c19f9

additional failure tests

86e1093

test fix

c069bca

Update refresh token

ba98f51

alesstimec requested changes Mar 5, 2024

View reviewed changes

alesstimec approved these changes Mar 5, 2024

View reviewed changes

mina1460 approved these changes Mar 6, 2024

View reviewed changes

ale8k merged commit 122abfb into canonical:feature-oidc Mar 6, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Css 6646/callback endpoint #1170

Css 6646/callback endpoint #1170

ale8k commented Mar 4, 2024

alesstimec Mar 4, 2024

ale8k Mar 4, 2024

alesstimec Mar 4, 2024

ale8k Mar 4, 2024

ale8k Mar 5, 2024

alesstimec Mar 5, 2024

alesstimec Mar 5, 2024

ale8k Mar 5, 2024

ale8k Mar 5, 2024

alesstimec Mar 5, 2024

ale8k Mar 6, 2024

alesstimec Mar 5, 2024

ale8k Mar 5, 2024

alesstimec Mar 5, 2024

ale8k Mar 5, 2024

mina1460 Mar 6, 2024

ale8k Mar 6, 2024

mina1460 Mar 6, 2024

ale8k Mar 6, 2024

mina1460 Mar 6, 2024

ale8k Mar 6, 2024

mina1460 Mar 6, 2024

ale8k Mar 6, 2024

Css 6646/callback endpoint #1170

Css 6646/callback endpoint #1170

Conversation

ale8k commented Mar 4, 2024

Description

Engineering checklist

Test instructions

Notes for code reviewers

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

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