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.

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

External client ID setter and getter. #45

Closed
wants to merge 8 commits into from
Closed

External client ID setter and getter. #45

wants to merge 8 commits into from

Conversation

awskii
Copy link
Contributor

@awskii awskii commented Nov 9, 2017

As far as we talking about ID method, i think it's true way - provide to users set their own external identifiers for client. So i implemented methods SetID(id interface{}) and ID() interface{} to provide them such facility. Take a glance and let's discuss that.

awskii and others added 2 commits November 9, 2017 13:18
Signed-off-by: John Preston <artem.tsskiy@gmail.com>
@fclairamb
Copy link
Owner

fclairamb commented Nov 9, 2017

The client driver should be used instead

If you want to store client related data, you can implement it on the client driver. Have a look at the (recently updated) sample ClientDriver, it could have been implemented this way:

// ClientDriver defines a very basic client driver
type ClientDriver struct {
	BaseDir string // Base directory from which to server file
        ID interface{} // <-- ADDED
}

But still...

The original discussion was around logging meaningful information from an app perspective. That's why I proposed to expose the Logger. I'm not so sure now (exposing variables exposes us to many things). We could see it another way, maybe add some logging context:

func (c *clientHandler) AddLoggingContext(keyvals ...interface{}) {
	c.logger = log.With(c.logger, keyvals ...interface{})
}

That way when the user logs in, you could add some user context information. Here is a possible implementation of what that idea would look like on the current sample driver:

// AuthUser authenticates the user and selects an handling driver
func (driver *MainDriver) AuthUser(cc server.ClientContext, user, pass string) (server.ClientHandlingDriver, error) {

	for _, act := range driver.config.Users {
		if act.User == user && act.Pass == pass {
			// If we are authenticated, we can return a client driver containing *our* basedir
			baseDir := driver.BaseDir + string(os.PathSeparator) + act.Dir
			os.MkdirAll(baseDir, 0777)
                        cc.AddLoggingContext( "userName", act.User ); // <-- ADDED
			return &ClientDriver{BaseDir: baseDir, ID: act.User}, nil
		}
	}

	return nil, fmt.Errorf("could not authenticate you")
}

@awskii
Copy link
Contributor Author

awskii commented Nov 10, 2017

@fclairamb I think, introduction of logger context could be helpful and logs will become more meaningful. But there should be a balance: user should have a way to configure what fields he\she wants to see in logs.

@awskii
Copy link
Contributor Author

awskii commented Nov 10, 2017

Also we forgot about exposing current active sessions counter, i create another PR for it.

@fclairamb
Copy link
Owner

I'm not convinced, I think this can be handled on the driver side. My goal is to avoid having any feature that could be implemented at a higher level.

@fclairamb fclairamb closed this Jan 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants