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

refactor to go pkg structure and add rb search #7

Merged
merged 9 commits into from Mar 5, 2018

Conversation

Projects
None yet
4 participants
@butlerx
Copy link
Collaborator

commented Feb 26, 2018

project would be renamed to redbrick/rbldap

close #6

@butlerx butlerx force-pushed the butlerx:rb-ldap branch from c720dc9 to 493ef61 Feb 26, 2018

@butlerx butlerx referenced this pull request Feb 26, 2018

Open

add rbuser test #8

@butlerx

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 26, 2018

@@ -1,19 +1,24 @@
# userVhost
# rb-ldap

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Feb 26, 2018

Perhaps repo rename would also be suiting then. But that's more of a question to @redbrick (o however you tag CMT now)

This comment has been minimized.

Copy link
@butlerx

butlerx Feb 26, 2018

Author Collaborator

i did specify a repo rename above

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Feb 26, 2018

Apologies.

Usage: "DCU Active Directory host to query",
},

cli.IntFlag{

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Feb 26, 2018

I think it would be better to have one flag for host:port both for RB and DCU

This comment has been minimized.

Copy link
@butlerx

butlerx Feb 26, 2018

Author Collaborator

I'm not a fan of a single flag as host and port won't change often and its more likely for just one too change then both. aswell in the library, they are handled separately

This comment has been minimized.

Copy link
@butlerx

butlerx Feb 26, 2018

Author Collaborator

I think @CSmartt input on this would be good

This comment has been minimized.

Copy link
@VoyTechnology

This comment has been minimized.

Copy link
@butlerx

butlerx Feb 26, 2018

Author Collaborator

i still would rather have a separate flag for user and port

This comment has been minimized.

Copy link
@CSmartt

CSmartt Feb 27, 2018

+1 for separation, far more likely that one will change rather than both, if at all. Worth remembering also that we're not writing a generic LDAP tool here, so while I can see the case for a combined host:port flag, I think for clarity at our end we should keep them apart.

Name: "dcu-password",
Usage: "password for the DCU ldap server",
FilePath: "/etc/dcu_ldap.secret",
},

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Feb 26, 2018

Even another idea is to combine all those flags into user:pass@host:port. I feel like url.URL would help with parsing. It makes the flags less awkward to use.

This comment has been minimized.

Copy link
@butlerx

butlerx Feb 26, 2018

Author Collaborator

for the squashed syntax again see above.
explain url.URL

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Feb 26, 2018

See https://play.golang.org/p/KAornm0cci1

You let the flag be parsed by url.Parse`. I understand the only problematic bit would be the password, which is understandably another CLI flag loading the password from file.

Edit: The password is optional so it can be omited: https://golang.org/pkg/net/url/#Userinfo.Password

This comment has been minimized.

Copy link
@butlerx

butlerx Feb 26, 2018

Author Collaborator

id MUCH rather keep using the filepath for a password file rather then a password

This comment has been minimized.

Copy link
@butlerx

butlerx Feb 26, 2018

Author Collaborator

that makes changing the variables seem pretty annoying

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Feb 27, 2018

Even if passwords are loaded from file, once we have sane defaults, how often would you actually need to specify it yourself?

Keeping password separate we go down from 8 flags total for RB and DCU LDAP, to just 4 (user@rb:port, user@dcu:port, rb-password, dcu-password). Honestly I have never encountered decent application which forces you to change 4 different flags to log in (and I am not counting mysql as a good example).

But this is just my opinion, I think settling this rests on cmt hands :)

This comment has been minimized.

Copy link
@butlerx

butlerx Feb 27, 2018

Author Collaborator

admins, not cmt
you aren't likely to be using any flags though the default will work out of the box the flags are only for if something changes

This comment has been minimized.

Copy link
@CSmartt

CSmartt Feb 27, 2018

Admin here, hi. Path to file is how this is happening, non-negotiable, I'd really prefer any user couldn't run a ps and see our pass being passed as an arg.


// Generate takes cli context and generates user vhost for rbuser
func Generate(ctx *cli.Context) error {
l, err := newRBLdap(ctx)

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Feb 26, 2018

This looks very weird. I would change it to newRbLdap. Still not ideal but at least consistent.

This comment has been minimized.

Copy link
@butlerx

butlerx Feb 26, 2018

Author Collaborator

how is Rb consistent compared to RB

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Feb 26, 2018

If you only capitalise L in Ldap then only R should be capitalised in Rb.

This comment has been minimized.

Copy link
@butlerx

butlerx Feb 26, 2018

Author Collaborator

hmmm well fine then

This comment has been minimized.

Copy link
@butlerx

butlerx Feb 26, 2018

Author Collaborator

what about newDCULdap then?

if err != nil {
return err
}
err = file.Sync()

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Feb 26, 2018

I would move this err straight to return since it's not used in the next line.

if ctx.Bool("noob") {
noob = "(newbie=TRUE)"
}
user, searchErr := rbuser.SearchRB(rb, filterAnd(filter("cn", name), filterOr(filter("uid", ctx.String("user")), filter("gecos", ctx.String("user"))), id, mail, noob))

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Feb 26, 2018

God plz split this ;_;

return 0, err
}

func Generate(l *ldap.Conn, output string) ([]string, error) {

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Feb 26, 2018

I feel like this is a weird to pass ldap.Conn. I feel like LDAP connection should be in its own structure, with GenerateVHost function.

This comment has been minimized.

Copy link
@butlerx

butlerx Feb 26, 2018

Author Collaborator

a struct just for passing ldap.Conn?

@@ -0,0 +1,74 @@
package rbuser

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Feb 26, 2018

File should be called rbuser.go

const output = `
User Information
================
{{ with .UID }}uid: {{ . }}

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Feb 26, 2018

