-
Notifications
You must be signed in to change notification settings - Fork 1
Defining recommended session approach for apps #55
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.
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
Conversation
samlown
commented
Jan 28, 2026
- Moves from using Enrollments to Session models that make it easier to handle authentication details when running embedded apps inside the Invopop console.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces a recommended session-based approach for embedded applications to replace the previous enrollment-based authentication method. The changes provide better support for applications running inside the Invopop Console by introducing a new Session model that handles authentication tokens and expiration more gracefully.
Changes:
- Added
Sessionstruct and management methods in the invopop client library for opinionated session handling - Implemented session middleware and cookie-based session storage in echopop package
- Moved enrollment-related functions to a separate file and deprecated the old
AuthEnrollmentmiddleware - Added
TokenExpiresfield to theEnrollmentstruct to support token expiration tracking
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| invopop/access_session.go | New Session struct with authorization, token management, and serialization methods |
| pkg/echopop/session.go | Session middleware for loading/storing sessions from headers or cookies |
| pkg/echopop/service.go | Added session key configuration option and AuthToken helper function |
| pkg/echopop/enrollments.go | Moved enrollment authentication functions from echopop.go (deprecated old middleware) |
| pkg/echopop/echopop.go | Removed enrollment functions (relocated to enrollments.go) |
| invopop/invopop.go | Added WithAuthToken method and deprecated SetAuthToken |
| invopop/errors.go | Added ErrAccessDenied error constant |
| invopop/access_enrollments.go | Added TokenExpires field to Enrollment struct |
| invopop/access.go | Added NewSession and NewSessionWithToken factory methods |
| go.mod, go.sum | Added gorilla/sessions and labstack/echo-contrib dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Try to extract the session from the cookie | ||
| err := extractSessionFromCookie(c, sess) | ||
| if err != nil { | ||
| return fmt.Errorf("extracting session from cookie: %w", err) |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The session middleware returns an error when extracting session from cookie fails. However, for a non-authenticated session load, it might be more appropriate to log the error and continue with an empty session rather than failing the request. Consider whether cookie extraction failures should be treated as soft failures (especially for initial page loads where no session exists yet).
| return fmt.Errorf("extracting session from cookie: %w", err) | |
| // For non-authenticated loads, treat cookie extraction failures as soft: | |
| // log the problem and continue with an empty session instead of failing. | |
| c.Logger().Warnf("failed to extract session from cookie: %v", err) |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>