Wouldn't uid: {{ .UID }} not be better? What's the reason to use with?

This comment has been minimized.

Copy link
@butlerx

butlerx Feb 26, 2018

Author Collaborator

with is basically an if statement it means it only prints if the variable exists

)

// SearchRB search redbrick ldap for a given filter and return first user that matches
func SearchRB(l *ldap.Conn, filter string) (RBUser, error) {

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Feb 26, 2018

This would be better as part of type RBLDAP struct. It is not usual to pass connection like this.

This comment has been minimized.

Copy link
@butlerx

butlerx Feb 26, 2018

Author Collaborator

structure of searchDCU would need to be reconsidered for that

@butlerx butlerx force-pushed the butlerx:rb-ldap branch from cb20230 to 00f1d0c Feb 26, 2018

@butlerx butlerx force-pushed the butlerx:rb-ldap branch from 00f1d0c to 228e5e6 Feb 26, 2018


cli.BoolFlag{
Name: "dry-run",
Usage: "output too console rather then file",

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Feb 26, 2018

nit(typo): s/too/to

cli.StringFlag{
Name: "conf, c",
Value: "./user_vhost_list.conf",
Usage: "File to output conf too",

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Feb 26, 2018

I would change that to Output configuration `FILE` . It would generate slightly nicer help string.

@butlerx butlerx force-pushed the butlerx:rb-ldap branch from d292719 to d6e3574 Feb 26, 2018

"github.com/urfave/cli"
)

func newRbLdap(ctx *cli.Context) (*rbuser.RbLdap, error) {

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Feb 27, 2018

I would add a TODO to not use cli.Context, in the future, seeing the maintenance track records of redbrick projects a TODO will help to show what is wrong, in case aliens abduct you and some poor admin takes care of it in the future.

This is also a prefect example where passing in url.URL would make much more sense ;)

func Search(ctx *cli.Context) error {
mail := filter("altmail", ctx.String("mail"))
id := filter("id", ctx.String("id"))
var re = regexp.MustCompile(`\ `)

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Feb 27, 2018

Any particular reason why this uses var instead of :=? I have nothing against one method or another unless they are consistent or there is a reason.

if err != nil {
return err
}
user, searchErr := dcu.Search(filterAnd(

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Feb 27, 2018

In this case overwriting err will have no side effects. searchErr -> err

return RbUser{}, err
}

func courseYear(whyNotTheSame string) (string, int) {

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Feb 27, 2018

Variable name left unchanged after debugging? Perhaps

func splitCourseYear(courseYear string) (string, int)

would be a better function signature in this case?

This comment has been minimized.

Copy link
@butlerx

butlerx Feb 27, 2018

Author Collaborator

you know youre the one who named this function

)

// LdapConf Server object used for connecting to server
type LdapConf struct {

This comment has been minimized.

Copy link
@VoyTechnology

VoyTechnology Feb 27, 2018

I feel like this should have only one New function declaration. It is also being created in internal/pkg/rbldap/ldap.go using cli.Context. Ideally that package should call NewLdapConf which is defined here. That way these variables in the struct don't need to be public, because they are never used outside LdapConf's functions.

This comment has been minimized.

Copy link
@butlerx

butlerx Feb 27, 2018

Author Collaborator

this doesn't really make sense this doesn't have any new function calls

butlerx added some commits Feb 27, 2018

@butlerx butlerx force-pushed the butlerx:rb-ldap branch from 00e79ce to b1a084e Feb 27, 2018

@GoldenBadger GoldenBadger merged commit 2024fb8 into redbrick:master Mar 5, 2018

@butlerx butlerx deleted the butlerx:rb-ldap branch Mar 5, 2018

butlerx added a commit to butlerx/rb-ldap that referenced this pull request Oct 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.